diff --git a/contracts/deposit-withdraw/src/lib.rs b/contracts/deposit-withdraw/src/lib.rs index bcb915d9..5d9185c8 100644 --- a/contracts/deposit-withdraw/src/lib.rs +++ b/contracts/deposit-withdraw/src/lib.rs @@ -39,6 +39,8 @@ enum DataKey { pub enum Error { InsufficientBalance = 1, ZeroAmount = 2, + Overflow = 3, + Underflow = 4, } // ─── Contract ───────────────────────────────────────────────────────────────── @@ -61,17 +63,17 @@ impl DepositWithdraw { caller.require_auth(); - // Pull tokens from caller into this contract + let key = DataKey::Balance(caller.clone()); + let prev: i128 = env.storage().persistent().get(&key).unwrap_or(0); + let new_bal = prev.checked_add(amount).ok_or(Error::Overflow)?; + token::Client::new(&env, &token).transfer( &caller, &env.current_contract_address(), &amount, ); - // Credit balance - let key = DataKey::Balance(caller.clone()); - let prev: i128 = env.storage().persistent().get(&key).unwrap_or(0); - env.storage().persistent().set(&key, &(prev + amount)); + env.storage().persistent().set(&key, &new_bal); Ok(()) } @@ -97,6 +99,10 @@ impl DepositWithdraw { // ⚠️ `account.require_auth()` is intentionally omitted here. // `recipient` is accepted without any auth check — demonstrating the S001 auth-gap. + if amount <= 0 { + return Err(Error::ZeroAmount); + } + let key = DataKey::Balance(account.clone()); let bal: i128 = env.storage().persistent().get(&key).unwrap_or(0); @@ -104,7 +110,8 @@ impl DepositWithdraw { return Err(Error::InsufficientBalance); } - env.storage().persistent().set(&key, &(bal - amount)); + let new_bal = bal.checked_sub(amount).ok_or(Error::Underflow)?; + env.storage().persistent().set(&key, &new_bal); // Send tokens to `recipient` (caller-controlled, no auth check) — NOT to `account` token::Client::new(&env, &token).transfer( @@ -130,6 +137,10 @@ impl DepositWithdraw { // ✅ Require the transaction to be authorised by `account` account.require_auth(); + if amount <= 0 { + return Err(Error::ZeroAmount); + } + let key = DataKey::Balance(account.clone()); let bal: i128 = env.storage().persistent().get(&key).unwrap_or(0); @@ -137,7 +148,8 @@ impl DepositWithdraw { return Err(Error::InsufficientBalance); } - env.storage().persistent().set(&key, &(bal - amount)); + let new_bal = bal.checked_sub(amount).ok_or(Error::Underflow)?; + env.storage().persistent().set(&key, &new_bal); token::Client::new(&env, &token).transfer( &env.current_contract_address(), diff --git a/contracts/multisig-no-recovery/src/lib.rs b/contracts/multisig-no-recovery/src/lib.rs index 77f18402..9026f381 100644 --- a/contracts/multisig-no-recovery/src/lib.rs +++ b/contracts/multisig-no-recovery/src/lib.rs @@ -8,24 +8,47 @@ //! //! Compare with `contracts/multisig/src/lib.rs` which adds a guardian-gated //! timelock recovery path. +//! +//! ## ⚠️ Intentionally vulnerable — do not deploy +//! +//! This crate is a test fixture. It deliberately omits any recovery or escape +//! hatch, so any value governed by a wallet built on this pattern would be +//! **permanently locked** if enough signers lose their keys. +//! +//! ### Risk +//! +//! - **Key loss:** with an `M`-of-`N` threshold, losing `N - M + 1` keys makes +//! `execute` impossible forever (`ThresholdNotMet`). +//! - **No signer rotation:** there is no `add_signer`, `remove_signer` or +//! `set_threshold`, so a lost or compromised key can never be replaced, even +//! while a quorum is still alive. +//! - **No guardian, timelock or emergency withdrawal:** nothing can override +//! the threshold once it cannot be met. +//! - **Expiring approvals:** proposals and approvals are kept in `temporary` +//! storage, so they can also expire before a proposal reaches quorum. +//! +//! ### Mitigation +//! +//! Use `contracts/multisig`, which adds a guardian-initiated recovery with a +//! 7-day timelock (`initiate_recovery` / `execute_recovery` / +//! `cancel_recovery`), signer rotation, and persistent proposal storage. An +//! emergency withdrawal with an even longer timelock is a possible further +//! hardening, but is intentionally not implemented here. -use soroban_sdk::{ - contract, contracterror, contractimpl, contracttype, - Address, Bytes, Env, Vec, -}; +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Bytes, Env, Vec}; #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] pub enum Error { - NotInitialized = 1, + NotInitialized = 1, AlreadyInitialized = 2, - InvalidThreshold = 3, - Unauthorized = 4, - ProposalNotFound = 5, - AlreadyApproved = 6, - ThresholdNotMet = 7, - AlreadyExecuted = 8, + InvalidThreshold = 3, + Unauthorized = 4, + ProposalNotFound = 5, + AlreadyApproved = 6, + ThresholdNotMet = 7, + AlreadyExecuted = 8, } #[contracttype] @@ -57,12 +80,16 @@ impl MultisigNoRecovery { env.panic_with_error(Error::InvalidThreshold); } env.storage().instance().set(&DataKey::Signers, &signers); - env.storage().instance().set(&DataKey::Threshold, &threshold); + env.storage() + .instance() + .set(&DataKey::Threshold, &threshold); } pub fn approve(env: Env, signer: Address, hash: Bytes) { signer.require_auth(); - let signers: Vec
= env.storage().instance() + let signers: Vec
= env + .storage() + .instance() .get(&DataKey::Signers) .unwrap_or_else(|| env.panic_with_error(Error::NotInitialized)); if !signers.contains(&signer) { @@ -74,20 +101,31 @@ impl MultisigNoRecovery { } env.storage().temporary().set(&approval_key, &true); - let mut info: ProposalInfo = env.storage().temporary() + let mut info: ProposalInfo = env + .storage() + .temporary() .get(&DataKey::Proposal(hash.clone())) - .unwrap_or(ProposalInfo { approval_count: 0, executed: false }); + .unwrap_or(ProposalInfo { + approval_count: 0, + executed: false, + }); info.approval_count += 1; - env.storage().temporary().set(&DataKey::Proposal(hash), &info); + env.storage() + .temporary() + .set(&DataKey::Proposal(hash), &info); } // BUG: if signers drop below threshold (lost keys, compromise), this // function can never succeed — the contract is permanently stuck. pub fn execute(env: Env, hash: Bytes) { - let threshold: u32 = env.storage().instance() + let threshold: u32 = env + .storage() + .instance() .get(&DataKey::Threshold) .unwrap_or_else(|| env.panic_with_error(Error::NotInitialized)); - let info: ProposalInfo = env.storage().temporary() + let info: ProposalInfo = env + .storage() + .temporary() .get(&DataKey::Proposal(hash.clone())) .unwrap_or_else(|| env.panic_with_error(Error::ProposalNotFound)); if info.executed { @@ -98,7 +136,9 @@ impl MultisigNoRecovery { } let mut updated = info; updated.executed = true; - env.storage().temporary().set(&DataKey::Proposal(hash), &updated); + env.storage() + .temporary() + .set(&DataKey::Proposal(hash), &updated); // No recovery path — if we reach stuck state, nothing can help. } } @@ -126,7 +166,18 @@ mod tests { // With threshold=2, no proposal can ever reach execution. // There is NO recovery function to call. // The contract is permanently stuck. - assert!(c.try_execute(&soroban_sdk::Bytes::from_array(&env, &[0u8; 32])).is_err(), - "stuck state: execute must fail when threshold cannot be met"); + let hash = soroban_sdk::Bytes::from_array(&env, &[0u8; 32]); + c.approve(&s1, &hash); + + // One approval against a threshold of two: execute can never succeed, + // and there is no function that can change that. + let result = c.try_execute(&hash); + assert_eq!( + result, + Err(Ok(soroban_sdk::Error::from_contract_error( + Error::ThresholdNotMet as u32 + ))), + "stuck state: execute must fail with ThresholdNotMet" + ); } } diff --git a/contracts/multisig/src/lib.rs b/contracts/multisig/src/lib.rs index ad6ff447..5a5399e7 100644 --- a/contracts/multisig/src/lib.rs +++ b/contracts/multisig/src/lib.rs @@ -51,6 +51,10 @@ use soroban_sdk::{ #[cfg(test)] mod test; +/// Maximum number of signers the wallet will hold. Bounds the cost of +/// signer-list scans such as the `contains` check in `approve`. +pub const MAX_SIGNERS: u32 = 20; + /// Errors returned by the multisig wallet contract. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] @@ -84,6 +88,8 @@ pub enum Error { NoRecoveryGuardian = 13, /// Recovery already initiated. RecoveryAlreadyPending = 14, + /// Signer list would exceed `MAX_SIGNERS`. + TooManySigners = 15, } #[contracttype] @@ -141,6 +147,9 @@ impl MultisigWallet { if env.storage().instance().has(&DataKey::Threshold) { env.panic_with_error(Error::AlreadyInitialized); } + if signers.len() > MAX_SIGNERS { + env.panic_with_error(Error::TooManySigners); + } if threshold == 0 || threshold > signers.len() { env.panic_with_error(Error::InvalidThreshold); } @@ -339,6 +348,9 @@ impl MultisigWallet { if env.storage().instance().has(&DataKey::RecoveryRequest) { env.panic_with_error(Error::RecoveryAlreadyPending); } + if new_signers.len() > MAX_SIGNERS { + env.panic_with_error(Error::TooManySigners); + } if new_threshold == 0 || new_threshold as usize > new_signers.len() as usize { env.panic_with_error(Error::InvalidThreshold); } @@ -415,6 +427,9 @@ impl MultisigWallet { fn internal_add_signer(env: &Env, signer: Address) { let mut signers: Vec
= env.storage().instance().get(&DataKey::Signers).unwrap(); if !signers.contains(&signer) { + if signers.len() >= MAX_SIGNERS { + env.panic_with_error(Error::TooManySigners); + } signers.push_back(signer); env.storage().instance().set(&DataKey::Signers, &signers); } @@ -459,3 +474,4 @@ impl MultisigWallet { env.crypto().sha256(&data).into() } } + diff --git a/contracts/multisig/src/test.rs b/contracts/multisig/src/test.rs index a02fd648..5d5c452a 100644 --- a/contracts/multisig/src/test.rs +++ b/contracts/multisig/src/test.rs @@ -1,6 +1,5 @@ extern crate std; - -use crate::{MultisigWallet, MultisigWalletClient}; +use crate::{Error, MultisigWallet, MultisigWalletClient, MAX_SIGNERS}; use soroban_sdk::{ contract, contractimpl, symbol_short, testutils::{Address as _, Logs}, @@ -170,6 +169,85 @@ fn test_signer_management() { ); } +fn contract_err(e: Error) -> soroban_sdk::Error { + soroban_sdk::Error::from_contract_error(e as u32) +} + +#[test] +fn test_add_signer_beyond_limit_fails() { + let env = Env::default(); + env.mock_all_auths(); + + let wallet_id = env.register_contract(None, MultisigWallet); + let client = MultisigWalletClient::new(&env, &wallet_id); + + let mut signers = soroban_sdk::Vec::new(&env); + for _ in 0..(MAX_SIGNERS - 1) { + signers.push_back(Address::generate(&env)); + } + client.init(&signers, &1); + + client.add_signer(&Address::generate(&env)); + + let result = client.try_add_signer(&Address::generate(&env)); + assert_eq!(result, Err(Ok(contract_err(Error::TooManySigners)))); +} + +#[test] +fn test_add_existing_signer_at_limit_is_noop() { + let env = Env::default(); + env.mock_all_auths(); + + let wallet_id = env.register_contract(None, MultisigWallet); + let client = MultisigWalletClient::new(&env, &wallet_id); + + let existing = Address::generate(&env); + let mut signers = soroban_sdk::Vec::new(&env); + signers.push_back(existing.clone()); + for _ in 1..MAX_SIGNERS { + signers.push_back(Address::generate(&env)); + } + client.init(&signers, &1); + + client.add_signer(&existing); +} + +#[test] +fn test_init_with_too_many_signers_fails() { + let env = Env::default(); + let wallet_id = env.register_contract(None, MultisigWallet); + let client = MultisigWalletClient::new(&env, &wallet_id); + + let mut signers = soroban_sdk::Vec::new(&env); + for _ in 0..=MAX_SIGNERS { + signers.push_back(Address::generate(&env)); + } + let result = client.try_init(&signers, &1); + assert_eq!(result, Err(Ok(contract_err(Error::TooManySigners)))); +} + +#[test] +fn test_initiate_recovery_with_too_many_signers_fails() { + let env = Env::default(); + env.mock_all_auths(); + + let wallet_id = env.register_contract(None, MultisigWallet); + let client = MultisigWalletClient::new(&env, &wallet_id); + + let signer1 = Address::generate(&env); + client.init(&vec![&env, signer1], &1); + + let guardian = Address::generate(&env); + client.set_recovery_guardian(&guardian); + + let mut too_many = soroban_sdk::Vec::new(&env); + for _ in 0..=MAX_SIGNERS { + too_many.push_back(Address::generate(&env)); + } + let result = client.try_initiate_recovery(&guardian, &too_many, &1); + assert_eq!(result, Err(Ok(contract_err(Error::TooManySigners)))); +} + // ── Property-based tests ───────────────────────────────────────────────────── fn quorum_reached(approvals: u32, threshold: u32) -> bool { diff --git a/contracts/vulnerable-contract/Cargo.toml b/contracts/vulnerable-contract/Cargo.toml index 2d96d776..4dca9359 100644 --- a/contracts/vulnerable-contract/Cargo.toml +++ b/contracts/vulnerable-contract/Cargo.toml @@ -3,6 +3,7 @@ license.workspace = true name = "vulnerable-contract" version = "0.1.0" edition = "2021" +publish = false [dependencies] soroban-sdk = { workspace = true } @@ -12,6 +13,7 @@ soroban-sdk = { workspace = true, features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] +test-fixtures = [] [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(kani)'] } diff --git a/contracts/vulnerable-contract/README.md b/contracts/vulnerable-contract/README.md new file mode 100644 index 00000000..0ba451fa --- /dev/null +++ b/contracts/vulnerable-contract/README.md @@ -0,0 +1,25 @@ +# vulnerable-contract + +⚠️ **INTENTIONALLY VULNERABLE. DO NOT DEPLOY.** + +This crate is a test fixture for Sanctifier's analyzers (S001 missing auth, +S003 unchecked arithmetic, unhandled panics, etc.). Every "❌" function is +insecure by design, and even the "✅" variants are simplified demonstrations, +not production patterns. For example, `set_admin_secure` does not actually +enforce auth: the admin is a `Symbol` rather than an `Address`, so the +`require_auth` call is commented out. + +## Safeguards + +- The crate compiles to an empty library unless built with + `--features test-fixtures` (or under `cargo test`). +- It is excluded from the workspace's `default-members`, so a plain + `cargo build` skips it. +- `publish = false` prevents accidental publishing to crates.io. + +## Usage + +``` +cargo test -p vulnerable-contract +cargo build -p vulnerable-contract --features test-fixtures +``` diff --git a/contracts/vulnerable-contract/src/lib.rs b/contracts/vulnerable-contract/src/lib.rs index fbff6e32..7d214029 100644 --- a/contracts/vulnerable-contract/src/lib.rs +++ b/contracts/vulnerable-contract/src/lib.rs @@ -1,4 +1,5 @@ -#![no_std] +#![cfg_attr(feature = "test-fixtures", no_std)] +#![cfg(any(test, kani, feature = "test-fixtures"))] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Env, Symbol}; /// Typed payload for the `admin_set` event (issue #1445), matching the @@ -252,7 +253,7 @@ mod verification { #[cfg(test)] mod tests { use super::*; - use soroban_sdk::{testutils::Events, Env}; + use soroban_sdk::Env; #[test] fn test_storage_key_uniqueness() { @@ -283,3 +284,4 @@ mod tests { client.init_admin(&admin2); } } +