fix(settlement): add cross-contract reentrancy guard (#8)

contracts/settlement/src/lib.rs

@@ -4,6 +4,8 @@ mod constants;
 mod conversion;
 mod fees;
 mod rate_application;
+mod reentrancy;
+mod storage;
 mod token_utils;
 mod types;
 
@@ -15,6 +17,7 @@ mod test;
 use crate::constants::MAX_FEE_RATE_BPS;
 use crate::conversion::convert_to_settlement_currency;
 use crate::fees::compute_fee;
+use crate::reentrancy::ReentrancyGuard;
 use crate::token_utils::collect_fee;
 use crate::types::{SettlementArgs, SettlementResult};
 
@@ -31,6 +34,8 @@ pub enum SettlementError {
     InvalidFeeRate = 1,
     InsufficientBalance = 2,
     SlippageExceeded = 3,
+    /// A cross-contract callback attempted to re-enter a guarded entry point.
+    ReentrantCall = 4,
 }
 
 #[contract]
@@ -60,6 +65,9 @@ impl SettlementContract {
         amount: i128,
         rate_bps: u32,
     ) -> (i128, i128) {
+        // Acquire before any cross-contract call; released on scope exit.
+        let _guard = ReentrancyGuard::new(&env);
+
         if rate_bps > MAX_FEE_RATE_BPS {
             panic_with_error!(&env, SettlementError::InvalidFeeRate);
         }
@@ -110,6 +118,11 @@ impl SettlementContract {
         args: SettlementArgs,
         rate_bps: u32,
     ) -> SettlementResult {
+        // Acquire before any cross-contract call (oracle / token); released on
+        // scope exit. Blocks a malicious token or oracle from re-entering
+        // finalize_settlement before this invocation completes.
+        let _guard = ReentrancyGuard::new(&env);
+
         if rate_bps > MAX_FEE_RATE_BPS {
             panic_with_error!(&env, SettlementError::InvalidFeeRate);
         }

contracts/settlement/src/reentrancy.rs

@@ -0,0 +1,81 @@
+//! Cross-contract reentrancy protection.
+//!
+//! The settlement entry points yield control to untrusted contracts at the
+//! cross-contract call boundary: the price oracle (`get_price_value`) and the
+//! settlement token (`transfer`, via [`crate::token_utils::collect_fee`] and the
+//! net-amount transfer). A malicious token or oracle can re-enter the settlement
+//! contract before the original invocation finishes and manipulate intermediate
+//! state (e.g. double-spend a fee transfer, settle twice off one authorization).
+//!
+//! [`ReentrancyGuard`] is a strict, RAII mutex: acquiring it while it is already
+//! held panics, so **any** re-entry into a guarded entry point aborts the whole
+//! transaction. It is acquired before any external call and released when the
+//! guard goes out of scope.
+//!
+//! ## Why a strict mutex (no re-entry) rather than a bounded callback depth
+//!
+//! For payment finalization there is no legitimate reason to re-enter while a
+//! settlement is in flight, so the safest control is to forbid re-entry entirely.
+//! That is strictly stronger than allowing a bounded depth and removes the class
+//! of bug where "a little" reentrancy is still exploitable.
+//!
+//! ## Cleanup on the panic path
+//!
+//! On normal return the lock is cleared by [`Drop`]. On a panic the Soroban host
+//! reverts **all** storage writes made by the invocation, so the lock is
+//! discarded automatically — important because Wasm builds use `panic = "abort"`,
+//! where `Drop` does not run during unwinding. Both paths therefore leave the
+//! lock clear for the next top-level invocation.
+
+use soroban_sdk::{panic_with_error, Env};
+
+use crate::storage::DataKey;
+use crate::SettlementError;
+
+/// TTL (in ledgers) for the lock entry. The lock only needs to outlive the
+/// current invocation (a single ledger); it is cleared on return and reverted on
+/// panic, so this is a safety floor rather than a functional dependency.
+const LOCK_TTL_LEDGERS: u32 = 1;
+
+/// RAII reentrancy mutex. Acquire at the top of every externally-callable entry
+/// point that performs cross-contract calls.
+pub struct ReentrancyGuard<'a> {
+    env: &'a Env,
+}
+
+impl<'a> ReentrancyGuard<'a> {
+    /// Acquire the lock. Panics with [`SettlementError::ReentrantCall`] if it is
+    /// already held — i.e. if this is a reentrant call.
+    pub fn new(env: &'a Env) -> Self {
+        let locked: bool = env
+            .storage()
+            .temporary()
+            .get(&DataKey::ReentrancyLock)
+            .unwrap_or(false);
+
+        if locked {
+            panic_with_error!(env, SettlementError::ReentrantCall);
+        }
+
+        env.storage()
+            .temporary()
+            .set(&DataKey::ReentrancyLock, &true);
+        env.storage().temporary().extend_ttl(
+            &DataKey::ReentrancyLock,
+            LOCK_TTL_LEDGERS,
+            LOCK_TTL_LEDGERS,
+        );
+
+        ReentrancyGuard { env }
+    }
+}
+
+impl Drop for ReentrancyGuard<'_> {
+    fn drop(&mut self) {
+        // Release on normal completion. (On panic the host revert handles this.)
+        self.env
+            .storage()
+            .temporary()
+            .set(&DataKey::ReentrancyLock, &false);
+    }
+}

contracts/settlement/src/storage.rs

@@ -0,0 +1,12 @@
+//! Storage key definitions for the settlement contract.
+
+use soroban_sdk::contracttype;
+
+/// Storage keys used by the settlement contract.
+#[contracttype]
+#[derive(Clone)]
+pub enum DataKey {
+    /// Cross-contract reentrancy mutex. Held in *temporary* storage for the
+    /// duration of a single contract invocation (see [`crate::reentrancy`]).
+    ReentrancyLock,
+}

contracts/settlement/src/test.rs

@@ -170,6 +170,42 @@ fn test_randomized_properties() {
 use soroban_sdk::testutils::Address as _;
 use soroban_sdk::{contract, contractimpl, contracttype, token, Address, Env};
 
+/// A malicious token whose `transfer` re-enters the settlement contract,
+/// modelling the cross-contract reentrancy attack from issue #8.
+#[contracttype]
+#[derive(Clone)]
+enum MalKey {
+    Settlement,
+    Payer,
+    Collector,
+}
+
+#[contract]
+struct MaliciousToken;
+
+#[contractimpl]
+impl MaliciousToken {
+    pub fn setup(env: Env, settlement: Address, payer: Address, collector: Address) {
+        env.storage().instance().set(&MalKey::Settlement, &settlement);
+        env.storage().instance().set(&MalKey::Payer, &payer);
+        env.storage().instance().set(&MalKey::Collector, &collector);
+    }
+
+    /// SEP-41 `transfer` entry point invoked by the settlement contract's
+    /// `collect_fee`. Instead of moving tokens it attempts to re-enter `settle`,
+    /// which must be rejected by the reentrancy guard.
+    pub fn transfer(env: Env, _from: Address, _to: Address, _amount: i128) {
+        let settlement: Address = env.storage().instance().get(&MalKey::Settlement).unwrap();
+        let payer: Address = env.storage().instance().get(&MalKey::Payer).unwrap();
+        let collector: Address = env.storage().instance().get(&MalKey::Collector).unwrap();
+        let me = env.current_contract_address();
+
+        let client = SettlementContractClient::new(&env, &settlement);
+        // Reentrant call: settle again using this malicious token as the token.
+        client.settle(&me, &payer, &payer, &collector, &10_000i128, &100u32);
+    }
+}
+
 use crate::constants::DECIMAL_DENOMINATOR;
 use crate::types::SettlementArgs;
 use crate::{SettlementContract, SettlementContractClient};
@@ -350,3 +386,68 @@ fn test_user_min_expected_amount_higher_than_actual_fails() {
 
     assert!(result.is_err());
 }
+
+#[test]
+fn test_settle_normal_flow_with_guard() {
+    // The reentrancy guard must not break a legitimate single (non-reentrant) call.
+    let (env, _oracle_id, _admin, payer, payee, fee_collector) = setup_env();
+    let settlement_id = setup_settlement(&env);
+    let settlement_client = SettlementContractClient::new(&env, &settlement_id);
+
+    let token_id = setup_token(&env, &payer, 1_000_000i128);
+
+    let (net, fee) =
+        settlement_client.settle(&token_id, &payer, &payee, &fee_collector, &10_000i128, &100u32);
+
+    assert_eq!(fee, 100);
+    assert_eq!(net, 9_900);
+}
+
+#[test]
+fn test_reentrancy_attack_is_blocked() {
+    // settle -> collect_fee -> malicious token.transfer -> reentrant settle.
+    // The second `settle` must hit the held lock and panic, aborting the tx.
+    let (env, _oracle_id, _admin, payer, _payee, fee_collector) = setup_env();
+    let settlement_id = setup_settlement(&env);
+    let settlement_client = SettlementContractClient::new(&env, &settlement_id);
+
+    let mal_id = env.register(MaliciousToken, ());
+    let mal_client = MaliciousTokenClient::new(&env, &mal_id);
+    mal_client.setup(&settlement_id, &payer, &fee_collector);
+
+    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        settlement_client.settle(
+            &mal_id,
+            &payer,
+            &payer, // payee
+            &fee_collector,
+            &10_000i128,
+            &100u32,
+        );
+    }));
+
+    assert!(result.is_err(), "reentrant settle must be rejected by the guard");
+}
+
+#[test]
+fn test_reentrancy_attack_via_finalize_is_blocked() {
+    // Same attack through the oracle-based entry point: collect_fee invokes the
+    // malicious token, which re-enters settle while finalize_settlement holds
+    // the lock.
+    let (env, oracle_id, _admin, payer, payee, fee_collector) = setup_env();
+    let settlement_id = setup_settlement(&env);
+    let settlement_client = SettlementContractClient::new(&env, &settlement_id);
+
+    let mal_id = env.register(MaliciousToken, ());
+    let mal_client = MaliciousTokenClient::new(&env, &mal_id);
+    mal_client.setup(&settlement_id, &payer, &fee_collector);
+
+    let volume = 1_000_0000i128;
+    let args = make_args(mal_id, volume, payee.clone(), None);
+
+    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        settlement_client.finalize_settlement(&oracle_id, &payer, &fee_collector, &args, &100u32);
+    }));
+
+    assert!(result.is_err(), "reentrant finalize_settlement must be rejected");
+}