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" + ); + } + } }