feat: add new errors and checks to contract to validate logic

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,14 +99,19 @@ 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);
 
         if amount > bal {
             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,14 +137,19 @@ 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);
 
         if amount > bal {
             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(),

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<Address> = env.storage().instance()
+        let signers: Vec<Address> = 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"
+        );
     }
 }

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<Address> = 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()
     }
 }
+

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 {

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)'] }