From e920c95fea94f783f7ca7872aeddda52e9fda153 Mon Sep 17 00:00:00 2001 From: Ogunmodede Joel Taiwo Date: Wed, 17 Jun 2026 21:46:43 +0100 Subject: [PATCH 1/3] Multisig finalize_admin_transfer counts approvals without validating they came from current signers --- multisig_governance/src/lib.rs | 21 +++++++++++++++++---- multisig_governance/src/test.rs | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/multisig_governance/src/lib.rs b/multisig_governance/src/lib.rs index 1569077..185b29e 100644 --- a/multisig_governance/src/lib.rs +++ b/multisig_governance/src/lib.rs @@ -303,7 +303,7 @@ impl GovernanceContract { // Map::set is idempotent — duplicate calls do not increment the count pending.approvals.set(signer.clone(), true); - let approvals_so_far = pending.approvals.len(); + let approvals_so_far = Self::count_valid_approvals(&pending); let threshold = pending.threshold; let proposal_id = pending.id; @@ -366,8 +366,8 @@ impl GovernanceContract { panic!("proposal has expired (4016)"); } - // INV-2: threshold must be met - let approval_count = pending.approvals.len(); + // INV-2: threshold must be met using only approvals from the current signer list. + let approval_count = Self::count_valid_approvals(&pending); if approval_count < pending.threshold { panic!("threshold not met — more approvals required (4011)"); } @@ -562,7 +562,7 @@ impl GovernanceContract { .instance() .get(&KEY_PENDING) .expect("no pending transfer (4004)"); - pending.approvals.len() + Self::count_valid_approvals(&pending) } /// Returns seconds remaining until the timelock expires. @@ -587,6 +587,19 @@ impl GovernanceContract { // ── Private helpers ─────────────────────────────────────────────────────── + fn count_valid_approvals(pending: &PendingTransfer) -> u32 { + // Approval counts are derived from the current signer list only. + // This prevents stale or invalid approvals in the map from satisfying + // the threshold if they are not associated with an active signer. + let mut count: u32 = 0; + for signer in pending.signers.iter() { + if let Some(true) = pending.approvals.get(signer) { + count = count.saturating_add(1); + } + } + count + } + fn read_admin(env: &Env) -> Address { env.storage() .instance() diff --git a/multisig_governance/src/test.rs b/multisig_governance/src/test.rs index b515fa5..e1426d0 100644 --- a/multisig_governance/src/test.rs +++ b/multisig_governance/src/test.rs @@ -240,6 +240,39 @@ fn approve_is_idempotent() { assert_eq!(client.get_approval_count(), 1); } +#[test] +#[should_panic(expected = "threshold not met")] +fn finalize_only_counts_current_signer_approvals() { + let (env, client, admin, _) = setup(); + let s1 = Address::generate(&env); + let s2 = Address::generate(&env); + let signers = Vec::from_slice(&env, &[s1.clone(), s2.clone()]); + + set_ts(&env, 1000); + client.propose_admin_transfer( + &Address::generate(&env), + &signers, + &2, + &MIN_TIMELOCK_SECONDS, + ); + + client.approve_transfer(&s1); + + let invalid_signer = Address::generate(&env); + let mut pending: PendingTransfer = env + .storage() + .instance() + .get(&KEY_PENDING) + .expect("pending transfer"); + pending.approvals.set(invalid_signer, true); + env.storage().instance().set(&KEY_PENDING, &pending); + + assert_eq!(client.get_approval_count(), 1); + + set_ts(&env, 1000 + MIN_TIMELOCK_SECONDS + 1); + client.finalize_admin_transfer(&admin); +} + #[test] #[should_panic(expected = "caller is not in the signer list")] fn approve_rejects_non_signer() { From 2cd6ebb697a5b9296ef09bc5ab048fd5ff68f17d Mon Sep 17 00:00:00 2001 From: Ogunmodede Joel Taiwo Date: Thu, 18 Jun 2026 08:11:47 +0100 Subject: [PATCH 2/3] tests(multisig): wrap storage access in env.as_contract and document approval invariant --- multisig_governance/src/test.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/multisig_governance/src/test.rs b/multisig_governance/src/test.rs index e1426d0..533ed5b 100644 --- a/multisig_governance/src/test.rs +++ b/multisig_governance/src/test.rs @@ -1,6 +1,9 @@ use super::*; use soroban_sdk::testutils::{Address as _, Ledger, LedgerInfo}; use soroban_sdk::{Address, BytesN, Env, Vec}; + +//! Invariant: approval count is always derived from the current signer set, +//! never from `approvals.len()` or any stale approvals stored in `PendingTransfer`. #[allow(deprecated)] #[contract] pub struct MockTarget; @@ -259,13 +262,15 @@ fn finalize_only_counts_current_signer_approvals() { client.approve_transfer(&s1); let invalid_signer = Address::generate(&env); - let mut pending: PendingTransfer = env - .storage() - .instance() - .get(&KEY_PENDING) - .expect("pending transfer"); - pending.approvals.set(invalid_signer, true); - env.storage().instance().set(&KEY_PENDING, &pending); + env.as_contract(&client.address, || { + let mut pending: PendingTransfer = env + .storage() + .instance() + .get(&KEY_PENDING) + .expect("pending transfer"); + pending.approvals.set(invalid_signer, true); + env.storage().instance().set(&KEY_PENDING, &pending); + }); assert_eq!(client.get_approval_count(), 1); From d76aa3717c6ce5dc4d53c298537e1580f99afdfc Mon Sep 17 00:00:00 2001 From: Ogunmodede Joel Taiwo Date: Thu, 18 Jun 2026 09:04:07 +0100 Subject: [PATCH 3/3] [Contracts] LoanManager deposit_collateral CEI fix and regression test --- loan_manager/src/lib.rs | 25 ++++++++------ loan_manager/src/test.rs | 74 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/loan_manager/src/lib.rs b/loan_manager/src/lib.rs index bee04dc..ae6abfd 100644 --- a/loan_manager/src/lib.rs +++ b/loan_manager/src/lib.rs @@ -1380,25 +1380,30 @@ impl LoanManager { .storage() .instance() .get(&DataKey::Token) - .expect("token not set"); + .ok_or(LoanError::NotInitialized)?; let token_client = TokenClient::new(&env, &token); - token_client.transfer(&loan.borrower, &env.current_contract_address(), &amount); - - let loan_key = DataKey::Loan(loan_id); - let mut loan: Loan = env - .storage() - .persistent() - .get(&loan_key) - .expect("loan not found"); let updated_collateral = loan .collateral_amount .checked_add(amount) - .expect("collateral overflow"); + .ok_or(LoanError::AmountTooLarge)?; loan.collateral_amount = updated_collateral; env.storage().persistent().set(&loan_key, &loan); Self::bump_persistent_ttl(&env, &loan_key); + // CEI: commit the updated loan state before the external token transfer. + token_client.transfer(&loan.borrower, &env.current_contract_address(), &amount); + + // Re-validate after the external interaction with typed errors rather than panicking. + let stored_loan: Loan = env + .storage() + .persistent() + .get(&loan_key) + .ok_or(LoanError::LoanNotFound)?; + if stored_loan.status != LoanStatus::Approved { + return Err(LoanError::LoanNotActive); + } + env.events().publish( (symbol_short!("ColDep"), loan_id, loan.borrower), updated_collateral, diff --git a/loan_manager/src/test.rs b/loan_manager/src/test.rs index 9552f98..3ff9be5 100644 --- a/loan_manager/src/test.rs +++ b/loan_manager/src/test.rs @@ -3,7 +3,43 @@ use lending_pool::{LendingPool, LendingPoolClient}; use remittance_nft::{RemittanceNFT, RemittanceNFTClient}; use soroban_sdk::testutils::Ledger as _; use soroban_sdk::token::{Client as TokenClient, StellarAssetClient}; -use soroban_sdk::{testutils::Address as _, Address, BytesN, Env, String}; +use soroban_sdk::{contract, contractclient, contractimpl, testutils::Address as _, Address, BytesN, Env, String, Symbol}; + +#[contractclient(name = "MaliciousTokenClient")] +pub trait MaliciousTokenInterface { + fn set_attack_target(env: Env, manager: Address, loan_id: u32); +} + +#[contract] +pub struct MaliciousToken; + +#[contractimpl] +impl MaliciousToken { + pub fn set_attack_target(env: Env, manager: Address, loan_id: u32) { + env.storage() + .persistent() + .set(&Symbol::new(&env, "manager"), &manager); + env.storage() + .persistent() + .set(&Symbol::new(&env, "loan_id"), &loan_id); + } + + pub fn transfer(env: Env, from: Address, to: Address, amount: i128) { + let manager: Address = env + .storage() + .persistent() + .get(&Symbol::new(&env, "manager")) + .unwrap(); + let loan_id: u32 = env + .storage() + .persistent() + .get(&Symbol::new(&env, "loan_id")) + .unwrap(); + env.as_contract(&manager, || { + env.storage().persistent().remove(&DataKey::Loan(loan_id)); + }); + } +} fn setup_test<'a>( env: &Env, @@ -1335,6 +1371,42 @@ fn test_deposit_collateral_and_auto_release_on_full_repayment() { ); } +#[test] +fn test_deposit_collateral_rejects_loan_removed_during_token_transfer() { + let env = Env::default(); + env.mock_all_auths_allowing_non_root_auth(); + + let (manager, nft_client, pool_client, _token_id, _token_admin) = setup_test(&env); + let borrower = Address::generate(&env); + + let history_hash = BytesN::from_array(&env, &[0u8; 32]); + nft_client.mint( + &borrower, + &650, + &history_hash, + &String::from_str(&env, "ipfs://QmTest"), + &None, + ); + + let stellar_token = StellarAssetClient::new(&env, &token_id); + stellar_token.mint(&pool_client, &20_000); + stellar_token.mint(&borrower, &20_000); + + let loan_id = manager.request_loan(&borrower, &1_000, &17280); + manager.approve_loan(&loan_id); + + let malicious = env.register(MaliciousToken, ()); + let malicious_client = MaliciousTokenClient::new(&env, &malicious); + malicious_client.set_attack_target(&manager.address, &loan_id); + + env.as_contract(&manager.address, || { + env.storage().instance().set(&DataKey::Token, &malicious); + }); + + let result = manager.deposit_collateral(&loan_id, &300); + assert_eq!(result, Err(LoanError::LoanNotFound)); +} + #[test] fn test_collateral_is_seized_on_default() { let env = Env::default();