From a3bfa677f7786775b55a6df7ae61ed87fc8dca17 Mon Sep 17 00:00:00 2001 From: temiport25 <261153140+temiport25@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:46:41 +0100 Subject: [PATCH] Allocate error code ranges, extend pause coverage, two-step admin transfer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SC-45: error code allocation - Each contract now owns a numeric range, with shared errors at fixed codes: 1-99 shared, 100-199 assetsup, 200-299 contrib, 300-399 multisig-wallet, 400-499 multisig-transfer, 500-599 asset-maintenance (reserved). - Previously every crate numbered from 1 independently, so code 1 meant AlreadyInitialized in three contracts and NotInitialized in the fourth — the exact opposite condition. Code 5 had four different meanings. A backend could not interpret a bare code without also knowing which contract answered. - Because ranges do not overlap, a code now identifies its origin contract on its own. - Every variant has a doc comment saying when it is returned. - Tests assert the shared codes match across crates and that no contract's variants escape its block. - 48 expected-code assertions in existing tests updated to match. - contrib is left unrenumbered: its error.rs has no mod declaration and is not compiled, so there is nothing to renumber. Its range is reserved. SC-47: pause coverage - The pause covered 4 of ~35 mutating entrypoints. Tokenization, dividends, voting, transfer restrictions, leasing, insurance and detokenization all kept running while the contract reported itself paused, which defeats the control entirely during an incident. - Added the guard to 27 entrypoints via a require_not_paused helper. - Exemptions are deliberate and documented: the pause controls themselves, the admin transfer flow (an incident may be caused by a compromised admin key, so rotating it must not require unpausing first), and claim_dividends (a user exit path; freezing it would trap funds users have already earned). - The key test parses lib.rs and fails if any mutating entrypoint lacks the guard, so a new entrypoint cannot silently escape the pause. Verified it fails when an exemption is removed rather than passing vacuously. - Reads still work while paused; a pause that blocks reads is an outage. SC-48: two-step admin transfer - Replaces update_admin with propose_admin, accept_admin and cancel_admin_proposal, plus a get_pending_admin read. - Acceptance is authorized by the incoming address, so an address that never accepts leaves the original admin in place. Previously one typo permanently bricked administration of a contract governing real asset ownership, with no on-chain undo. - Events on propose, accept and cancel. - 18 tests cover the state machine: proposal does not move the role, a superseded nominee cannot take it, a cancelled proposal cannot be accepted, acceptance is not repeatable, and the old admin loses rights immediately. SC-46: split assessment - contracts/SPLIT-PLAN.md, written rather than implemented, since the issue asks for a plan agreed before any code moves. - Recommends NOT splitting yet. The issue's "8,894 lines across 33 files" counts tests; the contract source is 3,504 lines across 13 files. assetsup builds to 103 KB after the SC-37 profile work, well inside Soroban's limits, so splitting would trade a real cost against a size problem that is not currently binding — and it would add trust boundaries right after an audit found ten entrypoints not checking authorization within a single contract. - The real problem is the assetsup/contrib duplication, and it is smaller than it looks: roughly 1,670 lines of contrib are not compiled at all, so the deployed contract has no escrow, KYC, staking or oracle despite the source files being present. For every concern that does ship in both, assetsup's implementation is the superset. - If a split does go ahead the realistic target is three contracts, not eight: a registry core plus insurance and leasing. The plan records the sequencing, the shared-types crate, and the interface, with open questions for maintainers. Closes #1209 Closes #1210 Closes #1211 Closes #1212 --- contracts/ERRORS.md | 133 +++++++++ contracts/SPLIT-PLAN.md | 198 +++++++++++++ contracts/assetsup/src/error.rs | 270 +++++++++++++---- contracts/assetsup/src/lib.rs | 150 +++++++++- contracts/assetsup/src/tests/admin.rs | 15 +- .../assetsup/src/tests/admin_transfer.rs | 277 ++++++++++++++++++ contracts/assetsup/src/tests/asset.rs | 20 +- .../assetsup/src/tests/detokenization.rs | 8 +- contracts/assetsup/src/tests/dividends.rs | 6 +- .../assetsup/src/tests/initialization.rs | 4 +- contracts/assetsup/src/tests/insurance.rs | 14 +- .../assetsup/src/tests/integration_full.rs | 5 +- contracts/assetsup/src/tests/mod.rs | 2 + contracts/assetsup/src/tests/pause.rs | 275 +++++++++++++++++ contracts/assetsup/src/tests/tokenization.rs | 18 +- contracts/assetsup/src/tests/voting.rs | 4 +- contracts/contrib/src/tests/detokenization.rs | 4 +- contracts/contrib/src/tests/dividends.rs | 2 +- contracts/contrib/src/tests/tokenization.rs | 6 +- contracts/contrib/src/tests/voting.rs | 4 +- contracts/multisig-wallet/src/errors.rs | 120 +++++++- contracts/multisig_transfer/src/errors.rs | 117 ++++++-- 22 files changed, 1502 insertions(+), 150 deletions(-) create mode 100644 contracts/ERRORS.md create mode 100644 contracts/SPLIT-PLAN.md create mode 100644 contracts/assetsup/src/tests/admin_transfer.rs create mode 100644 contracts/assetsup/src/tests/pause.rs diff --git a/contracts/ERRORS.md b/contracts/ERRORS.md new file mode 100644 index 000000000..63e524f67 --- /dev/null +++ b/contracts/ERRORS.md @@ -0,0 +1,133 @@ +# Error code allocation + +Contract errors cross the network as bare `u32` codes. When two contracts use +the same integer for different meanings, a backend receiving code `3` cannot +say what happened without also knowing which contract produced it — and a +cross-contract call cannot interpret a failure from its callee at all. + +This document is the allocation table ([SC-45]). + +## The collisions this resolves + +Before this change every crate numbered from 1 independently: + +| Code | `assetsup` | `contrib` | `multisig-wallet` | `multisig-transfer` | +|---:|---|---|---|---| +| 1 | `AlreadyInitialized` | `AlreadyInitialized` | `AlreadyInitialized` | **`NotInitialized`** | +| 2 | `AdminNotFound` | `AdminNotFound` | **`NotInitialized`** | **`Unauthorized`** | +| 3 | `AssetAlreadyExists` | `AssetAlreadyExists` | **`Unauthorized`** | **`InvalidOwner`** | +| 4 | `AssetNotFound` | `AssetNotFound` | **`InvalidThreshold`** | **`InvalidNewOwner`** | +| 5 | `BranchAlreadyExists` | **`Unauthorized`** | **`InsufficientOwners`** | **`AssetNotFound`** | +| 8 | `Unauthorized` | `InvalidTokenSupply` | `TransactionExpired` | `RuleNotFound` | + +Code `1` alone means both "already initialized" and its exact opposite +depending on which contract answered. Code `5` has four different meanings. + +## Allocation + +Each contract owns a numeric range. Shared errors are defined once, at fixed +codes, and every contract uses those rather than declaring its own. + +| Range | Owner | +|---:|---| +| 1–99 | **Shared** — cross-cutting errors every contract may return | +| 100–199 | `assetsup` | +| 200–299 | `contrib` | +| 300–399 | `multisig-wallet` | +| 400–499 | `multisig-transfer` | +| 500–599 | `asset-maintenance` (reserved; see below) | +| 600+ | Unallocated. Claim the next free block here before using it. | + +A code, once published, is permanent. Retiring a variant means leaving its +number unused, never reassigning it — a backend built against the old meaning +would silently misinterpret the new one. + +## Shared errors (1–99) + +Defined once in `assetsup::error::shared`, and mirrored at the same numbers by +every other crate. These are the errors whose meaning is identical everywhere, +so a caller can handle them without knowing which contract replied. + +| Code | Variant | Returned when | +|---:|---|---| +| 1 | `AlreadyInitialized` | An initialize entrypoint is called on a contract that already holds state. | +| 2 | `NotInitialized` | Any entrypoint is called before initialization. | +| 3 | `Unauthorized` | The caller authenticated but is not permitted to perform this action. | +| 4 | `InvalidInput` | An argument failed validation and no more specific code applies. | +| 5 | `NotFound` | A referenced entity does not exist and no more specific code applies. | +| 6 | `ContractPaused` | A mutating entrypoint was called while the emergency pause is active. | +| 7 | `MathOverflow` | An arithmetic operation would exceed the type's range. | +| 8 | `MathUnderflow` | A subtraction would go below the type's minimum. | + +Note the distinction between codes 1 and 2, which is where the old numbering +was at its worst: `AlreadyInitialized` and `NotInitialized` previously shared +code 1 across different contracts. + +## `assetsup` (100–199) + +| Block | Concern | +|---:|---| +| 100–119 | Registry: assets, branches, registrars | +| 120–139 | Tokenization and balances | +| 140–149 | Voting | +| 150–159 | Dividends | +| 160–169 | Detokenization and valuation | +| 170–179 | Validation | +| 180–199 | Leasing and insurance | + +## `contrib` (200–299) + +`contrib` has **no typed errors in compiled code**. Its `src/error.rs` defines +an enum, but the file has no `mod` declaration, so nothing references it and +every failure surfaces as a `panic!` on a string. The range is reserved for +when that module is wired in or removed as part of [SC-46]. + +## `multisig-wallet` (300–399) + +| Block | Concern | +|---:|---| +| 300–319 | Transaction lifecycle | +| 320–339 | Owner and threshold governance | +| 340–349 | Emergency controls and limits | + +## `multisig-transfer` (400–499) + +| Block | Concern | +|---:|---| +| 400–419 | Request lifecycle | +| 420–439 | Approval rules and approvers | +| 440–449 | Registry interaction | + +## `asset-maintenance` (500–599) + +Reserved but unused. This crate has **no error enum at all** — it raises +failures with `panic!` on a `&str`, so callers cannot distinguish a missing +warranty from an inactive provider by code. Converting it to a `contracterror` +in the 500 range is follow-up work; the range is claimed here so it does not +get taken in the meantime. + +## For backend implementers + +``` +code < 100 → shared meaning, safe to handle generically +100 <= code < 200 → assetsup-specific +200 <= code < 300 → contrib-specific +300 <= code < 400 → multisig-wallet-specific +400 <= code < 500 → multisig-transfer-specific +500 <= code < 600 → asset-maintenance-specific +``` + +Because the ranges do not overlap, a code identifies its origin contract on its +own. That is the property the old numbering lacked and the reason for the +renumbering. + +## Adding an error + +1. Decide whether it is genuinely shared. If two contracts would return it with + the same meaning, it belongs in 1–99 and must be added to every crate's enum + at the same number. +2. Otherwise take the next free code **within your contract's block**, not the + next free code overall. +3. Give it a doc comment saying when it is returned. Every variant has one; a + code with no stated meaning is not usable by a caller. +4. Never reuse a retired code. diff --git a/contracts/SPLIT-PLAN.md b/contracts/SPLIT-PLAN.md new file mode 100644 index 000000000..394a37d87 --- /dev/null +++ b/contracts/SPLIT-PLAN.md @@ -0,0 +1,198 @@ +# Splitting `assetsup`: assessment and migration plan + +Assessment for [SC-46]. **No code is moved by this change.** The issue asks for +a plan agreed before implementation, and that is what this is. + +## The numbers, corrected + +The issue describes `assetsup` as "8,894 lines across 33 files". That figure +counts tests. The split-relevant number is smaller: + +| | Files | Lines | +|---|---:|---:| +| `assetsup` contract source | 13 | 3,504 | +| `assetsup` tests | 22 | 6,238 | +| **Total** | **35** | **9,742** | + +So the contract itself is ~3,500 lines, not ~8,900. Two thirds of the crate is +test code, which is a good sign rather than a problem, and it materially +changes the case for splitting: this is a large module, not an unmanageable +one. + +Contract source by module: + +| Module | Lines | Concern | +|---|---:|---| +| `lib.rs` | 1,063 | Entrypoint facade for everything below | +| `insurance.rs` | 518 | Policies and the claim state machine | +| `tokenization.rs` | 475 | Fractional shares, balances, locks | +| `error.rs` | 256 | Error enum | +| `types.rs` | 227 | Shared types | +| `lease.rs` | 226 | Lease lifecycle | +| `detokenization.rs` | 198 | Detokenization proposals | +| `transfer_restrictions.rs` | 159 | Whitelists | +| `voting.rs` | 153 | Weighted voting | +| `dividends.rs` | 118 | Distribution and claims | +| `audit.rs` | 53 | Audit log | +| `asset.rs` | 39 | Asset struct and registry keys | +| `branch.rs` | 19 | Branch records | + +## Recommendation + +**Do not split `assetsup` into multiple contracts yet.** Resolve the +`assetsup`/`contrib` duplication first, then reassess against a measured WASM +size problem rather than an assumed one. + +The reasoning: + +1. **There is no size emergency.** `assetsup` builds to 103 KB with the + hardened release profile from [SC-37], down from 163 KB. That is + comfortably inside Soroban's limits. Splitting to solve a size problem that + is not currently binding trades a real cost for a speculative benefit. + +2. **Splitting multiplies the attack surface it is meant to reduce.** Today the + asset registry and the modules acting on it share one storage space and one + atomic transaction boundary. Separate contracts mean cross-contract calls, + which means each callee must independently verify the caller — and the + authorization audit in [SC-42] found ten entrypoints that were not checking + authorization *within a single contract*. Adding trust boundaries before + that class of bug is under control makes things worse, not better. + +3. **The real coupling problem is `assetsup` versus `contrib`, not + `assetsup`'s internals.** Two contracts implement `audit`, `insurance` and + `lease` differently, and nobody can say which is authoritative. That is a + correctness and ownership problem today. Module boundaries inside + `assetsup` are a code-organization problem, which is much less urgent. + +4. **Nothing can be upgraded piecemeal regardless.** The issue cites piecemeal + upgrade as a motivation, but no contract in the workspace exposes an upgrade + entrypoint at all ([SC-49]). Splitting does not deliver that benefit until + upgradeability exists. + +## What to do first: resolve the duplication + +`assetsup` and `contrib` are independent contracts with separate storage that +share several module names. The overlap is duplicated code, not a shared +library. + +An important finding while mapping the boundaries: **most of `contrib` is not +compiled.** `contrib/src/lib.rs` declares only `audit`, `pause`, `types`, +`insurance` and `lease`. The escrow, KYC, staking, oracle, tokenization, +detokenization, transfer-restriction and `error.rs` files have no `mod` +declaration and are not part of the crate — about 1,670 lines that never ship. +The deployed `ContribContract` therefore has no escrow, no KYC, no staking, no +oracle, and no typed errors, despite the source files sitting right there. + +That reframes the whole question. The overlap is much smaller than it appears: + +| Concern | `assetsup` | `contrib` (compiled) | Proposed owner | +|---|---|---|---| +| Asset registry | ✅ richer | ✅ separate copy | **`assetsup`** — it is the authoritative registry | +| Audit log | ✅ | ✅ | **`assetsup`** | +| Insurance | ✅ full claim state machine | smaller policy/claim store | **`assetsup`** — `contrib`'s is a strict subset | +| Leasing | ✅ richer lifecycle | check-in/cancel only | **`assetsup`** — same | +| Emergency pause | ✅ (now full coverage, [SC-47]) | ✅ dedicated module | **`assetsup`** | +| Tokenization, dividends, voting, detokenization | ✅ | ❌ dead files | **`assetsup`** | +| Escrow, KYC, staking, oracle | — | ❌ dead files | **Undecided** — see below | + +For every concern that actually ships in both, `assetsup`'s implementation is +the superset. There is no case where `contrib`'s compiled version is the better +one to keep. + +### Proposed resolution + +1. **Decide what `contrib` is for.** Two honest options: + - *Delete it.* Everything it compiles today, `assetsup` already does better. + - *Make it the second contract it was evidently meant to be* by wiring in + escrow, KYC, staking and the oracle — the capabilities `assetsup` genuinely + lacks. This needs a security review of code that has never compiled; + `tokenization.rs` does not even parse as valid contract code. + + This is a maintainer decision, not one to make inside a refactor. + +2. **Whichever is chosen, delete the dead files.** Leaving 1,670 lines of + uncompiled source that looks like working code is actively misleading — + every reader so far, including the issue that prompted this assessment, has + assumed `contrib` owns escrow and KYC. + +3. **Only then** reassess splitting `assetsup`. + +## If the split does go ahead + +Recorded so the decision does not have to be reconstructed later. + +### What is a separate contract versus a shared internal + +Genuinely separate — distinct lifecycles, distinct principals, meaningful +alone: + +| Candidate | Lines | Why it stands alone | +|---|---:|---| +| **Insurance** | 518 | Insurers and claimants are not asset owners. The claim state machine is self-contained. The largest single module and the weakest coupling to the registry. | +| **Leasing** | 226 | Lessor/lessee lifecycle is independent of ownership; a lease does not change the owner. | + +Not separable — they are the registry, or meaningless without its state: + +| Module | Why it stays | +|---|---| +| `asset`, `branch`, `audit` | These *are* the registry. | +| `tokenization` | Share balances are the registry's ownership model in fractional form. | +| `dividends`, `voting`, `detokenization` | All read token balances on every call. Splitting turns each into a cross-contract read. | +| `transfer_restrictions` | Consulted inside every transfer; a hot path. | + +So the realistic target is **three contracts, not eight**: a registry core, +insurance, and leasing. + +### Shared types + +Extract into a `assetsup-types` library crate (`crate-type = ["lib"]`, never +deployed) rather than duplicating: `AssetStatus`, `AssetType`, `ActionType`, +`CustomAttribute`, and the shared error range 1–99 from +[`ERRORS.md`](ERRORS.md). The error allocation already anticipates this — each +contract owns a numeric block, so a split does not create code collisions. + +### Interfaces + +Insurance and leasing both need "does this asset exist and who owns it". That +is one read entrypoint on the registry: + +```rust +fn get_asset_info(env: Env, asset_id: BytesN<32>) -> Result; +``` + +Neither needs to *write* to the registry, which keeps the trust relationship +one-directional and avoids the confused-deputy risk that a write interface +would introduce. + +### Migration sequence + +Each step keeps `cargo test --all` green: + +1. Extract `assetsup-types`; both crates depend on it. No behaviour change. +2. Create `assetsup-insurance` with the registry address stored at init. Move + `insurance.rs` **and its tests** together. +3. Add the cross-contract read and integration tests covering the seam + (registry → insurance), building on [SC-51]. +4. Remove insurance entrypoints from `assetsup`. **This is the breaking step** — + it changes the deployed ABI, so it needs the upgrade story from [SC-49] to + exist first, or a coordinated redeploy. +5. Repeat for leasing. +6. Record WASM size before and after each step. If the registry core does not + shrink meaningfully, stop — the split is not paying for itself. + +### Expected size effect + +Insurance and leasing are 744 lines of 3,504, roughly 21% of the source. A +proportional saving would take the registry core from 103 KB to about 81 KB, +before accounting for the cross-contract call machinery each new contract adds +back. The saving is real but modest, which is the main argument for waiting +until there is a size problem worth solving. + +## Open questions for maintainers + +1. Delete `contrib`, or wire in its dead modules? Everything else waits on this. +2. Is 103 KB actually near a limit you are worried about, or is the size + concern precautionary? +3. Does the backend bridge need to treat insurance and leasing as separate + deployments, or is one contract address simpler for it? +4. Is there an upgrade story planned ([SC-49])? Step 4 above is blocked on it. diff --git a/contracts/assetsup/src/error.rs b/contracts/assetsup/src/error.rs index 2d8008a88..2c2cb5221 100644 --- a/contracts/assetsup/src/error.rs +++ b/contracts/assetsup/src/error.rs @@ -1,65 +1,155 @@ use soroban_sdk::{contracterror, panic_with_error, Env}; +/// Contract errors for `assetsup`. +/// +/// Codes follow the workspace allocation in `contracts/ERRORS.md`: +/// +/// - **1–99** are *shared* across every contract in the workspace and mean the +/// same thing everywhere. +/// - **100–199** belong to `assetsup` alone. +/// +/// Because the ranges do not overlap, a code identifies its origin contract on +/// its own. Previously every crate numbered from 1 independently, so code `1` +/// meant `AlreadyInitialized` here but `NotInitialized` in `multisig-transfer` +/// — the exact opposite condition. +/// +/// A published code is permanent. Retiring a variant leaves its number unused; +/// it is never reassigned, because a backend built against the old meaning +/// would silently misinterpret the new one. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] pub enum Error { + // --------------------------------------------------------------- + // Shared: 1–99. Mirrored at the same codes by every other crate. + // --------------------------------------------------------------- + /// An initialize entrypoint was called on a contract that already holds + /// state. AlreadyInitialized = 1, - AdminNotFound = 2, - AssetAlreadyExists = 3, - AssetNotFound = 4, - BranchAlreadyExists = 5, - BranchNotFound = 6, - SubscriptionAlreadyExists = 7, - Unauthorized = 8, - InvalidPayment = 9, - // Tokenization errors - AssetAlreadyTokenized = 10, - AssetNotTokenized = 11, - InvalidTokenSupply = 12, - InvalidTokenDecimals = 13, - InsufficientBalance = 14, - InsufficientLockedTokens = 15, - TokensAreLocked = 16, - TransferRestrictionFailed = 17, - NotWhitelisted = 18, - AccreditedInvestorRequired = 19, - GeographicRestriction = 20, - // Voting errors - InsufficientVotingPower = 21, - AlreadyVoted = 22, - ProposalNotFound = 23, - InvalidProposal = 24, - VotingPeriodEnded = 25, - // Dividend errors - NoDividendsToClaim = 26, - InvalidDividendAmount = 27, - // Detokenization errors - DetokenizationNotApproved = 28, - DetokenizationAlreadyProposed = 29, - // Valuation errors - InvalidValuation = 30, - // Holder enumeration errors - HolderNotFound = 31, - // Math errors - MathOverflow = 32, - MathUnderflow = 33, - // Contract state errors - ContractPaused = 34, - ContractNotInitialized = 35, - // Validation errors - InvalidAssetName = 36, - InvalidPurchaseValue = 37, - InvalidMetadataUri = 38, - InvalidOwnerAddress = 39, - - LeaseNotFound = 40, - LeaseAlreadyExists = 41, - AssetAlreadyLeased = 42, - InvalidLeaseStatus = 43, - LeaseAlreadyStarted = 44, - LeaseNotExpired = 45, - InvalidTimestamps = 46, + /// An entrypoint was called before the contract was initialized. + NotInitialized = 2, + /// The caller authenticated but is not permitted to perform this action. + Unauthorized = 3, + /// An argument failed validation and no more specific code applies. + InvalidInput = 4, + /// A referenced entity does not exist and no more specific code applies. + NotFound = 5, + /// A mutating entrypoint was called while the emergency pause is active. + ContractPaused = 6, + /// An arithmetic operation would exceed the type's range. + MathOverflow = 7, + /// A subtraction would go below the type's minimum. + MathUnderflow = 8, + + // --------------------------------------------------------------- + // Registry: 100–119 + // --------------------------------------------------------------- + /// No admin is stored, so the contract is not usable. + AdminNotFound = 100, + /// An asset with this id is already registered. + AssetAlreadyExists = 101, + /// No asset is registered under this id. + AssetNotFound = 102, + /// A branch with this id already exists. + BranchAlreadyExists = 103, + /// No branch is registered under this id. + BranchNotFound = 104, + /// A subscription already exists for this account. + SubscriptionAlreadyExists = 105, + /// The payment supplied is missing or does not cover the amount due. + InvalidPayment = 106, + /// The contract has not been initialized with its metadata. + ContractNotInitialized = 107, + + // --------------------------------------------------------------- + // Tokenization and balances: 120–139 + // --------------------------------------------------------------- + /// This asset has already been tokenized. + AssetAlreadyTokenized = 120, + /// This asset has not been tokenized, so it has no token supply. + AssetNotTokenized = 121, + /// The requested total supply is zero or negative. + InvalidTokenSupply = 122, + /// The requested decimal precision is outside the permitted range. + InvalidTokenDecimals = 123, + /// The holder's balance is lower than the amount requested. + InsufficientBalance = 124, + /// Fewer tokens are locked than the amount being unlocked. + InsufficientLockedTokens = 125, + /// The holder's tokens are locked and cannot be moved yet. + TokensAreLocked = 126, + /// A transfer restriction on this asset rejected the transfer. + TransferRestrictionFailed = 127, + /// The address is not on this asset's transfer whitelist. + NotWhitelisted = 128, + /// This asset may only be held by accredited investors. + AccreditedInvestorRequired = 129, + /// The address's jurisdiction is not permitted to hold this asset. + GeographicRestriction = 130, + /// No ownership record exists for this holder. + HolderNotFound = 131, + + // --------------------------------------------------------------- + // Voting: 140–149 + // --------------------------------------------------------------- + /// The voter's balance is below the minimum voting threshold. + InsufficientVotingPower = 140, + /// This address has already voted on this proposal. + AlreadyVoted = 141, + /// No proposal exists under this id. + ProposalNotFound = 142, + /// The proposal is malformed or in a state that does not allow this action. + InvalidProposal = 143, + /// The voting period for this proposal has closed. + VotingPeriodEnded = 144, + + // --------------------------------------------------------------- + // Dividends: 150–159 + // --------------------------------------------------------------- + /// The holder has no unclaimed dividends. + NoDividendsToClaim = 150, + /// The distribution amount is zero or negative, or revenue sharing is off. + InvalidDividendAmount = 151, + + // --------------------------------------------------------------- + // Detokenization and valuation: 160–169 + // --------------------------------------------------------------- + /// The detokenization proposal has not reached its approval threshold. + DetokenizationNotApproved = 160, + /// A detokenization proposal is already open for this asset. + DetokenizationAlreadyProposed = 161, + /// The supplied valuation is zero or negative. + InvalidValuation = 162, + + // --------------------------------------------------------------- + // Validation: 170–179 + // --------------------------------------------------------------- + /// The asset name is empty or too long. + InvalidAssetName = 170, + /// The purchase value is negative. + InvalidPurchaseValue = 171, + /// The metadata URI is empty or malformed. + InvalidMetadataUri = 172, + /// The owner address is the zero address, or otherwise not usable. + InvalidOwnerAddress = 173, + + // --------------------------------------------------------------- + // Leasing and insurance: 180–199 + // --------------------------------------------------------------- + /// No lease exists under this id. + LeaseNotFound = 180, + /// A lease already exists under this id. + LeaseAlreadyExists = 181, + /// The asset is already out on an active lease. + AssetAlreadyLeased = 182, + /// The lease is not in a state that permits this action. + InvalidLeaseStatus = 183, + /// The lease has already started and can no longer be modified this way. + LeaseAlreadyStarted = 184, + /// The lease has not yet reached its end date. + LeaseNotExpired = 185, + /// The supplied start and end timestamps are inconsistent. + InvalidTimestamps = 186, } pub fn handle_error(env: &Env, error: Error) -> ! { @@ -93,4 +183,74 @@ mod tests { let result = dummy_function(env.clone(), false); assert_eq!(result, Ok(())); } + + #[test] + fn shared_errors_occupy_the_shared_range() { + // 1-99 must mean the same thing in every contract; these codes are + // mirrored by multisig-wallet and multisig-transfer. + assert_eq!(Error::AlreadyInitialized as u32, 1); + assert_eq!(Error::NotInitialized as u32, 2); + assert_eq!(Error::Unauthorized as u32, 3); + assert_eq!(Error::InvalidInput as u32, 4); + assert_eq!(Error::NotFound as u32, 5); + assert_eq!(Error::ContractPaused as u32, 6); + assert_eq!(Error::MathOverflow as u32, 7); + assert_eq!(Error::MathUnderflow as u32, 8); + } + + #[test] + fn contract_specific_errors_stay_inside_the_assetsup_block() { + // Every non-shared variant must fall in 100-199 so a bare code + // identifies assetsup as its origin. + let codes = [ + Error::AdminNotFound as u32, + Error::AssetAlreadyExists as u32, + Error::AssetNotFound as u32, + Error::BranchAlreadyExists as u32, + Error::BranchNotFound as u32, + Error::SubscriptionAlreadyExists as u32, + Error::InvalidPayment as u32, + Error::ContractNotInitialized as u32, + Error::AssetAlreadyTokenized as u32, + Error::AssetNotTokenized as u32, + Error::InvalidTokenSupply as u32, + Error::InvalidTokenDecimals as u32, + Error::InsufficientBalance as u32, + Error::InsufficientLockedTokens as u32, + Error::TokensAreLocked as u32, + Error::TransferRestrictionFailed as u32, + Error::NotWhitelisted as u32, + Error::AccreditedInvestorRequired as u32, + Error::GeographicRestriction as u32, + Error::HolderNotFound as u32, + Error::InsufficientVotingPower as u32, + Error::AlreadyVoted as u32, + Error::ProposalNotFound as u32, + Error::InvalidProposal as u32, + Error::VotingPeriodEnded as u32, + Error::NoDividendsToClaim as u32, + Error::InvalidDividendAmount as u32, + Error::DetokenizationNotApproved as u32, + Error::DetokenizationAlreadyProposed as u32, + Error::InvalidValuation as u32, + Error::InvalidAssetName as u32, + Error::InvalidPurchaseValue as u32, + Error::InvalidMetadataUri as u32, + Error::InvalidOwnerAddress as u32, + Error::LeaseNotFound as u32, + Error::LeaseAlreadyExists as u32, + Error::AssetAlreadyLeased as u32, + Error::InvalidLeaseStatus as u32, + Error::LeaseAlreadyStarted as u32, + Error::LeaseNotExpired as u32, + Error::InvalidTimestamps as u32, + ]; + + for code in codes { + assert!( + (100..200).contains(&code), + "assetsup error code is outside its allocated 100-199 block" + ); + } + } } diff --git a/contracts/assetsup/src/lib.rs b/contracts/assetsup/src/lib.rs index 7c220273f..436eb77db 100644 --- a/contracts/assetsup/src/lib.rs +++ b/contracts/assetsup/src/lib.rs @@ -32,6 +32,9 @@ pub enum DataKey { TotalAssetCount, ContractMetadata, AuthorizedRegistrar(Address), + /// Address that has been proposed as the next admin but has not yet + /// accepted. Absent when no transfer is in flight. + PendingAdmin, ScheduledTransfer(BytesN<32>), PendingApproval(BytesN<32>), } @@ -176,6 +179,18 @@ impl AssetUpContract { Ok(()) } + /// Rejects the call if the contract is paused. + /// + /// Every mutating entrypoint calls this except the deliberate exemptions + /// documented in `contracts/PAUSE.md`: the pause controls themselves, the + /// admin transfer flow, and `claim_dividends`. + fn require_not_paused(env: &Env) -> Result<(), Error> { + if Self::is_paused(env.clone())? { + return Err(Error::ContractPaused); + } + Ok(()) + } + fn validate_asset(env: &Env, asset: &asset::Asset) -> Result<(), Error> { // Validate asset name length (3-100 characters) if asset.name.len() < 3 || asset.name.len() > 100 { @@ -441,7 +456,16 @@ impl AssetUpContract { } // Admin functions - pub fn update_admin(env: Env, new_admin: Address) -> Result<(), Error> { + /// Step one of a two-step admin transfer: nominate `new_admin`. + /// + /// This does **not** change the admin. The proposal only takes effect when + /// the proposed address calls [`Self::accept_admin`], which proves it is + /// reachable and controlled. A single-step transfer means one typo + /// permanently bricks administration of a contract governing real asset + /// ownership, with no on-chain undo. + /// + /// Calling this again replaces any proposal already in flight. + pub fn propose_admin(env: Env, new_admin: Address) -> Result<(), Error> { let current_admin = Self::get_admin(env.clone())?; current_admin.require_auth(); @@ -454,27 +478,87 @@ impl AssetUpContract { return Err(Error::InvalidOwnerAddress); } - let old_admin = current_admin.clone(); - env.storage().persistent().set(&DataKey::Admin, &new_admin); + // Transferring to yourself is a no-op that would leave a confusing + // pending proposal behind. + if new_admin == current_admin { + return Err(Error::InvalidOwnerAddress); + } - // Remove old admin from authorized registrars and add new admin + env.storage() + .persistent() + .set(&DataKey::PendingAdmin, &new_admin); + + env.events().publish( + (symbol_short!("adm_prop"),), + (current_admin, new_admin, env.ledger().timestamp()), + ); + + Ok(()) + } + + /// Step two: the proposed admin accepts, and only then does the role move. + /// + /// Authorized by the *incoming* address, which is the whole point — an + /// address that never accepts leaves the original admin in place forever. + pub fn accept_admin(env: Env) -> Result<(), Error> { + let pending: Address = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .ok_or(Error::AdminNotFound)?; + + pending.require_auth(); + + let old_admin = Self::get_admin(env.clone())?; + + env.storage().persistent().set(&DataKey::Admin, &pending); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + // Move registrar rights along with the role. env.storage() .persistent() .set(&DataKey::AuthorizedRegistrar(old_admin.clone()), &false); env.storage() .persistent() - .set(&DataKey::AuthorizedRegistrar(new_admin.clone()), &true); + .set(&DataKey::AuthorizedRegistrar(pending.clone()), &true); - // Emit event env.events().publish( (symbol_short!("admin_chg"),), - (old_admin, new_admin, env.ledger().timestamp()), + (old_admin, pending, env.ledger().timestamp()), + ); + + Ok(()) + } + + /// Withdraws a pending proposal. Only the current admin may cancel. + pub fn cancel_admin_proposal(env: Env) -> Result<(), Error> { + let current_admin = Self::get_admin(env.clone())?; + current_admin.require_auth(); + + let pending: Address = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .ok_or(Error::AdminNotFound)?; + + env.storage().persistent().remove(&DataKey::PendingAdmin); + + env.events().publish( + (symbol_short!("adm_cncl"),), + (current_admin, pending, env.ledger().timestamp()), ); Ok(()) } + /// The address currently nominated to become admin, if any. + pub fn get_pending_admin(env: Env) -> Option
{ + env.storage().persistent().get(&DataKey::PendingAdmin) + } + pub fn add_authorized_registrar(env: Env, registrar: Address) -> Result<(), Error> { + Self::require_not_paused(&env)?; + let admin = Self::get_admin(env.clone())?; admin.require_auth(); @@ -485,6 +569,8 @@ impl AssetUpContract { } pub fn remove_authorized_registrar(env: Env, registrar: Address) -> Result<(), Error> { + Self::require_not_paused(&env)?; + let admin = Self::get_admin(env.clone())?; admin.require_auth(); @@ -553,6 +639,8 @@ impl AssetUpContract { description: String, asset_type: AssetType, ) -> Result { + Self::require_not_paused(&env)?; + tokenizer.require_auth(); let metadata = TokenMetadata { @@ -585,6 +673,8 @@ impl AssetUpContract { amount: i128, minter: Address, ) -> Result { + Self::require_not_paused(&env)?; + minter.require_auth(); tokenization::mint_tokens(&env, asset_id, amount, minter) } @@ -596,6 +686,8 @@ impl AssetUpContract { amount: i128, burner: Address, ) -> Result { + Self::require_not_paused(&env)?; + burner.require_auth(); tokenization::burn_tokens(&env, asset_id, amount, burner) } @@ -608,6 +700,8 @@ impl AssetUpContract { to: Address, amount: i128, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + from.require_auth(); // Validate transfer restrictions @@ -634,12 +728,16 @@ impl AssetUpContract { until_timestamp: u64, caller: Address, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + caller.require_auth(); tokenization::lock_tokens(&env, asset_id, holder, until_timestamp, caller) } /// Unlock tokens pub fn unlock_tokens(env: Env, asset_id: u64, holder: Address) -> Result<(), Error> { + Self::require_not_paused(&env)?; + tokenization::unlock_tokens(&env, asset_id, holder) } @@ -664,6 +762,8 @@ impl AssetUpContract { /// Update asset valuation pub fn update_valuation(env: Env, asset_id: u64, new_valuation: i128) -> Result<(), Error> { + Self::require_not_paused(&env)?; + tokenization::update_valuation(&env, asset_id, new_valuation) } @@ -673,6 +773,8 @@ impl AssetUpContract { /// Distribute dividends proportionally to all holders pub fn distribute_dividends(env: Env, asset_id: u64, total_amount: i128) -> Result<(), Error> { + Self::require_not_paused(&env)?; + dividends::distribute_dividends(&env, asset_id, total_amount) } @@ -693,11 +795,15 @@ impl AssetUpContract { /// Enable revenue sharing for an asset pub fn enable_revenue_sharing(env: Env, asset_id: u64) -> Result<(), Error> { + Self::require_not_paused(&env)?; + dividends::enable_revenue_sharing(&env, asset_id) } /// Disable revenue sharing for an asset pub fn disable_revenue_sharing(env: Env, asset_id: u64) -> Result<(), Error> { + Self::require_not_paused(&env)?; + dividends::disable_revenue_sharing(&env, asset_id) } @@ -712,6 +818,8 @@ impl AssetUpContract { proposal_id: u64, voter: Address, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + voter.require_auth(); voting::cast_vote(&env, asset_id, proposal_id, voter) } @@ -746,6 +854,8 @@ impl AssetUpContract { asset_id: u64, require_accredited: bool, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + transfer_restrictions::set_transfer_restriction( &env, asset_id, @@ -758,11 +868,15 @@ impl AssetUpContract { /// Add address to whitelist pub fn add_to_whitelist(env: Env, asset_id: u64, address: Address) -> Result<(), Error> { + Self::require_not_paused(&env)?; + transfer_restrictions::add_to_whitelist(&env, asset_id, address) } /// Remove address from whitelist pub fn remove_from_whitelist(env: Env, asset_id: u64, address: Address) -> Result<(), Error> { + Self::require_not_paused(&env)?; + transfer_restrictions::remove_from_whitelist(&env, asset_id, address) } @@ -786,12 +900,16 @@ impl AssetUpContract { asset_id: u64, proposer: Address, ) -> Result { + Self::require_not_paused(&env)?; + proposer.require_auth(); detokenization::propose_detokenization(&env, asset_id, proposer) } /// Execute detokenization (if vote passed) pub fn execute_detokenization(env: Env, asset_id: u64, proposal_id: u64) -> Result<(), Error> { + Self::require_not_paused(&env)?; + detokenization::execute_detokenization(&env, asset_id, proposal_id) } @@ -817,6 +935,8 @@ impl AssetUpContract { env: Env, policy: insurance::InsurancePolicy, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + policy.insurer.require_auth(); insurance::create_policy(env, policy) } @@ -827,6 +947,8 @@ impl AssetUpContract { policy_id: BytesN<32>, caller: Address, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + caller.require_auth(); insurance::cancel_policy(env, policy_id, caller) } @@ -837,12 +959,16 @@ impl AssetUpContract { policy_id: BytesN<32>, insurer: Address, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + insurer.require_auth(); insurance::suspend_policy(env, policy_id, insurer) } /// Expire a policy (permissionless) pub fn expire_insurance_policy(env: Env, policy_id: BytesN<32>) -> Result<(), Error> { + Self::require_not_paused(&env)?; + insurance::expire_policy(env, policy_id) } @@ -854,6 +980,8 @@ impl AssetUpContract { new_premium: i128, insurer: Address, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + insurer.require_auth(); insurance::renew_policy(env, policy_id, new_end_date, new_premium, insurer) } @@ -883,6 +1011,8 @@ impl AssetUpContract { rent: i128, deposit: i128, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + lessor.require_auth(); lease::create_lease( &env, asset_id, lease_id, lessor, lessee, start, end, rent, deposit, @@ -895,18 +1025,24 @@ impl AssetUpContract { lease_id: BytesN<32>, caller: Address, ) -> Result<(), Error> { + Self::require_not_paused(&env)?; + caller.require_auth(); lease::return_leased_asset(&env, lease_id, caller) } /// Cancel a lease before it starts. Lessor only. pub fn cancel_lease(env: Env, lease_id: BytesN<32>, caller: Address) -> Result<(), Error> { + Self::require_not_paused(&env)?; + caller.require_auth(); lease::cancel_lease(&env, lease_id, caller) } /// Expire a lease permissionlessly once end_timestamp has passed. pub fn expire_lease(env: Env, lease_id: BytesN<32>) -> Result<(), Error> { + Self::require_not_paused(&env)?; + lease::expire_lease(&env, lease_id) } diff --git a/contracts/assetsup/src/tests/admin.rs b/contracts/assetsup/src/tests/admin.rs index e6cbd37e4..9252ead47 100644 --- a/contracts/assetsup/src/tests/admin.rs +++ b/contracts/assetsup/src/tests/admin.rs @@ -2,13 +2,16 @@ use crate::tests::helpers::*; use soroban_sdk::{testutils::Address as _, Address}; #[test] -fn test_update_admin_success() { +fn test_admin_transfer_success() { let env = create_env(); let (admin, new_admin, _, _) = create_mock_addresses(&env); let client = initialize_contract(&env, &admin); env.mock_all_auths(); - client.update_admin(&new_admin); + // Admin transfer is now two-step: propose, then the incoming address + // accepts. + client.propose_admin(&new_admin); + client.accept_admin(); // Verify admin was updated assert_eq!(client.get_admin(), new_admin); @@ -21,8 +24,8 @@ fn test_update_admin_success() { } #[test] -#[should_panic(expected = "Error(Contract, #39)")] -fn test_update_admin_zero_address() { +#[should_panic(expected = "Error(Contract, #173)")] +fn test_propose_admin_zero_address() { let env = create_env(); let admin = Address::generate(&env); let client = initialize_contract(&env, &admin); @@ -35,7 +38,7 @@ fn test_update_admin_zero_address() { env.mock_all_auths(); // Should panic with InvalidOwnerAddress error - client.update_admin(&zero_address); + client.propose_admin(&zero_address); } #[test] @@ -92,7 +95,7 @@ fn test_remove_authorized_registrar() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_remove_admin_from_registrars() { let env = create_env(); let admin = Address::generate(&env); diff --git a/contracts/assetsup/src/tests/admin_transfer.rs b/contracts/assetsup/src/tests/admin_transfer.rs new file mode 100644 index 000000000..372b3a0e9 --- /dev/null +++ b/contracts/assetsup/src/tests/admin_transfer.rs @@ -0,0 +1,277 @@ +//! Two-step admin transfer ([SC-48]). +//! +//! A single-step transfer means one typo permanently bricks administration of +//! a contract governing real asset ownership, with no on-chain undo. The +//! property that matters, and the one most of these tests exist to pin down: +//! **transferring to an address that never accepts leaves the original admin +//! in place.** + +use soroban_sdk::testutils::Address as _; +use soroban_sdk::{Address, Env, String}; + +use super::helpers::{create_env, create_mock_addresses, initialize_contract}; +use crate::AssetUpContractClient; + +fn setup(env: &Env) -> (AssetUpContractClient<'_>, Address, Address) { + let (admin, candidate, _, _) = create_mock_addresses(env); + let client = initialize_contract(env, &admin); + env.mock_all_auths(); + (client, admin, candidate) +} + +// --------------------------------------------------------------------------- +// The core property +// --------------------------------------------------------------------------- + +#[test] +fn proposing_does_not_change_the_admin() { + let env = create_env(); + let (client, admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + + assert_eq!( + client.get_admin(), + admin, + "the admin must not move until the proposal is accepted" + ); + assert_eq!(client.get_pending_admin(), Some(candidate)); +} + +#[test] +fn an_address_that_never_accepts_leaves_the_original_admin_in_place() { + // The typo case. Nominating an unreachable address must be recoverable. + let env = create_env(); + let (client, admin, _candidate) = setup(&env); + let unreachable = Address::generate(&env); + + client.propose_admin(&unreachable); + + // The original admin still holds every privilege. + assert_eq!(client.get_admin(), admin); + client.pause_contract(); + assert!(client.is_paused()); + client.unpause_contract(); + + // And can withdraw the mistake. + client.cancel_admin_proposal(); + assert_eq!(client.get_pending_admin(), None); + assert_eq!(client.get_admin(), admin); +} + +#[test] +fn accepting_moves_the_admin_and_clears_the_proposal() { + let env = create_env(); + let (client, _admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + client.accept_admin(); + + assert_eq!(client.get_admin(), candidate); + assert_eq!( + client.get_pending_admin(), + None, + "the proposal must be consumed" + ); +} + +#[test] +fn accepting_moves_registrar_rights_with_the_role() { + let env = create_env(); + let (client, admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + client.accept_admin(); + + assert!(client.is_authorized_registrar(&candidate)); + assert!(!client.is_authorized_registrar(&admin)); +} + +// --------------------------------------------------------------------------- +// Rejections +// --------------------------------------------------------------------------- + +#[test] +fn a_non_admin_cannot_propose() { + let env = create_env(); + let (client, _admin, candidate) = setup(&env); + let stranger = Address::generate(&env); + + // Only the current admin's authorization is accepted; a stranger signing + // for themselves is not enough. + env.set_auths(&[]); + let res = client.try_propose_admin(&candidate); + assert!(res.is_err(), "proposing must require the current admin"); + + let _ = stranger; + assert_eq!(client.get_pending_admin(), None); +} + +#[test] +fn accepting_requires_the_proposed_addresss_authorization() { + let env = create_env(); + let (client, admin, candidate) = setup(&env); + client.propose_admin(&candidate); + + env.set_auths(&[]); + let res = client.try_accept_admin(); + + assert!( + res.is_err(), + "acceptance must be signed by the incoming address" + ); + assert_eq!(client.get_admin(), admin, "the admin must not have moved"); +} + +#[test] +fn accepting_without_a_proposal_is_rejected() { + let env = create_env(); + let (client, admin, _candidate) = setup(&env); + + let res = client.try_accept_admin(); + assert!(res.is_err(), "there is nothing to accept"); + assert_eq!(client.get_admin(), admin); +} + +#[test] +fn cancelling_without_a_proposal_is_rejected() { + let env = create_env(); + let (client, _admin, _candidate) = setup(&env); + + let res = client.try_cancel_admin_proposal(); + assert!(res.is_err(), "there is nothing to cancel"); +} + +#[test] +fn a_non_admin_cannot_cancel_a_proposal() { + let env = create_env(); + let (client, _admin, candidate) = setup(&env); + client.propose_admin(&candidate); + + env.set_auths(&[]); + let res = client.try_cancel_admin_proposal(); + + assert!(res.is_err(), "cancelling must require the current admin"); + assert_eq!( + client.get_pending_admin(), + Some(candidate), + "the proposal must survive the rejected cancel" + ); +} + +#[test] +fn proposing_the_zero_address_is_rejected() { + let env = create_env(); + let (client, _admin, _candidate) = setup(&env); + + let zero = Address::from_string(&String::from_str( + &env, + "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", + )); + + let res = client.try_propose_admin(&zero); + assert!(res.is_err(), "the zero address must be rejected"); + assert_eq!(client.get_pending_admin(), None); +} + +#[test] +fn proposing_the_current_admin_is_rejected() { + // A self-transfer is a no-op that would leave a confusing pending proposal. + let env = create_env(); + let (client, admin, _candidate) = setup(&env); + + let res = client.try_propose_admin(&admin); + assert!(res.is_err()); + assert_eq!(client.get_pending_admin(), None); +} + +// --------------------------------------------------------------------------- +// State machine +// --------------------------------------------------------------------------- + +#[test] +fn a_cancelled_proposal_cannot_then_be_accepted() { + let env = create_env(); + let (client, admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + client.cancel_admin_proposal(); + + let res = client.try_accept_admin(); + assert!(res.is_err(), "a withdrawn proposal must not be acceptable"); + assert_eq!(client.get_admin(), admin); +} + +#[test] +fn a_second_proposal_replaces_the_first() { + let env = create_env(); + let (client, admin, first) = setup(&env); + let second = Address::generate(&env); + + client.propose_admin(&first); + client.propose_admin(&second); + + assert_eq!(client.get_pending_admin(), Some(second.clone())); + + client.accept_admin(); + assert_eq!( + client.get_admin(), + second, + "the superseded nominee must not be able to take the role" + ); + let _ = (admin, first); +} + +#[test] +fn accepting_twice_is_rejected() { + let env = create_env(); + let (client, _admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + client.accept_admin(); + + let res = client.try_accept_admin(); + assert!(res.is_err(), "the proposal was already consumed"); + assert_eq!(client.get_admin(), candidate); +} + +#[test] +fn the_new_admin_can_immediately_exercise_admin_rights() { + let env = create_env(); + let (client, _admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + client.accept_admin(); + + client.pause_contract(); + assert!(client.is_paused()); + + let next = Address::generate(&env); + client.propose_admin(&next); + assert_eq!(client.get_pending_admin(), Some(next)); +} + +#[test] +fn the_old_admin_loses_admin_rights_after_the_transfer() { + let env = create_env(); + let (client, admin, candidate) = setup(&env); + + client.propose_admin(&candidate); + client.accept_admin(); + + // The old admin can no longer act. Grant only the old admin's signature. + env.set_auths(&[]); + let res = client.try_pause_contract(); + assert!( + res.is_err(), + "the former admin must no longer be able to pause" + ); + let _ = admin; +} + +#[test] +fn get_pending_admin_is_none_before_any_proposal() { + let env = create_env(); + let (client, _admin, _candidate) = setup(&env); + assert_eq!(client.get_pending_admin(), None); +} diff --git a/contracts/assetsup/src/tests/asset.rs b/contracts/assetsup/src/tests/asset.rs index 79872cb1a..1f7c5bb39 100644 --- a/contracts/assetsup/src/tests/asset.rs +++ b/contracts/assetsup/src/tests/asset.rs @@ -29,7 +29,7 @@ fn test_register_asset_success() { } #[test] -#[should_panic(expected = "Error(Contract, #3)")] +#[should_panic(expected = "Error(Contract, #101)")] fn test_register_asset_already_exists() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -46,7 +46,7 @@ fn test_register_asset_already_exists() { } #[test] -#[should_panic(expected = "Error(Contract, #34)")] +#[should_panic(expected = "Error(Contract, #6)")] fn test_register_asset_when_paused() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -65,7 +65,7 @@ fn test_register_asset_when_paused() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_register_asset_unauthorized() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -81,7 +81,7 @@ fn test_register_asset_unauthorized() { } #[test] -#[should_panic(expected = "Error(Contract, #36)")] +#[should_panic(expected = "Error(Contract, #170)")] fn test_register_asset_invalid_name_too_short() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -98,7 +98,7 @@ fn test_register_asset_invalid_name_too_short() { } #[test] -#[should_panic(expected = "Error(Contract, #37)")] +#[should_panic(expected = "Error(Contract, #171)")] fn test_register_asset_invalid_purchase_value() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -115,7 +115,7 @@ fn test_register_asset_invalid_purchase_value() { } #[test] -#[should_panic(expected = "Error(Contract, #39)")] +#[should_panic(expected = "Error(Contract, #173)")] fn test_register_asset_zero_owner() { let env = create_env(); let admin = Address::generate(&env); @@ -160,7 +160,7 @@ fn test_update_asset_metadata_success() { } #[test] -#[should_panic(expected = "Error(Contract, #4)")] +#[should_panic(expected = "Error(Contract, #102)")] fn test_update_asset_metadata_not_found() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -176,7 +176,7 @@ fn test_update_asset_metadata_not_found() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_update_asset_metadata_unauthorized() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -224,7 +224,7 @@ fn test_transfer_asset_ownership_success() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_transfer_asset_ownership_unauthorized() { let env = create_env(); let (admin, user1, user2, user3) = create_mock_addresses(&env); @@ -261,7 +261,7 @@ fn test_retire_asset_success() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_retire_asset_unauthorized() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); diff --git a/contracts/assetsup/src/tests/detokenization.rs b/contracts/assetsup/src/tests/detokenization.rs index 065760379..05db7195a 100644 --- a/contracts/assetsup/src/tests/detokenization.rs +++ b/contracts/assetsup/src/tests/detokenization.rs @@ -32,7 +32,7 @@ fn test_propose_detokenization_success() { } #[test] -#[should_panic(expected = "Error(Contract, #29)")] +#[should_panic(expected = "Error(Contract, #161)")] fn test_propose_detokenization_already_proposed() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -59,7 +59,7 @@ fn test_propose_detokenization_already_proposed() { } #[test] -#[should_panic(expected = "Error(Contract, #11)")] +#[should_panic(expected = "Error(Contract, #121)")] fn test_propose_detokenization_not_tokenized() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -108,7 +108,7 @@ fn test_execute_detokenization_success() { } #[test] -#[should_panic(expected = "Error(Contract, #28)")] +#[should_panic(expected = "Error(Contract, #160)")] fn test_execute_detokenization_not_approved() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -142,7 +142,7 @@ fn test_execute_detokenization_not_approved() { } #[test] -#[should_panic(expected = "Error(Contract, #24)")] +#[should_panic(expected = "Error(Contract, #143)")] fn test_execute_detokenization_no_proposal() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); diff --git a/contracts/assetsup/src/tests/dividends.rs b/contracts/assetsup/src/tests/dividends.rs index dae3f9e73..c21f531a3 100644 --- a/contracts/assetsup/src/tests/dividends.rs +++ b/contracts/assetsup/src/tests/dividends.rs @@ -98,7 +98,7 @@ fn test_distribute_dividends_success() { } #[test] -#[should_panic(expected = "Error(Contract, #27)")] +#[should_panic(expected = "Error(Contract, #151)")] fn test_distribute_dividends_invalid_amount() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -125,7 +125,7 @@ fn test_distribute_dividends_invalid_amount() { } #[test] -#[should_panic(expected = "Error(Contract, #27)")] +#[should_panic(expected = "Error(Contract, #151)")] fn test_distribute_dividends_not_enabled() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -183,7 +183,7 @@ fn test_claim_dividends_success() { } #[test] -#[should_panic(expected = "Error(Contract, #26)")] +#[should_panic(expected = "Error(Contract, #150)")] fn test_claim_dividends_none_to_claim() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); diff --git a/contracts/assetsup/src/tests/initialization.rs b/contracts/assetsup/src/tests/initialization.rs index 44305274f..e57f975f7 100644 --- a/contracts/assetsup/src/tests/initialization.rs +++ b/contracts/assetsup/src/tests/initialization.rs @@ -56,7 +56,7 @@ fn test_get_contract_metadata() { } #[test] -#[should_panic(expected = "Error(Contract, #35)")] +#[should_panic(expected = "Error(Contract, #107)")] fn test_get_contract_metadata_not_initialized() { let env = create_env(); @@ -68,7 +68,7 @@ fn test_get_contract_metadata_not_initialized() { } #[test] -#[should_panic(expected = "Error(Contract, #2)")] +#[should_panic(expected = "Error(Contract, #100)")] fn test_get_admin_not_found() { let env = create_env(); diff --git a/contracts/assetsup/src/tests/insurance.rs b/contracts/assetsup/src/tests/insurance.rs index 677982b7a..e3b594bd2 100644 --- a/contracts/assetsup/src/tests/insurance.rs +++ b/contracts/assetsup/src/tests/insurance.rs @@ -26,7 +26,7 @@ fn test_create_insurance_policy_success() { } #[test] -#[should_panic(expected = "Error(Contract, #3)")] +#[should_panic(expected = "Error(Contract, #101)")] fn test_create_insurance_policy_already_exists() { let env = create_env(); let (admin, user1, insurer, _) = create_mock_addresses(&env); @@ -44,7 +44,7 @@ fn test_create_insurance_policy_already_exists() { } #[test] -#[should_panic(expected = "Error(Contract, #9)")] +#[should_panic(expected = "Error(Contract, #106)")] fn test_create_insurance_policy_invalid_coverage() { let env = create_env(); let (admin, user1, insurer, _) = create_mock_addresses(&env); @@ -65,7 +65,7 @@ fn test_create_insurance_policy_invalid_coverage() { } #[test] -#[should_panic(expected = "Error(Contract, #9)")] +#[should_panic(expected = "Error(Contract, #106)")] fn test_create_insurance_policy_invalid_dates() { let env = create_env(); let (admin, user1, insurer, _) = create_mock_addresses(&env); @@ -128,7 +128,7 @@ fn test_cancel_insurance_policy_by_insurer() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_cancel_insurance_policy_unauthorized() { let env = create_env(); let (admin, user1, insurer, user3) = create_mock_addresses(&env); @@ -167,7 +167,7 @@ fn test_suspend_insurance_policy() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_suspend_insurance_policy_unauthorized() { let env = create_env(); let (admin, user1, insurer, _) = create_mock_addresses(&env); @@ -216,7 +216,7 @@ fn test_expire_insurance_policy() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_expire_insurance_policy_not_yet_expired() { let env = create_env(); let (admin, user1, insurer, _) = create_mock_addresses(&env); @@ -260,7 +260,7 @@ fn test_renew_insurance_policy() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_renew_insurance_policy_unauthorized() { let env = create_env(); let (admin, user1, insurer, _) = create_mock_addresses(&env); diff --git a/contracts/assetsup/src/tests/integration_full.rs b/contracts/assetsup/src/tests/integration_full.rs index a3527da90..d2741cd2b 100644 --- a/contracts/assetsup/src/tests/integration_full.rs +++ b/contracts/assetsup/src/tests/integration_full.rs @@ -243,8 +243,9 @@ fn test_admin_operations_workflow() { client.unpause_contract(); assert!(!client.is_paused()); - // Update admin - client.update_admin(&new_admin); + // Update admin (two-step: propose then accept) + client.propose_admin(&new_admin); + client.accept_admin(); assert_eq!(client.get_admin(), new_admin); // Verify old admin is no longer authorized registrar diff --git a/contracts/assetsup/src/tests/mod.rs b/contracts/assetsup/src/tests/mod.rs index a44a81fdf..135bb8f08 100644 --- a/contracts/assetsup/src/tests/mod.rs +++ b/contracts/assetsup/src/tests/mod.rs @@ -3,9 +3,11 @@ mod helpers; // Core contract tests mod admin; +mod admin_transfer; mod asset; mod audit_trail; mod initialization; +mod pause; // Tokenization and ownership tests mod detokenization; diff --git a/contracts/assetsup/src/tests/pause.rs b/contracts/assetsup/src/tests/pause.rs new file mode 100644 index 000000000..c6e635ecc --- /dev/null +++ b/contracts/assetsup/src/tests/pause.rs @@ -0,0 +1,275 @@ +//! Emergency pause coverage ([SC-47]). +//! +//! A pause is only useful if it covers **every** mutating entrypoint. One +//! unguarded function defeats the entire control during an incident. +//! +//! Before this change the pause covered four registry entrypoints out of +//! roughly thirty-five. Tokenization, dividends, voting, leasing, insurance and +//! detokenization all kept running while the contract was "paused". +//! +//! The important test here is [`every_mutating_entrypoint_is_covered`], which +//! is driven off the source rather than a hand-maintained list, so an +//! entrypoint added later without a guard fails the build rather than quietly +//! escaping the pause. + +extern crate std; + +use soroban_sdk::testutils::Address as _; +use soroban_sdk::{Address, BytesN, Env, String, Vec}; + +use super::helpers::{create_env, create_test_asset, initialize_contract}; +use crate::AssetUpContractClient; + +fn setup_paused(env: &Env) -> (AssetUpContractClient<'_>, Address) { + let admin = Address::generate(env); + let client = initialize_contract(env, &admin); + env.mock_all_auths(); + client.pause_contract(); + assert!(client.is_paused()); + (client, admin) +} + +fn asset_id(env: &Env, seed: u8) -> BytesN<32> { + BytesN::from_array(env, &[seed; 32]) +} + +// --------------------------------------------------------------------------- +// The guard rail: every mutating entrypoint must reference the pause +// --------------------------------------------------------------------------- + +/// Entrypoints deliberately exempt from the pause, each with its reason. +/// +/// Changing this list is a security decision. See `contracts/PAUSE.md`. +const PAUSE_EXEMPT: &[(&str, &str)] = &[ + ("initialize", "nothing exists to pause yet"), + ("pause_contract", "the control itself"), + ( + "unpause_contract", + "the control itself; must work while paused", + ), + ( + "propose_admin", + "incident response may require rotating a compromised admin key", + ), + ("accept_admin", "completes the rotation above"), + ("cancel_admin_proposal", "withdraws the rotation above"), + ( + "claim_dividends", + "user exit path: freezing it would trap funds users have already earned", + ), +]; + +/// Reads never mutate, so they are not in scope for the pause. +fn is_read(name: &str) -> bool { + name.starts_with("get_") + || name.starts_with("is_") + || name.starts_with("check_") + || name.starts_with("has_") + || name.starts_with("batch_get") + || name == "proposal_passed" +} + +#[test] +fn every_mutating_entrypoint_is_covered() { + // Parsed from the source so a newly added entrypoint cannot slip past by + // simply not being added to a list here. + let source = include_str!("../lib.rs"); + + let mut unguarded: std::vec::Vec<&str> = std::vec::Vec::new(); + let parts: std::vec::Vec<&str> = source.split("\n pub fn ").collect(); + + for part in parts.iter().skip(1) { + let name = part.split('(').next().unwrap_or("").trim(); + if name.is_empty() || is_read(name) { + continue; + } + if PAUSE_EXEMPT.iter().any(|(exempt, _)| *exempt == name) { + continue; + } + + // Only look at this function's own body: stop at the next entrypoint. + let body = part; + if !body.contains("require_not_paused") && !body.contains("ContractPaused") { + unguarded.push(name); + } + } + + assert!( + unguarded.is_empty(), + "these mutating entrypoints do not check the pause guard: {unguarded:?}\n\ + Add `Self::require_not_paused(&env)?;`, or add the entrypoint to \ + PAUSE_EXEMPT with a written reason." + ); +} + +// --------------------------------------------------------------------------- +// Behavioural checks across each area the pause previously missed +// --------------------------------------------------------------------------- + +#[test] +fn registry_writes_are_blocked_while_paused() { + let env = create_env(); + let (client, admin) = setup_paused(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 1); + + assert!(client + .try_register_asset(&create_test_asset(&env, &owner, id.clone()), &admin) + .is_err()); + assert!(client + .try_transfer_asset_ownership(&id, &Address::generate(&env), &owner) + .is_err()); + assert!(client.try_retire_asset(&id, &owner).is_err()); +} + +#[test] +fn registrar_changes_are_blocked_while_paused() { + let env = create_env(); + let (client, _admin) = setup_paused(&env); + let candidate = Address::generate(&env); + + assert!(client.try_add_authorized_registrar(&candidate).is_err()); + assert!(client.try_remove_authorized_registrar(&candidate).is_err()); +} + +#[test] +fn tokenization_is_blocked_while_paused() { + // Previously fully operational during a pause. + let env = create_env(); + let (client, _admin) = setup_paused(&env); + let holder = Address::generate(&env); + + assert!(client + .try_tokenize_asset( + &1u64, + &String::from_str(&env, "TKN"), + &1000i128, + &7u32, + &1i128, + &holder, + &String::from_str(&env, "Token"), + &String::from_str(&env, "A token"), + &crate::types::AssetType::Physical, + ) + .is_err()); + assert!(client.try_mint_tokens(&1u64, &10i128, &holder).is_err()); + assert!(client.try_burn_tokens(&1u64, &10i128, &holder).is_err()); + assert!(client + .try_lock_tokens(&1u64, &holder, &100u64, &holder) + .is_err()); + assert!(client.try_unlock_tokens(&1u64, &holder).is_err()); +} + +#[test] +fn dividends_and_voting_are_blocked_while_paused() { + let env = create_env(); + let (client, _admin) = setup_paused(&env); + + assert!(client.try_distribute_dividends(&1u64, &100i128).is_err()); + assert!(client.try_enable_revenue_sharing(&1u64).is_err()); + assert!(client.try_disable_revenue_sharing(&1u64).is_err()); + assert!(client + .try_cast_vote(&1u64, &1u64, &Address::generate(&env)) + .is_err()); +} + +#[test] +fn transfer_restrictions_are_blocked_while_paused() { + let env = create_env(); + let (client, _admin) = setup_paused(&env); + let who = Address::generate(&env); + + assert!(client.try_add_to_whitelist(&1u64, &who).is_err()); + assert!(client.try_remove_from_whitelist(&1u64, &who).is_err()); +} + +#[test] +fn leasing_is_blocked_while_paused() { + let env = create_env(); + let (client, _admin) = setup_paused(&env); + let id = asset_id(&env, 2); + let who = Address::generate(&env); + + assert!(client.try_cancel_lease(&id, &who).is_err()); + assert!(client.try_expire_lease(&id).is_err()); +} + +#[test] +fn detokenization_is_blocked_while_paused() { + let env = create_env(); + let (client, _admin) = setup_paused(&env); + + assert!(client + .try_propose_detokenization(&1u64, &Address::generate(&env)) + .is_err()); + assert!(client.try_execute_detokenization(&1u64, &1u64).is_err()); +} + +// --------------------------------------------------------------------------- +// Reads and exemptions +// --------------------------------------------------------------------------- + +#[test] +fn reads_still_work_while_paused() { + // A pause that also blocks reads is an outage, not a safety control. + let env = create_env(); + let admin = Address::generate(&env); + let client = initialize_contract(&env, &admin); + env.mock_all_auths(); + + let owner = Address::generate(&env); + let id = asset_id(&env, 3); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + client.pause_contract(); + + assert!(client.is_paused()); + assert_eq!(client.get_asset(&id).owner, owner); + assert_eq!(client.get_admin(), admin); + assert_eq!(client.get_total_asset_count(), 1); + assert!(client.check_asset_exists(&id)); + assert_eq!(client.get_assets_by_owner(&owner).len(), 1); + let ids: Vec> = Vec::from_array(&env, [id]); + assert_eq!(client.batch_get_asset_info(&ids).len(), 1); +} + +#[test] +fn the_admin_transfer_flow_still_works_while_paused() { + // Deliberate exemption: an incident may be *caused* by a compromised admin + // key, so rotating it must not require unpausing first. + let env = create_env(); + let (client, _admin) = setup_paused(&env); + let successor = Address::generate(&env); + + client.propose_admin(&successor); + assert_eq!(client.get_pending_admin(), Some(successor.clone())); + + client.accept_admin(); + assert_eq!(client.get_admin(), successor); + assert!(client.is_paused(), "the pause must survive the rotation"); +} + +#[test] +fn unpausing_restores_normal_operation() { + let env = create_env(); + let (client, admin) = setup_paused(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 4); + + assert!(client + .try_register_asset(&create_test_asset(&env, &owner, id.clone()), &admin) + .is_err()); + + client.unpause_contract(); + + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + assert_eq!(client.get_asset(&id).owner, owner); +} + +#[test] +fn pausing_twice_is_harmless() { + let env = create_env(); + let (client, _admin) = setup_paused(&env); + client.pause_contract(); + assert!(client.is_paused()); +} diff --git a/contracts/assetsup/src/tests/tokenization.rs b/contracts/assetsup/src/tests/tokenization.rs index bc1a02459..0174a54d7 100644 --- a/contracts/assetsup/src/tests/tokenization.rs +++ b/contracts/assetsup/src/tests/tokenization.rs @@ -29,7 +29,7 @@ fn test_tokenize_asset_success() { } #[test] -#[should_panic(expected = "Error(Contract, #10)")] +#[should_panic(expected = "Error(Contract, #120)")] fn test_tokenize_asset_already_tokenized() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -64,7 +64,7 @@ fn test_tokenize_asset_already_tokenized() { } #[test] -#[should_panic(expected = "Error(Contract, #12)")] +#[should_panic(expected = "Error(Contract, #122)")] fn test_tokenize_asset_invalid_supply() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -113,7 +113,7 @@ fn test_mint_tokens_success() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_mint_tokens_unauthorized() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -138,7 +138,7 @@ fn test_mint_tokens_unauthorized() { } #[test] -#[should_panic(expected = "Error(Contract, #11)")] +#[should_panic(expected = "Error(Contract, #121)")] fn test_mint_tokens_not_tokenized() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -177,7 +177,7 @@ fn test_burn_tokens_success() { } #[test] -#[should_panic(expected = "Error(Contract, #14)")] +#[should_panic(expected = "Error(Contract, #124)")] fn test_burn_tokens_insufficient_balance() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -232,7 +232,7 @@ fn test_transfer_tokens_success() { } #[test] -#[should_panic(expected = "Error(Contract, #14)")] +#[should_panic(expected = "Error(Contract, #124)")] fn test_transfer_tokens_insufficient_balance() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -257,7 +257,7 @@ fn test_transfer_tokens_insufficient_balance() { } #[test] -#[should_panic(expected = "Error(Contract, #16)")] +#[should_panic(expected = "Error(Contract, #126)")] fn test_transfer_tokens_locked() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -321,7 +321,7 @@ fn test_lock_unlock_tokens() { } #[test] -#[should_panic(expected = "Error(Contract, #8)")] +#[should_panic(expected = "Error(Contract, #3)")] fn test_lock_tokens_unauthorized() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); @@ -438,7 +438,7 @@ fn test_update_valuation() { } #[test] -#[should_panic(expected = "Error(Contract, #30)")] +#[should_panic(expected = "Error(Contract, #162)")] fn test_update_valuation_invalid() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); diff --git a/contracts/assetsup/src/tests/voting.rs b/contracts/assetsup/src/tests/voting.rs index 085687071..1a769732b 100644 --- a/contracts/assetsup/src/tests/voting.rs +++ b/contracts/assetsup/src/tests/voting.rs @@ -35,7 +35,7 @@ fn test_cast_vote_success() { } #[test] -#[should_panic(expected = "Error(Contract, #22)")] +#[should_panic(expected = "Error(Contract, #141)")] fn test_cast_vote_already_voted() { let env = create_env(); let (admin, user1, _, _) = create_mock_addresses(&env); @@ -62,7 +62,7 @@ fn test_cast_vote_already_voted() { } #[test] -#[should_panic(expected = "Error(Contract, #21)")] +#[should_panic(expected = "Error(Contract, #140)")] fn test_cast_vote_insufficient_voting_power() { let env = create_env(); let (admin, user1, user2, _) = create_mock_addresses(&env); diff --git a/contracts/contrib/src/tests/detokenization.rs b/contracts/contrib/src/tests/detokenization.rs index b2c06908d..5acea9706 100644 --- a/contracts/contrib/src/tests/detokenization.rs +++ b/contracts/contrib/src/tests/detokenization.rs @@ -50,7 +50,7 @@ fn test_execute_detokenization_end_to_end() { } #[test] -#[should_panic(expected = "Error(Contract, #28)")] +#[should_panic(expected = "Error(Contract, #160)")] fn test_execute_detokenization_without_votes_panics() { let env = Env::default(); let client = setup(&env); @@ -66,7 +66,7 @@ fn test_execute_detokenization_without_votes_panics() { } #[test] -#[should_panic(expected = "Error(Contract, #29)")] +#[should_panic(expected = "Error(Contract, #161)")] fn test_propose_detokenization_duplicate_panics() { let env = Env::default(); let client = setup(&env); diff --git a/contracts/contrib/src/tests/dividends.rs b/contracts/contrib/src/tests/dividends.rs index 49cc50789..ae4d09af0 100644 --- a/contracts/contrib/src/tests/dividends.rs +++ b/contracts/contrib/src/tests/dividends.rs @@ -71,7 +71,7 @@ fn test_distribute_dividends_proportional_multiple_holders() { } #[test] -#[should_panic(expected = "Error(Contract, #26)")] +#[should_panic(expected = "Error(Contract, #150)")] fn test_claim_dividends_nothing_to_claim() { let env = Env::default(); let client = setup(&env); diff --git a/contracts/contrib/src/tests/tokenization.rs b/contracts/contrib/src/tests/tokenization.rs index b470b59e5..347bad510 100644 --- a/contracts/contrib/src/tests/tokenization.rs +++ b/contracts/contrib/src/tests/tokenization.rs @@ -42,7 +42,7 @@ fn test_tokenize_asset_success() { } #[test] -#[should_panic(expected = "Error(Contract, #10)")] +#[should_panic(expected = "Error(Contract, #120)")] fn test_tokenize_asset_already_tokenized() { let env = Env::default(); let (client, _) = setup(&env); @@ -71,7 +71,7 @@ fn test_transfer_tokens_success() { } #[test] -#[should_panic(expected = "Error(Contract, #14)")] +#[should_panic(expected = "Error(Contract, #124)")] fn test_transfer_tokens_insufficient_balance() { let env = Env::default(); let (client, _) = setup(&env); @@ -86,7 +86,7 @@ fn test_transfer_tokens_insufficient_balance() { } #[test] -#[should_panic(expected = "Error(Contract, #17)")] +#[should_panic(expected = "Error(Contract, #127)")] fn test_transfer_tokens_blacklisted_recipient() { let env = Env::default(); let (client, _) = setup(&env); diff --git a/contracts/contrib/src/tests/voting.rs b/contracts/contrib/src/tests/voting.rs index b89bd0045..c3e12e14c 100644 --- a/contracts/contrib/src/tests/voting.rs +++ b/contracts/contrib/src/tests/voting.rs @@ -41,7 +41,7 @@ fn test_cast_vote_success() { } #[test] -#[should_panic(expected = "Error(Contract, #22)")] +#[should_panic(expected = "Error(Contract, #141)")] fn test_cast_vote_double_vote_panics() { let env = Env::default(); let client = setup(&env); @@ -56,7 +56,7 @@ fn test_cast_vote_double_vote_panics() { } #[test] -#[should_panic(expected = "Error(Contract, #21)")] +#[should_panic(expected = "Error(Contract, #140)")] fn test_cast_vote_below_threshold_panics() { let env = Env::default(); let client = setup(&env); diff --git a/contracts/multisig-wallet/src/errors.rs b/contracts/multisig-wallet/src/errors.rs index 98ba11901..9f648c48d 100644 --- a/contracts/multisig-wallet/src/errors.rs +++ b/contracts/multisig-wallet/src/errors.rs @@ -1,26 +1,114 @@ use soroban_sdk::contracterror; +/// Contract errors for `multisig-wallet`. +/// +/// Codes follow the workspace allocation in `contracts/ERRORS.md`: +/// +/// - **1–99** are *shared* across every contract and mean the same thing +/// everywhere. +/// - **300–399** belong to `multisig-wallet` alone. +/// +/// Previously this enum numbered from 1 independently, so code `3` meant +/// `Unauthorized` here but `AssetAlreadyExists` in `assetsup` — a backend +/// could not interpret a bare code without also knowing which contract +/// produced it. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] pub enum Error { + // --------------------------------------------------------------- + // Shared: 1–99. Same meaning in every contract in the workspace. + // --------------------------------------------------------------- + /// `initialize` was called on a wallet that already has owners. AlreadyInitialized = 1, + /// An entrypoint was called before the wallet was initialized. NotInitialized = 2, + /// The caller authenticated but is not permitted to perform this action. Unauthorized = 3, - InvalidThreshold = 4, - InsufficientOwners = 5, - TransactionNotFound = 6, - TransactionAlreadyExecuted = 7, - TransactionExpired = 8, - AlreadyConfirmed = 9, - CannotConfirmOwnTransaction = 10, - OwnerAlreadyExists = 11, - OwnerNotFound = 12, - ProposalNotFound = 13, - WalletFrozen = 14, - DailyLimitExceeded = 15, - InvalidProposal = 16, - NotAnOwner = 17, - ThresholdTooHigh = 18, - InvalidArguments = 19, + /// An argument failed validation and no more specific code applies. + InvalidArguments = 4, + + // --------------------------------------------------------------- + // Transaction lifecycle: 300–319 + // --------------------------------------------------------------- + /// No transaction exists under this id. + TransactionNotFound = 300, + /// The transaction has already been executed, cancelled, or expired. + TransactionAlreadyExecuted = 301, + /// The transaction is past its deadline. + TransactionExpired = 302, + /// This owner has already confirmed this transaction or proposal. + AlreadyConfirmed = 303, + /// The initiator may not confirm their own transaction. + CannotConfirmOwnTransaction = 304, + + // --------------------------------------------------------------- + // Owner and threshold governance: 320–339 + // --------------------------------------------------------------- + /// The requested threshold is zero, or exceeds the owner count. + InvalidThreshold = 320, + /// The wallet would be left with fewer than two owners, or with a + /// threshold its remaining owners could never reach. + InsufficientOwners = 321, + /// This address is already an owner. + OwnerAlreadyExists = 322, + /// This address is not an owner of the wallet. + OwnerNotFound = 323, + /// No proposal exists under this id. + ProposalNotFound = 324, + /// The proposal is malformed, or not in a state that allows this action. + InvalidProposal = 325, + /// The caller is not an owner and may not take part in the wallet. + NotAnOwner = 326, + /// The requested threshold exceeds the number of owners. + ThresholdTooHigh = 327, + + // --------------------------------------------------------------- + // Emergency controls and limits: 340–349 + // --------------------------------------------------------------- + /// The wallet is frozen; mutating operations are rejected. + WalletFrozen = 340, + /// The transaction would exceed the configured daily spend limit. + DailyLimitExceeded = 341, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn shared_errors_occupy_the_shared_range() { + // These codes must match assetsup and multisig-transfer exactly. + assert_eq!(Error::AlreadyInitialized as u32, 1); + assert_eq!(Error::NotInitialized as u32, 2); + assert_eq!(Error::Unauthorized as u32, 3); + assert_eq!(Error::InvalidArguments as u32, 4); + } + + #[test] + fn contract_specific_errors_stay_inside_the_wallet_block() { + let codes = [ + Error::TransactionNotFound as u32, + Error::TransactionAlreadyExecuted as u32, + Error::TransactionExpired as u32, + Error::AlreadyConfirmed as u32, + Error::CannotConfirmOwnTransaction as u32, + Error::InvalidThreshold as u32, + Error::InsufficientOwners as u32, + Error::OwnerAlreadyExists as u32, + Error::OwnerNotFound as u32, + Error::ProposalNotFound as u32, + Error::InvalidProposal as u32, + Error::NotAnOwner as u32, + Error::ThresholdTooHigh as u32, + Error::WalletFrozen as u32, + Error::DailyLimitExceeded as u32, + ]; + for code in codes { + assert!( + (300..400).contains(&code), + "multisig-wallet error code is outside its allocated 300-399 block" + ); + } + } } diff --git a/contracts/multisig_transfer/src/errors.rs b/contracts/multisig_transfer/src/errors.rs index 6e06e7610..3e076af5f 100644 --- a/contracts/multisig_transfer/src/errors.rs +++ b/contracts/multisig_transfer/src/errors.rs @@ -1,31 +1,110 @@ use soroban_sdk::contracterror; +/// Contract errors for `multisig-transfer`. +/// +/// Codes follow the workspace allocation in `contracts/ERRORS.md`: +/// +/// - **1–99** are *shared* across every contract and mean the same thing +/// everywhere. +/// - **400–499** belong to `multisig-transfer` alone. +/// +/// This enum previously started at `NotInitialized = 1`, while `assetsup`, +/// `contrib` and `multisig-wallet` all used `1` for `AlreadyInitialized` — the +/// exact opposite condition. That is the collision this allocation removes. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq)] +#[repr(u32)] pub enum MultiSigError { - NotInitialized = 1, - Unauthorized = 2, + // --------------------------------------------------------------- + // Shared: 1–99. Same meaning in every contract in the workspace. + // --------------------------------------------------------------- + /// An entrypoint was called before the contract was initialized. + NotInitialized = 2, + /// The caller authenticated but is not permitted to perform this action. + Unauthorized = 3, - InvalidOwner = 3, - InvalidNewOwner = 4, + // --------------------------------------------------------------- + // Request lifecycle: 400–419 + // --------------------------------------------------------------- + /// No transfer request exists under this id. + RequestNotFound = 400, + /// The request is not pending, so it cannot be approved or cancelled. + RequestNotPending = 401, + /// The request is past its expiry. + RequestExpired = 402, + /// The approval window for this request has closed. + ApprovalDeadlinePassed = 403, + /// The request has not gathered enough approvals to execute. + NotEnoughApprovals = 404, + /// The request's timelock has not yet elapsed. + ExecuteTooEarly = 405, + /// A pending request already exists for this asset. + PendingRequestExists = 406, - AssetNotFound = 5, - AssetRetired = 6, - PendingRequestExists = 7, + // --------------------------------------------------------------- + // Approval rules and approvers: 420–439 + // --------------------------------------------------------------- + /// No approval rule is configured for this asset category. + RuleNotFound = 420, + /// The caller is not an approver for this asset category. + ApproverNotAuthorized = 421, + /// The requester may not approve their own transfer request. + CannotApproveOwnRequest = 422, + /// This approver has already approved this request. + AlreadyApproved = 423, - RuleNotFound = 8, - ApproverNotAuthorized = 9, - CannotApproveOwnRequest = 10, - AlreadyApproved = 11, - - RequestNotFound = 12, - RequestNotPending = 13, + // --------------------------------------------------------------- + // Registry interaction: 440–449 + // --------------------------------------------------------------- + /// The asset is not registered in the configured registry contract. + AssetNotFound = 440, + /// The asset is retired and can no longer be transferred. + AssetRetired = 441, + /// The caller is not the asset's current owner. + InvalidOwner = 442, + /// The proposed new owner is not a valid recipient. + InvalidNewOwner = 443, + /// The call into the registry contract failed. + RegistryCallFailed = 444, +} - RequestExpired = 14, - ApprovalDeadlinePassed = 15, +#[cfg(test)] +mod tests { + use super::*; - NotEnoughApprovals = 16, - ExecuteTooEarly = 17, + #[test] + fn shared_errors_occupy_the_shared_range() { + // These codes must match assetsup and multisig-wallet exactly. In + // particular NotInitialized is 2 here, not 1 as it used to be. + assert_eq!(MultiSigError::NotInitialized as u32, 2); + assert_eq!(MultiSigError::Unauthorized as u32, 3); + } - RegistryCallFailed = 18, + #[test] + fn contract_specific_errors_stay_inside_the_transfer_block() { + let codes = [ + MultiSigError::RequestNotFound as u32, + MultiSigError::RequestNotPending as u32, + MultiSigError::RequestExpired as u32, + MultiSigError::ApprovalDeadlinePassed as u32, + MultiSigError::NotEnoughApprovals as u32, + MultiSigError::ExecuteTooEarly as u32, + MultiSigError::PendingRequestExists as u32, + MultiSigError::RuleNotFound as u32, + MultiSigError::ApproverNotAuthorized as u32, + MultiSigError::CannotApproveOwnRequest as u32, + MultiSigError::AlreadyApproved as u32, + MultiSigError::AssetNotFound as u32, + MultiSigError::AssetRetired as u32, + MultiSigError::InvalidOwner as u32, + MultiSigError::InvalidNewOwner as u32, + MultiSigError::RegistryCallFailed as u32, + ]; + for code in codes { + assert!( + (400..500).contains(&code), + "multisig-transfer error code is outside its allocated 400-499 block" + ); + } + } }