Merge pull request #27 from jotel-dev/loan-manager-deposit-collateral-cei-fix

Loan manager deposit collateral cei fix

Author: ogazboiz

loan_manager/src/lib.rs

@@ -1392,7 +1392,7 @@ impl LoanManager {
         }
 
         let loan_key = DataKey::Loan(loan_id);
-        let loan: Loan = env
+        let mut loan: Loan = env
             .storage()
             .persistent()
             .get(&loan_key)
@@ -1419,27 +1419,32 @@ 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),
+            (symbol_short!("ColDep"), loan_id, stored_loan.borrower),
             updated_collateral,
         );
 

loan_manager/src/test.rs

@@ -3,7 +3,40 @@ 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, contractimpl, testutils::Address as _, Address, BytesN, Env, String, Symbol,
+};
+
+#[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 +1368,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.try_deposit_collateral(&loan_id, &300);
+    assert_eq!(result, Err(Ok(LoanError::LoanNotFound)));
+}
+
 #[test]
 fn test_collateral_is_seized_on_default() {
     let env = Env::default();