Loan manager deposit collateral cei fix

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,

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();

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()

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;
@@ -240,6 +243,41 @@ 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);
+    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);
+
+    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() {