From da69cde33b79c596924fe0dfe5a0aa059a8690ca Mon Sep 17 00:00:00 2001 From: real-venus Date: Thu, 25 Jun 2026 03:01:40 -0700 Subject: [PATCH] fix(settlement): add cross-contract reentrancy guard (#8) Settlement entry points yield control to untrusted contracts at the cross-contract call boundary (oracle get_price_value, token transfer via collect_fee and the net transfer). A malicious token or oracle could re-enter settle/finalize_settlement before the first invocation completes and manipulate intermediate state. - Add DataKey::ReentrancyLock in storage.rs (temporary storage) - Add ReentrancyGuard RAII mutex in reentrancy.rs: acquire-before-call, release on Drop; strict (any re-entry panics with ReentrantCall). Panic path is covered by the host's storage revert (wasm uses panic=abort, so Drop does not run on unwind). - Acquire the guard at the top of settle() and finalize_settlement() - Add SettlementError::ReentrantCall - Tests: malicious-token reentry via settle and via finalize_settlement are both rejected; legitimate single settle still succeeds --- contracts/settlement/src/lib.rs | 13 ++++ contracts/settlement/src/reentrancy.rs | 81 ++++++++++++++++++++ contracts/settlement/src/storage.rs | 12 +++ contracts/settlement/src/test.rs | 101 +++++++++++++++++++++++++ 4 files changed, 207 insertions(+) create mode 100644 contracts/settlement/src/reentrancy.rs create mode 100644 contracts/settlement/src/storage.rs diff --git a/contracts/settlement/src/lib.rs b/contracts/settlement/src/lib.rs index 28da324..f63a700 100644 --- a/contracts/settlement/src/lib.rs +++ b/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); } diff --git a/contracts/settlement/src/reentrancy.rs b/contracts/settlement/src/reentrancy.rs new file mode 100644 index 0000000..dc15161 --- /dev/null +++ b/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); + } +} diff --git a/contracts/settlement/src/storage.rs b/contracts/settlement/src/storage.rs new file mode 100644 index 0000000..53d9641 --- /dev/null +++ b/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, +} diff --git a/contracts/settlement/src/test.rs b/contracts/settlement/src/test.rs index 7d772b6..2e6b634 100644 --- a/contracts/settlement/src/test.rs +++ b/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"); +}