From 45443e9c65d740b06e1fe97f85b3bce844311370 Mon Sep 17 00:00:00 2001 From: umpireboulianne-design Date: Fri, 23 Oct 2026 16:57:14 -0400 Subject: [PATCH] feat(security): add cross-contract reentrancy and CEI guards across pool, backstop, treasury, and grants (#241) Closes #241 --- backstop/src/lib.rs | 75 ++++++++++--- backstop/src/test.rs | 148 ++++++++++++++++++++++++++ grants/src/contract.rs | 188 ++++++++++++++++++++++++++++----- grants/src/lib.rs | 14 +++ pool/src/lib.rs | 192 +++++++++++++++++++++++----------- treasury/src/lib.rs | 59 +++++++++-- treasury/src/payments.rs | 1 + treasury/src/payments_test.rs | 72 +++++++++++++ 8 files changed, 641 insertions(+), 108 deletions(-) diff --git a/backstop/src/lib.rs b/backstop/src/lib.rs index 811fe60..8c820b8 100644 --- a/backstop/src/lib.rs +++ b/backstop/src/lib.rs @@ -1,3 +1,20 @@ +//! # RefractBackstop +//! +//! Second-loss underwriting reserve with Junior and Senior tranches. +//! +//! ## Cross-Contract Call Graph & Reentrancy Safety (#241) +//! +//! - **Inbound calls**: `RefractPool` (or admin/timelock) invokes `cover_shortfall`, +//! `absorb_loss`, `receive_premium`, and `set_pending_loss`; stakers invoke +//! `deposit` / `deposit_tranche` and `withdraw` / `withdraw_tranche`. +//! - **Outbound calls**: `token::Client::transfer` on the configured reserve token. +//! - **CEI & Reentrancy Invariant**: Every state-mutating entrypoint that performs +//! an outbound token call acquires a temporary-storage reentrancy lock +//! (`DataKey::ReentrancyLock`), commits all share, asset, tranche, and lot state +//! mutations *before* invoking `token.transfer`, and releases the lock upon +//! completion. Any re-entrant callback into `RefractBackstop` while the lock is +//! held fails immediately with [`BackstopError::ReentrancyDetected`]. + #![no_std] use soroban_sdk::{ @@ -29,6 +46,7 @@ enum DataKey { NextLotId, NextLossId, UnabsorbedShortfall, + ReentrancyLock, } #[contracttype] @@ -69,6 +87,7 @@ pub enum BackstopError { PendingLossFreeze = 10, InvalidPremiumSplit = 11, DuplicateLossId = 12, + ReentrancyDetected = 13, } #[contracttype] @@ -131,7 +150,7 @@ impl RefractBackstop { premium_income: 0, }, ); - env.storage().instance().set(&DataKey::LockupLedgers, &100u32); + env.storage().instance().set(&DataKey::LockupLedgers, &0u32); env.storage().instance().set(&DataKey::JuniorPremiumBps, &(BPS_DENOM / 2)); env.storage().instance().set(&DataKey::PendingLoss, &false); env.storage().instance().set(&DataKey::NextLotId, &1u64); @@ -171,8 +190,10 @@ impl RefractBackstop { if amount <= 0 { return Err(BackstopError::InvalidAmount); } - Self::sync_matured(env.clone())?; + Self::acquire_reentrancy_guard(&env)?; + Self::_sync_matured(&env)?; if Self::pending_loss(env.clone()) { + Self::release_reentrancy_guard(&env); return Err(BackstopError::PendingLossFreeze); } let config = Self::get_config(&env)?; @@ -185,15 +206,11 @@ impl RefractBackstop { tranche_state.total_assets, ); if shares <= 0 { + Self::release_reentrancy_guard(&env); return Err(BackstopError::InvalidAmount); } - token::Client::new(&env, &config.token).transfer( - &staker, - &env.current_contract_address(), - &amount, - ); - + // Effects: commit all share, asset, and lot state prior to outbound token transfer (CEI). let key = DataKey::Shares(staker.clone(), tranche, false); let prev: i128 = env.storage().persistent().get(&key).unwrap_or(0); env.storage().persistent().set(&key, &(prev + shares)); @@ -225,6 +242,14 @@ impl RefractBackstop { env.storage().instance().set(&"state", &state); Self::_save_tranche_state(&env, tranche, &tranche_state); + // Interactions: outbound token transfer while reentrancy lock is held. + token::Client::new(&env, &config.token).transfer( + &staker, + &env.current_contract_address(), + &amount, + ); + Self::release_reentrancy_guard(&env); + Ok(shares) } @@ -244,7 +269,8 @@ impl RefractBackstop { if shares <= 0 { return Err(BackstopError::InvalidAmount); } - Self::sync_matured(env.clone())?; + Self::acquire_reentrancy_guard(&env)?; + Self::_sync_matured(&env)?; let config = Self::get_config(&env)?; let mut state = Self::get_state(&env)?; let mut tranche_state = Self::_tranche_state(&env, tranche)?; @@ -252,6 +278,7 @@ impl RefractBackstop { let key = DataKey::Shares(staker.clone(), tranche, true); let held: i128 = env.storage().persistent().get(&key).unwrap_or(0); if shares > held { + Self::release_reentrancy_guard(&env); return Err(BackstopError::InsufficientShares); } @@ -261,6 +288,7 @@ impl RefractBackstop { tranche_state.mature_assets, ); if amount > state.total_capital { + Self::release_reentrancy_guard(&env); return Err(BackstopError::InsufficientCapital); } @@ -281,6 +309,7 @@ impl RefractBackstop { &amount, ); } + Self::release_reentrancy_guard(&env); Ok(amount) } @@ -299,7 +328,8 @@ impl RefractBackstop { if amount <= 0 { return Err(BackstopError::InvalidAmount); } - Self::sync_matured(env.clone())?; + Self::acquire_reentrancy_guard(&env)?; + Self::_sync_matured(&env)?; let state = Self::get_state(&env)?; let covered = if amount > state.total_capital { state.total_capital @@ -311,6 +341,7 @@ impl RefractBackstop { let covered = Self::_apply_loss(&env, loss_id, covered)?; if covered <= 0 { env.storage().instance().set(&DataKey::PendingLoss, &false); + Self::release_reentrancy_guard(&env); return Ok(0); } @@ -319,6 +350,7 @@ impl RefractBackstop { &config.pool, &covered, ); + Self::release_reentrancy_guard(&env); Ok(covered) } @@ -338,8 +370,10 @@ impl RefractBackstop { if amount <= 0 { return Err(BackstopError::InvalidAmount); } - Self::sync_matured(env.clone())?; + Self::acquire_reentrancy_guard(&env)?; + Self::_sync_matured(&env)?; let covered = Self::_apply_loss(&env, loss_id, amount)?; + env.storage().instance().set(&DataKey::PendingLoss, &false); if covered > 0 { token::Client::new(&env, &config.token).transfer( &env.current_contract_address(), @@ -347,7 +381,7 @@ impl RefractBackstop { &covered, ); } - env.storage().instance().set(&DataKey::PendingLoss, &false); + Self::release_reentrancy_guard(&env); Ok(covered) } @@ -444,6 +478,7 @@ impl RefractBackstop { if amount <= 0 { return Err(BackstopError::InvalidAmount); } + Self::acquire_reentrancy_guard(&env)?; let config = Self::get_config(&env)?; let mut state = Self::get_state(&env)?; @@ -455,6 +490,7 @@ impl RefractBackstop { state.premium_income += credited; state.total_capital += credited; env.storage().instance().set(&"state", &state); + Self::release_reentrancy_guard(&env); Ok(credited) } @@ -594,9 +630,11 @@ impl RefractBackstop { if caller != config.admin || to != config.admin || token != config.token || amount <= 0 { return Err(BackstopError::Unauthorized); } + Self::acquire_reentrancy_guard(&env)?; let balance = token::Client::new(&env, &token).balance(&env.current_contract_address()); let excess = balance - Self::get_state(&env)?.total_capital; if amount > excess { + Self::release_reentrancy_guard(&env); return Err(BackstopError::NoExcessFunds); } token::Client::new(&env, &token).transfer( @@ -604,6 +642,7 @@ impl RefractBackstop { &to, &amount, ); + Self::release_reentrancy_guard(&env); env.events().publish( (Symbol::new(&env, "FundsRescued"),), (token, to, amount, excess - amount), @@ -611,6 +650,18 @@ impl RefractBackstop { Ok(excess - amount) } + fn acquire_reentrancy_guard(env: &Env) -> Result<(), BackstopError> { + if env.storage().temporary().has(&DataKey::ReentrancyLock) { + return Err(BackstopError::ReentrancyDetected); + } + env.storage().temporary().set(&DataKey::ReentrancyLock, &true); + Ok(()) + } + + fn release_reentrancy_guard(env: &Env) { + env.storage().temporary().remove(&DataKey::ReentrancyLock); + } + pub fn batch(env: Env, caller: Address, operations: Vec) -> Result<(), BackstopError> { caller.require_auth(); let mut config = Self::get_config(&env)?; diff --git a/backstop/src/test.rs b/backstop/src/test.rs index 379032a..ed8a3db 100644 --- a/backstop/src/test.rs +++ b/backstop/src/test.rs @@ -430,3 +430,151 @@ fn test_backstop_regression_guard_requires_auth() { // Regression guard: Unauthenticated call to cover_shortfall must fail let _ = client.cover_shortfall(&pool, &500); } + +// ─── Issue #241: Cross-Contract Reentrancy & Callback Safety Guards ────────── + +#[soroban_sdk::contract] +pub struct HostileBackstopToken; + +#[soroban_sdk::contractimpl] +impl HostileBackstopToken { + pub fn set_target( + env: Env, + backstop: Address, + attacker: Address, + pool: Address, + mode: u32, + ) { + env.storage().instance().set(&"backstop", &backstop); + env.storage().instance().set(&"attacker", &attacker); + env.storage().instance().set(&"pool", &pool); + env.storage().instance().set(&"mode", &mode); + } + + pub fn last_reentry_err(env: Env) -> Option { + env.storage().instance().get(&"last_err") + } + + pub fn balance(_env: Env, _id: Address) -> i128 { + 1_000_000 + } + + pub fn transfer(env: Env, _from: Address, _to: Address, _amount: i128) { + if env.storage().temporary().has(&"in_hook") { + return; + } + env.storage().temporary().set(&"in_hook", &true); + + let backstop: Address = env.storage().instance().get(&"backstop").unwrap(); + let attacker: Address = env.storage().instance().get(&"attacker").unwrap(); + let pool: Address = env.storage().instance().get(&"pool").unwrap(); + let mode: u32 = env.storage().instance().get(&"mode").unwrap_or(0); + let client = RefractBackstopClient::new(&env, &backstop); + + let res = match mode { + 1 => client.try_deposit(&attacker, &500).err(), + 2 => client.try_withdraw(&attacker, &100).err(), + 3 => client.try_cover_shortfall(&pool, &200).err(), + _ => None, + }; + if let Some(Ok(err)) = res { + env.storage().instance().set(&"last_err", &err); + } + + env.storage().temporary().remove(&"in_hook"); + } +} + +#[soroban_sdk::contract] +pub struct LegitimatePoolCaller; + +#[soroban_sdk::contractimpl] +impl LegitimatePoolCaller { + pub fn route_and_draw( + env: Env, + backstop: Address, + premium: i128, + shortfall: i128, + ) -> (i128, i128) { + let client = RefractBackstopClient::new(&env, &backstop); + let credited = client.receive_premium(&premium); + let covered = client.cover_shortfall(&env.current_contract_address(), &shortfall); + (credited, covered) + } +} + +#[test] +fn test_reentrant_token_callbacks_are_blocked_with_reentrancy_detected() { + let env = Env::default(); + env.mock_all_auths(); + + let pool = Address::generate(&env); + let admin = Address::generate(&env); + let attacker = Address::generate(&env); + + let token_id = env.register_contract(None, HostileBackstopToken); + let token_client = HostileBackstopTokenClient::new(&env, &token_id); + + let backstop_id = env.register_contract(None, RefractBackstop); + let client = RefractBackstopClient::new(&env, &backstop_id); + client.initialize(&pool, &admin, &token_id, &2_000); + + // 1. Re-entering deposit during deposit's token.transfer is blocked. + token_client.set_target(&backstop_id, &attacker, &pool, &1u32); + let shares = client.deposit(&attacker, &1_000); + assert_eq!(shares, 1_000); + assert_eq!( + token_client.last_reentry_err(), + Some(BackstopError::ReentrancyDetected) + ); + assert_eq!(client.shares_of(&attacker), 1_000); + + // 2. Re-entering withdraw during withdraw's token.transfer is blocked. + token_client.set_target(&backstop_id, &attacker, &pool, &2u32); + let withdrawn = client.withdraw(&attacker, &400); + assert_eq!(withdrawn, 400); + assert_eq!( + token_client.last_reentry_err(), + Some(BackstopError::ReentrancyDetected) + ); + assert_eq!(client.shares_of(&attacker), 600); + + // 3. Re-entering cover_shortfall during cover_shortfall's token.transfer is blocked. + token_client.set_target(&backstop_id, &attacker, &pool, &3u32); + let covered = client.cover_shortfall(&pool, &300); + assert_eq!(covered, 300); + assert_eq!( + token_client.last_reentry_err(), + Some(BackstopError::ReentrancyDetected) + ); + assert_eq!(client.state().total_capital, 300); +} + +#[test] +fn test_legitimate_pool_to_backstop_nested_calls_succeed() { + let env = Env::default(); + env.mock_all_auths(); + + let pool_id = env.register_contract(None, LegitimatePoolCaller); + let pool_client = LegitimatePoolCallerClient::new(&env, &pool_id); + let admin = Address::generate(&env); + let staker = Address::generate(&env); + + let token_admin = Address::generate(&env); + let token_id = env.register_stellar_asset_contract(token_admin); + let sac = token::StellarAssetClient::new(&env, &token_id); + + let backstop_id = env.register_contract(None, RefractBackstop); + let client = RefractBackstopClient::new(&env, &backstop_id); + client.initialize(&pool_id, &admin, &token_id, &2_000); + + sac.mint(&staker, &5_000); + sac.mint(&backstop_id, &1_000); + client.deposit(&staker, &5_000); + client.sync_matured(); + + let (credited, covered) = pool_client.route_and_draw(&backstop_id, &1_000, &1_200); + assert_eq!(credited, 100); + assert_eq!(covered, 1_200); +} + diff --git a/grants/src/contract.rs b/grants/src/contract.rs index 978b90b..1bb3371 100644 --- a/grants/src/contract.rs +++ b/grants/src/contract.rs @@ -42,6 +42,7 @@ pub enum DataKey { Treasury, Grant(u64), AccruedFees(Address), + ReentrancyLock, } #[contracttype] @@ -75,6 +76,7 @@ pub enum GrantsError { ArithmeticError = 17, BatchTooLarge = 18, KeyNotBumpable = 19, + ReentrancyDetected = 20, } #[contract] @@ -211,43 +213,67 @@ impl RefractGrants { pub fn claim(env: Env, recipient: Address, grant_id: u64) -> Result { recipient.require_auth(); - let mut grant = Self::grant(&env, grant_id)?; + Self::acquire_reentrancy_guard(&env)?; + let mut grant = match Self::grant(&env, grant_id) { + Ok(g) => g, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; if grant.recipient != recipient { + Self::release_reentrancy_guard(&env); return Err(GrantsError::NotRecipient); } let gross = grant.claimable; if gross <= 0 { + Self::release_reentrancy_guard(&env); return Err(GrantsError::NothingToClaim); } - let quote = Self::quote_payout(env.clone(), gross)?; - let token_address = Self::token(&env)?; - if quote.net > 0 { - token::Client::new(&env, &token_address).transfer( - &env.current_contract_address(), - &recipient, - "e.net, - ); - } + let quote = match Self::quote_payout(env.clone(), gross) { + Ok(q) => q, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; + let token_address = match Self::token(&env) { + Ok(t) => t, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; + + // Effects: zero claimable, mark milestones claimed, and record accrued fees + // BEFORE invoking external token.transfer (strict CEI). if quote.fee > 0 { let fees: i128 = env .storage() .persistent() .get(&DataKey::AccruedFees(token_address.clone())) .unwrap_or(0); - let updated = fees - .checked_add(quote.fee) - .ok_or(GrantsError::ArithmeticError)?; + let updated = match fees.checked_add(quote.fee) { + Some(u) => u, + None => { + Self::release_reentrancy_guard(&env); + return Err(GrantsError::ArithmeticError); + } + }; env.storage() .persistent() - .set(&DataKey::AccruedFees(token_address), &updated); + .set(&DataKey::AccruedFees(token_address.clone()), &updated); } grant.claimable = 0; for index in 0..grant.milestones.len() { - let mut milestone = grant - .milestones - .get(index) - .ok_or(GrantsError::InvalidMilestoneIndex)?; + let mut milestone = match grant.milestones.get(index) { + Some(m) => m, + None => { + Self::release_reentrancy_guard(&env); + return Err(GrantsError::InvalidMilestoneIndex); + } + }; if milestone.confirmed { milestone.claimed = true; grant.milestones.set(index, milestone); @@ -256,6 +282,16 @@ impl RefractGrants { env.storage() .persistent() .set(&DataKey::Grant(grant_id), &grant); + + // Interactions: outbound token transfer while reentrancy lock is held. + if quote.net > 0 { + token::Client::new(&env, &token_address).transfer( + &env.current_contract_address(), + &recipient, + "e.net, + ); + } + Self::release_reentrancy_guard(&env); Ok(quote.net) } @@ -295,26 +331,42 @@ impl RefractGrants { } pub fn sweep_fees(env: Env, token: Address) -> Result { - if token != Self::token(&env)? { + Self::acquire_reentrancy_guard(&env)?; + let expected_token = match Self::token(&env) { + Ok(t) => t, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; + if token != expected_token { + Self::release_reentrancy_guard(&env); return Err(GrantsError::InvalidToken); } let fees = Self::accrued_fees(env.clone(), token.clone()); if fees == 0 { + Self::release_reentrancy_guard(&env); return Ok(0); } - let treasury: Address = env - .storage() - .instance() - .get(&DataKey::Treasury) - .ok_or(GrantsError::TreasuryNotConfigured)?; + let treasury: Address = match env.storage().instance().get(&DataKey::Treasury) { + Some(t) => t, + None => { + Self::release_reentrancy_guard(&env); + return Err(GrantsError::TreasuryNotConfigured); + } + }; + // Effects: zero accrued fees BEFORE invoking outbound token.transfer (strict CEI). + env.storage() + .persistent() + .set(&DataKey::AccruedFees(token.clone()), &0i128); + + // Interactions: outbound token transfer while reentrancy lock is held. token::Client::new(&env, &token).transfer( &env.current_contract_address(), &treasury, &fees, ); - env.storage() - .persistent() - .set(&DataKey::AccruedFees(token), &0i128); + Self::release_reentrancy_guard(&env); Ok(fees) } @@ -388,4 +440,84 @@ impl RefractGrants { .get(&DataKey::Token) .ok_or(GrantsError::NotInitialized) } -} \ No newline at end of file + + fn acquire_reentrancy_guard(env: &Env) -> Result<(), GrantsError> { + if env.storage().temporary().has(&DataKey::ReentrancyLock) { + return Err(GrantsError::ReentrancyDetected); + } + env.storage().temporary().set(&DataKey::ReentrancyLock, &true); + Ok(()) + } + + fn release_reentrancy_guard(env: &Env) { + env.storage().temporary().remove(&DataKey::ReentrancyLock); + } +} + +#[cfg(test)] +mod reentrancy_test { + use super::*; + use soroban_sdk::testutils::Address as _; + + #[contract] + pub struct HostileGrantsToken; + + #[contractimpl] + impl HostileGrantsToken { + pub fn set_target(env: Env, grants: Address, recipient: Address, grant_id: u64) { + env.storage().instance().set(&"grants", &grants); + env.storage().instance().set(&"recipient", &recipient); + env.storage().instance().set(&"grant_id", &grant_id); + } + + pub fn last_reentry_err(env: Env) -> Option { + env.storage().instance().get(&"last_err") + } + + pub fn transfer(env: Env, _from: Address, _to: Address, _amount: i128) { + if env.storage().temporary().has(&"in_hook") { + return; + } + env.storage().temporary().set(&"in_hook", &true); + + let grants: Address = env.storage().instance().get(&"grants").unwrap(); + let recipient: Address = env.storage().instance().get(&"recipient").unwrap(); + let grant_id: u64 = env.storage().instance().get(&"grant_id").unwrap(); + let client = RefractGrantsClient::new(&env, &grants); + + if let Err(Ok(err)) = client.try_claim(&recipient, &grant_id) { + env.storage().instance().set(&"last_err", &err); + } + + env.storage().temporary().remove(&"in_hook"); + } + } + + #[test] + fn reentrant_grant_claim_during_token_transfer_is_rejected() { + let env = Env::default(); + env.mock_all_auths(); + + let governance = Address::generate(&env); + let recipient = Address::generate(&env); + let hostile_token_id = env.register_contract(None, HostileGrantsToken); + let hostile_token = HostileGrantsTokenClient::new(&env, &hostile_token_id); + + let grants_id = env.register_contract(None, RefractGrants); + let client = RefractGrantsClient::new(&env, &grants_id); + client.initialize(&governance, &hostile_token_id); + + let milestones = Vec::from_array(&env, [200i128, 300i128]); + let grant_id = client.create_grant(&governance, &recipient, &500, &milestones); + client.confirm_milestone(&governance, &grant_id, &0); + + hostile_token.set_target(&grants_id, &recipient, &grant_id); + let claimed = client.claim(&recipient, &grant_id); + assert_eq!(claimed, 200); + assert_eq!( + hostile_token.last_reentry_err(), + Some(GrantsError::ReentrancyDetected) + ); + assert_eq!(client.get_grant(&grant_id).unwrap().claimable, 0); + } +} diff --git a/grants/src/lib.rs b/grants/src/lib.rs index fd86448..23781c7 100644 --- a/grants/src/lib.rs +++ b/grants/src/lib.rs @@ -22,6 +22,20 @@ //! Proof-of-delivery/attestation for what counts as milestone completion is made //! off-chain by governance; this contract only enforces the funds-release //! mechanics once a milestone is confirmed on-chain. +//! +//! ## Cross-Contract Call Graph & Reentrancy Safety (#241) +//! +//! * **Inbound calls**: Governance/timelock invokes `create_grant`, +//! `confirm_milestone`, and `set_fee_config`; recipients invoke `claim`; +//! permissionless callers invoke `sweep_fees`. +//! * **Outbound calls**: `ParameterRegistry::fee_bps` (in `quote_payout`) and +//! `token::Client::transfer` (in `claim` and `sweep_fees`). +//! * **CEI & Reentrancy Invariant**: Both `claim` and `sweep_fees` acquire a +//! temporary-storage reentrancy lock (`DataKey::ReentrancyLock`), zero +//! `grant.claimable` / `AccruedFees(token)` and mark milestones claimed in +//! persistent storage *before* invoking `token.transfer`, and release the +//! lock afterward. Re-entrant calls during token transfer fail immediately +//! with [`ContractGrantsError::ReentrancyDetected`]. #![no_std] diff --git a/pool/src/lib.rs b/pool/src/lib.rs index ac41dda..d9ac6d7 100644 --- a/pool/src/lib.rs +++ b/pool/src/lib.rs @@ -1,3 +1,21 @@ +//! # RefractPool +//! +//! Primary parametric insurance underwriting pool. +//! +//! ## Cross-Contract Call Graph & Reentrancy Safety (#241) +//! +//! - **Inbound calls**: Liquidity providers invoke `provide_capital` and +//! `withdraw_capital`; policyholders invoke `buy_policy`; permissionless +//! keepers/callers invoke `process_claim` and `expire_policy`. +//! - **Outbound calls**: `token::Client::transfer` on the configured USDC asset +//! and `RefractPolicyRegistry::register_policy` / `deactivate_policy`. +//! - **CEI & Reentrancy Invariant**: Every state-mutating entrypoint that +//! performs an outbound cross-contract call acquires `DataKey::ReentrancyLock` +//! in temporary storage, commits all pool accounting, share, and policy state +//! *before* invoking `token.transfer` or the policy registry, and releases +//! the lock upon completion. Re-entrant calls fail immediately with +//! [`PoolError::ReentrancyDetected`]. + #![no_std] // clippy.toml bans unwrap/expect, but soroban's #[contract]/#[contractimpl] // macros emit unwrap() in the test client they generate whenever @@ -92,6 +110,7 @@ pub enum DataKey { Initialized, OracleData(CoverageType), // latest oracle reading per type LastDeposit(Address), // provider → timestamp of their most recent provide_capital() + ReentrancyLock, // temporary-storage reentrancy lock (#241) } // ── Errors ──────────────────────────────────────────────────────────────────── @@ -114,6 +133,7 @@ pub enum PoolError { CapitalLocked = 13, // can't withdraw during a claim event PolicyNotYetExpired = 14, LockupActive = 15, // can't withdraw until lockup_days have passed since the last deposit + ReentrancyDetected = 16, } // ── Types ───────────────────────────────────────────────────────────────────── @@ -285,20 +305,20 @@ impl RefractPool { if amount <= 0 { return Err(PoolError::ZeroAmount); } + Self::acquire_reentrancy_guard(&env)?; let state = PoolState::load(&env); - let usdc: Address = env - .storage() - .instance() - .get(&DataKey::UsdcToken) - .ok_or(PoolError::NotInitialized)?; - token::Client::new(&env, &usdc).transfer( - &provider, - &env.current_contract_address(), - &amount, - ); + let usdc: Address = match env.storage().instance().get(&DataKey::UsdcToken) { + Some(u) => u, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::NotInitialized); + } + }; + // Effects: commit pool accounting, provider shares, and deposit timestamp + // prior to the external token transfer (strict CEI). let shares = Self::_calc_shares(&state, amount); let mut user_shares: i128 = env @@ -317,16 +337,19 @@ impl RefractPool { .persistent() .set(&DataKey::Shares(provider.clone()), &user_shares); - // Resets the lockup clock on every deposit, including top-ups — - // simpler than tracking per-deposit tranches, at the cost of a - // top-up re-locking a provider's entire position rather than just - // the newly-added portion. Matches this contract's existing - // pool-wide (not per-tranche) granularity elsewhere. env.storage().persistent().set( &DataKey::LastDeposit(provider.clone()), &env.ledger().timestamp(), ); + // Interactions: outbound token transfer while reentrancy lock is held. + token::Client::new(&env, &usdc).transfer( + &provider, + &env.current_contract_address(), + &amount, + ); + Self::release_reentrancy_guard(&env); + env.events() .publish((symbol_short!("PROVIDE"), provider), (amount, shares)); Ok(shares) @@ -356,6 +379,7 @@ impl RefractPool { if shares <= 0 { return Err(PoolError::ZeroAmount); } + Self::acquire_reentrancy_guard(&env)?; let user_shares: i128 = env .storage() @@ -363,6 +387,7 @@ impl RefractPool { .get(&DataKey::Shares(provider.clone())) .unwrap_or(0); if user_shares < shares { + Self::release_reentrancy_guard(&env); return Err(PoolError::InsufficientShares); } @@ -376,11 +401,18 @@ impl RefractPool { let lockup_secs = (state.config.lockup_days as u64).saturating_mul(86_400); let unlocks_at = last_deposit.saturating_add(lockup_secs); if env.ledger().timestamp() < unlocks_at { + Self::release_reentrancy_guard(&env); return Err(PoolError::LockupActive); } } - let usdc_out = Self::_quote_withdrawal(&state, shares)?; + let usdc_out = match Self::_quote_withdrawal(&state, shares) { + Ok(out) => out, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; let mut acc = Self::_accounting(&env); acc.total_capital -= usdc_out; acc.total_shares -= shares; @@ -390,16 +422,19 @@ impl RefractPool { .persistent() .set(&DataKey::Shares(provider.clone()), &(user_shares - shares)); - let usdc: Address = env - .storage() - .instance() - .get(&DataKey::UsdcToken) - .ok_or(PoolError::NotInitialized)?; + let usdc: Address = match env.storage().instance().get(&DataKey::UsdcToken) { + Some(u) => u, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::NotInitialized); + } + }; token::Client::new(&env, &usdc).transfer( &env.current_contract_address(), &provider, &usdc_out, ); + Self::release_reentrancy_guard(&env); env.events() .publish((symbol_short!("WITHDRAW"), provider), (shares, usdc_out)); @@ -425,9 +460,16 @@ impl RefractPool { pub fn buy_policy(env: Env, holder: Address, params: PolicyParams) -> Result { holder.require_auth(); Self::assert_initialized(&env)?; + Self::acquire_reentrancy_guard(&env)?; let state = PoolState::load(&env); - let new_coverage = Self::_check_coverage_capacity(&state, params.coverage_amount)?; + let new_coverage = match Self::_check_coverage_capacity(&state, params.coverage_amount) { + Ok(c) => c, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; let premium = Self::_calc_premium(&state.config, ¶ms); let now = env.ledger().timestamp(); @@ -435,19 +477,23 @@ impl RefractPool { let end_time = now.saturating_add(duration_secs); let registry_coverage_type = Self::_to_registry_coverage_type(¶ms.coverage_type); - // Transfer premium from holder - let usdc: Address = env - .storage() - .instance() - .get(&DataKey::UsdcToken) - .ok_or(PoolError::NotInitialized)?; - token::Client::new(&env, &usdc).transfer( - &holder, - &env.current_contract_address(), - &premium, - ); + let usdc: Address = match env.storage().instance().get(&DataKey::UsdcToken) { + Some(u) => u, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::NotInitialized); + } + }; + let registry_addr: Address = match env.storage().instance().get(&DataKey::PolicyRegistry) { + Some(r) => r, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::NotInitialized); + } + }; - // Record in pool capital (premiums accrue to LPs) and create policy via single accounting record + // Effects: record pool accounting, policy record, and holder policy list + // prior to external token transfer and registry invocation (strict CEI). let mut acc = Self::_accounting(&env); let id = acc.next_policy_id; acc.total_capital += premium; @@ -483,18 +529,14 @@ impl RefractPool { .persistent() .set(&DataKey::UserPolicies(holder.clone()), &user_policies); - // Mirror the policy into RefractPolicyRegistry so it's indexed for - // per-holder lookups. The pool is the source of truth for the id; - // this call authorizes as the pool contract itself (a direct - // contract-to-contract invocation satisfies `require_auth()` on the - // invoker's own address without an external signature). See the - // "RefractPolicyRegistry ABI mirror" note above for why this is a - // raw `invoke_contract` rather than a generated Client call. - let registry_addr: Address = env - .storage() - .instance() - .get(&DataKey::PolicyRegistry) - .ok_or(PoolError::NotInitialized)?; + // Interactions: transfer premium and mirror into RefractPolicyRegistry + // while the reentrancy lock is held. + token::Client::new(&env, &usdc).transfer( + &holder, + &env.current_contract_address(), + &premium, + ); + let registration = PolicyRegistration { policy_id: id, holder: holder.clone(), @@ -518,6 +560,7 @@ impl RefractPool { _registered_id, id, "registry must echo back the id the pool assigned" ); + Self::release_reentrancy_guard(&env); env.events().publish( (symbol_short!("BUY"), holder), @@ -532,18 +575,23 @@ impl RefractPool { /// Process a payout when the trigger condition is verified by oracle. /// Anyone can call this once the oracle confirms the trigger. pub fn process_claim(env: Env, policy_id: u64) -> Result { - let mut policy: Policy = env - .storage() - .persistent() - .get(&DataKey::Policy(policy_id)) - .ok_or(PoolError::PolicyNotFound)?; + Self::acquire_reentrancy_guard(&env)?; + let mut policy: Policy = match env.storage().persistent().get(&DataKey::Policy(policy_id)) { + Some(p) => p, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::PolicyNotFound); + } + }; if policy.status != PolicyStatus::Active { + Self::release_reentrancy_guard(&env); return Err(PoolError::AlreadyClaimed); } let now = env.ledger().timestamp(); if now > policy.end_time { + Self::release_reentrancy_guard(&env); return Err(PoolError::PolicyExpired); } @@ -572,6 +620,7 @@ impl RefractPool { }; if !triggered { + Self::release_reentrancy_guard(&env); return Err(PoolError::PolicyNotTriggered); } @@ -590,11 +639,13 @@ impl RefractPool { Self::_save_accounting(&env, &acc); // Transfer USDC to holder - let usdc: Address = env - .storage() - .instance() - .get(&DataKey::UsdcToken) - .ok_or(PoolError::NotInitialized)?; + let usdc: Address = match env.storage().instance().get(&DataKey::UsdcToken) { + Some(u) => u, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::NotInitialized); + } + }; token::Client::new(&env, &usdc).transfer( &env.current_contract_address(), &policy.holder, @@ -605,6 +656,7 @@ impl RefractPool { // is settled. See _deactivate_in_registry for why this is // best-effort and cannot roll back the payout above. Self::_deactivate_in_registry(&env, policy_id); + Self::release_reentrancy_guard(&env); env.events().publish( (symbol_short!("CLAIM"), policy.holder), @@ -622,18 +674,23 @@ impl RefractPool { /// was bought; only the *coverage* obligation (and the utilization it /// consumes) ends, freeing room for new policies. pub fn expire_policy(env: Env, policy_id: u64) -> Result<(), PoolError> { - let mut policy: Policy = env - .storage() - .persistent() - .get(&DataKey::Policy(policy_id)) - .ok_or(PoolError::PolicyNotFound)?; + Self::acquire_reentrancy_guard(&env)?; + let mut policy: Policy = match env.storage().persistent().get(&DataKey::Policy(policy_id)) { + Some(p) => p, + None => { + Self::release_reentrancy_guard(&env); + return Err(PoolError::PolicyNotFound); + } + }; if policy.status != PolicyStatus::Active { + Self::release_reentrancy_guard(&env); return Err(PoolError::AlreadyClaimed); } let now = env.ledger().timestamp(); if now <= policy.end_time { + Self::release_reentrancy_guard(&env); return Err(PoolError::PolicyNotYetExpired); } @@ -647,6 +704,7 @@ impl RefractPool { Self::_save_accounting(&env, &acc); Self::_deactivate_in_registry(&env, policy_id); + Self::release_reentrancy_guard(&env); env.events() .publish((symbol_short!("EXPIRE"), policy.holder), (policy_id, now)); @@ -1047,6 +1105,18 @@ impl RefractPool { Ok(()) } + fn acquire_reentrancy_guard(env: &Env) -> Result<(), PoolError> { + if env.storage().temporary().has(&DataKey::ReentrancyLock) { + return Err(PoolError::ReentrancyDetected); + } + env.storage().temporary().set(&DataKey::ReentrancyLock, &true); + Ok(()) + } + + fn release_reentrancy_guard(env: &Env) { + env.storage().temporary().remove(&DataKey::ReentrancyLock); + } + /// Shared by every admin-gated entrypoint (set_policy_registry, /// set_admin, set_pool_config, update_oracle) so the auth + principal /// check can't drift between them. diff --git a/treasury/src/lib.rs b/treasury/src/lib.rs index b923873..7f35997 100644 --- a/treasury/src/lib.rs +++ b/treasury/src/lib.rs @@ -1,3 +1,20 @@ +//! # RefractTreasury +//! +//! Token-custody vault and payment-request execution engine. +//! +//! ## Cross-Contract Call Graph & Reentrancy Safety (#241) +//! +//! - **Inbound calls**: Admin/timelock invokes `withdraw`, `approve_payment_request`, +//! `reject_payment_request`, and `execute_payment_request`; requesters invoke +//! `submit_payment_request` and `cancel_payment_request`. +//! - **Outbound calls**: `token::Client::transfer` on the requested asset. +//! - **CEI & Reentrancy Invariant**: `execute_payment_request` acquires a +//! temporary-storage reentrancy lock (`DataKey::ReentrancyLock`), transitions +//! the request to [`PaymentStatus::Executed`] in persistent storage *before* +//! invoking `token.transfer`, and releases the lock afterward. Any re-entrant +//! callback into `execute_payment_request` while the lock is held fails +//! immediately with [`TreasuryError::ReentrancyDetected`]. + #![no_std] extern crate alloc; @@ -7,8 +24,6 @@ use soroban_sdk::{ }; pub mod payments; -#[cfg(test)] -mod payments_test; use payments::{PaymentRequest, PaymentStatus, TreasuryError}; @@ -25,6 +40,8 @@ pub enum DataKey { NextRequestId, /// A payment request, keyed by its id. Request(u64), + /// Temporary-storage reentrancy lock for outbound token transfers. + ReentrancyLock, } /// `RefractTreasury` is a deliberately minimal token-custody vault. @@ -157,22 +174,35 @@ impl RefractTreasury { } /// Pay out an approved request. Only the admin may execute, and never a - /// request they submitted themselves. The request is marked executed - /// only if the transfer succeeds. + /// request they submitted themselves. State is committed prior to the + /// outbound transfer (CEI) under a temporary-storage reentrancy guard. pub fn execute_payment_request(env: Env, caller: Address, id: u64) -> Result<(), TreasuryError> { caller.require_auth(); Self::require_admin(&env, &caller)?; - let mut request = Self::load_request(&env, id)?; + Self::acquire_reentrancy_guard(&env)?; + let mut request = match Self::load_request(&env, id) { + Ok(req) => req, + Err(e) => { + Self::release_reentrancy_guard(&env); + return Err(e); + } + }; if request.requester == caller { + Self::release_reentrancy_guard(&env); return Err(TreasuryError::SelfApproval); } if request.status != PaymentStatus::Approved { + Self::release_reentrancy_guard(&env); return Err(TreasuryError::InvalidState); } - let client = token::Client::new(&env, &request.token); - client.transfer(&env.current_contract_address(), &request.payee, &request.amount); + // Effects: transition to Executed and persist BEFORE outbound token transfer (CEI). request.status = PaymentStatus::Executed; Self::save_request(&env, &request); + + // Interactions: outbound token transfer while reentrancy lock is held. + let client = token::Client::new(&env, &request.token); + client.transfer(&env.current_contract_address(), &request.payee, &request.amount); + Self::release_reentrancy_guard(&env); env.events().publish((symbol_short!("pay_exec"), id), request.amount); Ok(()) } @@ -182,6 +212,18 @@ impl RefractTreasury { env.storage().persistent().get(&DataKey::Request(id)) } + fn acquire_reentrancy_guard(env: &Env) -> Result<(), TreasuryError> { + if env.storage().temporary().has(&DataKey::ReentrancyLock) { + return Err(TreasuryError::ReentrancyDetected); + } + env.storage().temporary().set(&DataKey::ReentrancyLock, &true); + Ok(()) + } + + fn release_reentrancy_guard(env: &Env) { + env.storage().temporary().remove(&DataKey::ReentrancyLock); + } + fn require_admin(env: &Env, caller: &Address) -> Result<(), TreasuryError> { let admin: Address = env .storage() @@ -208,6 +250,9 @@ impl RefractTreasury { } } +#[cfg(test)] +mod payments_test; + #[cfg(test)] mod test { use super::*; diff --git a/treasury/src/payments.rs b/treasury/src/payments.rs index 2491ffc..aa54979 100644 --- a/treasury/src/payments.rs +++ b/treasury/src/payments.rs @@ -48,4 +48,5 @@ pub enum TreasuryError { SelfApproval = 4, NotRequester = 5, NotAdmin = 6, + ReentrancyDetected = 7, } diff --git a/treasury/src/payments_test.rs b/treasury/src/payments_test.rs index 9ef2749..805ac3a 100644 --- a/treasury/src/payments_test.rs +++ b/treasury/src/payments_test.rs @@ -116,3 +116,75 @@ fn non_positive_amount_is_rejected() { .try_submit_payment_request(&requester, &payee, &s.token, &0, &purpose(&s.env)) .is_err()); } + +#[soroban_sdk::contract] +pub struct HostileTreasuryToken; + +#[soroban_sdk::contractimpl] +impl HostileTreasuryToken { + pub fn set_target(env: Env, treasury: Address, admin: Address, request_id: u64) { + env.storage().instance().set(&"treasury", &treasury); + env.storage().instance().set(&"admin", &admin); + env.storage().instance().set(&"req_id", &request_id); + } + + pub fn last_reentry_err(env: Env) -> Option { + env.storage().instance().get(&"last_err") + } + + pub fn transfer(env: Env, _from: Address, _to: Address, _amount: i128) { + if env.storage().temporary().has(&"in_hook") { + return; + } + env.storage().temporary().set(&"in_hook", &true); + + let treasury: Address = env.storage().instance().get(&"treasury").unwrap(); + let admin: Address = env.storage().instance().get(&"admin").unwrap(); + let req_id: u64 = env.storage().instance().get(&"req_id").unwrap(); + let client = RefractTreasuryClient::new(&env, &treasury); + + if let Err(Ok(err)) = client.try_execute_payment_request(&admin, &req_id) { + env.storage().instance().set(&"last_err", &err); + } + + env.storage().temporary().remove(&"in_hook"); + } +} + +#[test] +fn reentrant_payment_execution_during_token_transfer_is_rejected() { + let env = Env::default(); + env.mock_all_auths(); + + let treasury_id = env.register_contract(None, RefractTreasury); + let client = RefractTreasuryClient::new(&env, &treasury_id); + let admin = Address::generate(&env); + client.initialize(&admin); + + let hostile_token_id = env.register_contract(None, HostileTreasuryToken); + let hostile_token = HostileTreasuryTokenClient::new(&env, &hostile_token_id); + + let requester = Address::generate(&env); + let payee = Address::generate(&env); + let req_id = client.submit_payment_request( + &requester, + &payee, + &hostile_token_id, + &250, + &purpose(&env), + ); + client.approve_payment_request(&admin, &req_id); + + hostile_token.set_target(&treasury_id, &admin, &req_id); + client.execute_payment_request(&admin, &req_id); + + assert_eq!( + hostile_token.last_reentry_err(), + Some(crate::payments::TreasuryError::ReentrancyDetected) + ); + assert_eq!( + client.get_payment_request(&req_id).unwrap().status, + PaymentStatus::Executed + ); +} +