Fix missing require_auth, add checked arithmetic and TTL policy - #1228
Merged
yusuftomilola merged 1 commit intoJul 27, 2026
Merged
Conversation
SC-42: authorization audit Found and fixed a critical authentication gap in two crates. Ten entrypoints took a `caller: Address`, compared it against a registrar allowlist, the asset owner, the stored admin, or the approver set — and never called `caller.require_auth()`. Because `caller` is chosen by whoever builds the transaction, every one of those checks was satisfied by simply naming a privileged address. assetsup: register_asset, update_asset_metadata, transfer_asset_ownership, retire_asset. transfer_asset_ownership was a direct asset-theft path: name the current owner, receive the asset. multisig-transfer: configure_approval_rule, create_transfer_request, approve_transfer_request, reject_transfer_request, cancel_transfer_request. Approvals were forgeable by naming an authorized approver, and the approval threshold rewritable by naming the admin. Also closed the initialize front-running window in assetsup, contrib, multisig-transfer and asset-maintenance, and gated four asset-maintenance entrypoints that had no authorization at all — anyone could write warranty terms, file claims, and raise alerts, forging the audit evidence that contract exists to provide. execute_transfer stays permissionless, matching multisig-wallet's execute_*: the approvers already made the decision. Its unused `caller` argument now says so in a comment, since an authentic-looking unused parameter is what made this class of bug easy to miss. 23 negative tests run without mock_all_auths, including one proving an attacker's own valid signature does not authorize a transfer they named someone else for. With auths mocked, an entrypoint that never authenticates is indistinguishable from one that does, which is why none of this was caught before. The full table is in contracts/AUTHORIZATION.md, including nine assetsup tokenization entrypoints still unguarded — those need a decision about which principal applies, so they are listed rather than guessed at. SC-43: arithmetic - Add assetsup/src/math.rs with checked add/sub/mul and a mul_div that survives an intermediate product overflowing i128 by cancelling common factors first. - Apply it to the dividend split, token mint/burn/transfer, and every ownership percentage. (balance * total_amount) / total_supply was the worst case: it trapped whenever the product exceeded i128::MAX even though the quotient was small. - Overflow now returns MathOverflow/MathUnderflow instead of trapping. - Rounding is documented: mul_div rounds down and the remainder stays with the contract, so a distribution can never overpay and no holder is advantaged by iteration order. - Boundary tests at 0, 1 and type max. SC-44: storage TTL - Add assetsup/src/ttl.rs holding the policy and constants in one place: extend persistent entries when under 30 days, out to 90. - Extend on read as well as write. An asset that is only ever queried would otherwise be archived despite being in active use. - Extend on write in initialize: a freshly written entry gets the network minimum, and a read cannot rescue an entry that is already gone. - Bump the contract instance TTL. Nothing did, so the instance itself could be archived and take the whole contract with it regardless of what else survived. The ledger-advancing tests caught this. - Tests advance 60 days and prove assets, transfers and configuration are still readable. SC-41: asset-maintenance coverage - 4 tests to 70, covering every entrypoint with happy and failure paths. - Validation boundaries: service date exactly now versus one second future, zero cost, condition and quality ratings at 1 and 10 and just outside. - Scheduling boundaries including a schedule due in the past reading as overdue and one due exactly now not yet overdue. - Three tests for the core audit property: history is append-only, an earlier record is never rewritten by a later one, and no operation removes a record. - Negative auth tests for each newly gated entrypoint. multisig_transfer had no tests at all; it now has 8. Closes DistinctCodes#1205 Closes DistinctCodes#1206 Closes DistinctCodes#1207 Closes DistinctCodes#1208
|
@tolulopedd26 is attempting to deploy a commit to the naijabuz's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@tolulopedd26 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
yusuftomilola
approved these changes
Jul 27, 2026
yusuftomilola
left a comment
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
Important security hardening! Missing auth checks fixed, checked arithmetic prevents overflows, and TTL policy ensures data lifecycle management. Strong security focus.
10 tasks
portableDD
added a commit
to portableDD/AssetsUp
that referenced
this pull request
Jul 27, 2026
…ents Resolves the conflicts with main. The crate rename made this the largest of them: git detected the directory rename and relocated main's new multisig_transfer files (README.md, auth_tests.rs) into multisig-transfer/, which is the right answer and is accepted as-is. assetsup/src/lib.rs: - initialize gained the typed contract_initialized event here and the TTL extend-on-write from SC-44 on main; kept both. - The two-step admin transfer from SC-48 landed on main emitting three events through the deprecated untyped API this branch removes. SDK 23 deprecates env.events().publish(), and CI runs clippy with -D warnings, so leaving them would fail the build. propose_admin, accept_admin and cancel_admin_proposal now emit typed events, with two new #[contractevent] types added for the steps that had no equivalent: AdminProposed and AdminProposalCancelled. Each step is separately observable, which the old single-step admin_changed could not express. asset-maintenance/src/lib.rs: kept main's crate docs with this branch's import line, which no longer needs symbol_short. add_warranty_information gained the require_admin gate from SC-42 on main and the factored store_warranty helper plus typed event here; kept both. assetsup/src/tests/mod.rs: kept both module lists. Two things the merge surfaced that needed real fixes: main does not currently compile its tests. The auth audit (DistinctCodes#1228) added a test calling update_admin, and the two-step admin transfer (DistinctCodes#1229) removed it. The two merged cleanly line by line and broke each other semantically, which no conflict marker catches. Rewritten against the two-step API and split so both halves are covered. This branch's own event test used the same removed entrypoint; it now asserts a distinct event at each step of the transfer, plus cancellation. Documentation the rename made stale is corrected: the crate and workspace READMEs no longer describe the directory/package mismatch as an open problem, and the SECURITY.md line claiming multisig_transfer has no tests is corrected — it has 10 since DistinctCodes#1228, though the request lifecycle itself is still uncovered. All 393 tests pass, clippy and fmt are clean, all five crates resolve by directory name, and all five build to WASM. The 16 remaining untyped publishes are all in contrib's uncompiled files, which is why the build has no deprecation warnings.
amberly-d
added a commit
to amberly-d/AssetsUp
that referenced
this pull request
Jul 27, 2026
Resolves the conflicts with main. contracts/.gitignore: both sides wanted. Kept the deployments/ ignore from SC-55 and the lcov.info ignore from SC-39. CI: the security and coverage jobs this branch adds were still installing a floating `stable` toolchain. Stable has since moved to 1.97.1, which fails to compile ethnum 1.5.2 in the soroban dependency tree, so both jobs would have failed exactly as every unpinned contracts job on main currently does. They now read the channel from rust-toolchain.toml like the jobs SC-53 already pinned. assetsup/src/tests/auth.rs: main does not currently compile its tests. The auth audit (DistinctCodes#1228) added a test calling update_admin, and the two-step admin transfer (DistinctCodes#1229) replaced update_admin with propose_admin/accept_admin. The two merged cleanly line by line and broke each other semantically, which no conflict marker catches. The test is rewritten against the two-step API and split in two, so both halves of the flow are covered: nominating requires the current admin, and accepting requires the incoming address. It also asserts the rejected proposal is not recorded and the admin is unchanged, which the original did not. All 435 tests pass, clippy is clean, fmt is clean.
TomikeDS
added a commit
to TomikeDS/AssetsUp
that referenced
this pull request
Jul 27, 2026
…test Resolves the conflicts with main. assetsup/src/lib.rs, three hunks, all additive on both sides: - DataKey gained StorageVersion here and PendingAdmin on main; kept both. - initialize gained the version stamp here and the TTL extend-on-write from SC-44 on main; kept both, and added StorageVersion to the extended keys so the new entry gets the same lifetime as the rest of the configuration. - The upgrade/migrate/storage_version entrypoints landed in the same place as main's accept_admin/cancel_admin_proposal/get_pending_admin; kept both, with the admin flow first so it reads continuously from propose_admin. assetsup/src/tests/mod.rs: kept both module lists. multisig_transfer/src/lib.rs: main added caller.require_auth() to create_transfer_request while this branch renamed _admin to admin to use it in the ownership check. Both are needed and both are kept. Two things the merge surfaced that needed real fixes rather than resolution: The pause guard-rail test from SC-47 failed, correctly, flagging upgrade, migrate and storage_version as mutating entrypoints with no pause check. That test is doing exactly its job. upgrade and migrate are deliberately exempt — an upgrade is often how you fix the incident that caused the pause, as UPGRADE.md already documented — so they are recorded in PAUSE_EXEMPT with that reason. storage_version is a read and is classified as one. main does not currently compile its tests: the auth audit (DistinctCodes#1228) added a test calling update_admin, and the two-step admin transfer (DistinctCodes#1229) removed it. The two merged cleanly line by line and broke each other semantically. The test is rewritten against the two-step API and split so both halves are covered. CI: the wasm-size-delta job this branch adds was still installing a floating `stable` toolchain, which is now 1.97.1 and fails to compile ethnum in the soroban tree. It now reads the pinned channel like every other contracts job. All 404 tests pass, clippy and fmt are clean, and every contract is within its WASM budget.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1205
Closes #1206
Closes #1207
Closes #1208