diff --git a/README.md b/README.md index 3cc8ec3..085812e 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,27 @@ The escrow contract now enforces a minimal on-chain state machine instead of pla Reviewer-focused contract notes and the formal threat model live in [docs/escrow/README.md](/home/christopher/drips_projects/Talenttrust-Contracts/docs/escrow/README.md). +## Cross-contract call hardening + +The escrow contract includes explicit guards for external token-contract assumptions and failures: + +- A configured token contract is required before guarded transfers. +- External call target validation rejects self-referential addresses. +- A reentrancy lock is acquired during transfer attempts. +- External token call failures are mapped to typed `EscrowError` values. +- Non-positive transfer amounts are rejected before any external call. + +Primary implementation points: + +- `contracts/escrow/src/lib.rs`: + - `set_token_contract` + - `get_token_contract` + - `guarded_external_transfer` +- `contracts/escrow/src/cross_contract.rs`: + - `safe_token_transfer` + - `validate_external_address` + - `acquire_call_lock` / `release_call_lock` + ## Protocol governance The escrow contract supports guarded protocol parameter updates for live validation logic: diff --git a/contracts/escrow/src/cross_contract.rs b/contracts/escrow/src/cross_contract.rs new file mode 100644 index 0000000..459325f --- /dev/null +++ b/contracts/escrow/src/cross_contract.rs @@ -0,0 +1,220 @@ +//! # Cross-Contract Call Hardening +//! +//! This module provides validated, guarded wrappers around every external +//! (cross-contract) call made by the TalentTrust Escrow contract. +//! +//! ## Threat model +//! +//! | Threat | Mitigation | +//! |------------------------------|-------------------------------------------------| +//! | Reentrancy attack | [`CallLock`] stored in instance storage before | +//! | | any external call; cleared after return | +//! | Self-referential call | [`validate_external_address`] rejects `addr == | +//! | | env.current_contract_address()` | +//! | Silent transfer failure | `try_transfer` result is checked; returns | +//! | | [`EscrowError::ExternalCallFailed`] on error | +//! | Unregistered token contract | [`get_required_token_contract`] returns | +//! | | [`EscrowError::TokenContractNotSet`] if absent | +//! | Zero-amount transfer | Guard validates `amount > 0` before call | +//! | Balance drift | Pre/post balance comparison via | +//! | | [`verify_balance_received`] | +//! +//! ## Usage +//! +//! ```text +//! // Inside release_milestone: +//! let token = get_required_token_contract(&env)?; +//! safe_token_transfer(&env, &token, &from, &to, amount)?; +//! ``` + +use soroban_sdk::{token, Address, Env}; + +use crate::{DataKey, EscrowError}; + +// ── Reentrancy guard ────────────────────────────────────────────────────────── + +/// Acquire the single reentrancy lock stored in contract instance storage. +/// +/// # Security +/// Sets `DataKey::CallLock = true` before any external call so that a +/// malicious callee cannot re-enter the escrow contract mid-execution and +/// observe or mutate inconsistent state. +/// +/// If the lock is already held (i.e., another external call is in progress +/// on this contract), the function returns +/// [`EscrowError::ReentrancyDetected`]. +/// +/// # Errors +/// - [`EscrowError::ReentrancyDetected`] – lock already acquired. +pub fn acquire_call_lock(env: &Env) -> Result<(), EscrowError> { + let locked: bool = env + .storage() + .instance() + .get(&DataKey::CallLock) + .unwrap_or(false); + + if locked { + return Err(EscrowError::ReentrancyDetected); + } + + env.storage().instance().set(&DataKey::CallLock, &true); + Ok(()) +} + +/// Release the reentrancy lock. +/// +/// Must be called after every successful [`acquire_call_lock`], including +/// the error paths so the contract is never left in a permanently locked state. +/// Because Soroban rolls back all storage writes on panic, a forgotten +/// `release_call_lock` after a panicking path is safe—the lock is cleared by +/// the host rollback—but callers should still release explicitly for clarity. +pub fn release_call_lock(env: &Env) { + env.storage().instance().remove(&DataKey::CallLock); +} + +// ── Address validation ──────────────────────────────────────────────────────── + +/// Validate that `addr` is a safe target for an external call. +/// +/// Guards enforced (in order): +/// 1. **Non-self**: `addr` must not equal `env.current_contract_address()`. +/// Self-calls would cause the escrow contract to manipulate its own state +/// unexpectedly mid-execution. +/// +/// # Errors +/// - [`EscrowError::SelfReferentialAddress`] – `addr` is this contract. +pub fn validate_external_address(env: &Env, addr: &Address) -> Result<(), EscrowError> { + if *addr == env.current_contract_address() { + return Err(EscrowError::SelfReferentialAddress); + } + Ok(()) +} + +// ── Token contract access ───────────────────────────────────────────────────── + +/// Retrieve the registered token contract address, returning an error if +/// none has been set via [`crate::Escrow::set_token_contract`]. +/// +/// # Errors +/// - [`EscrowError::TokenContractNotSet`] – no token contract registered. +pub fn get_required_token_contract(env: &Env) -> Result { + env.storage() + .instance() + .get(&DataKey::TokenContract) + .ok_or(EscrowError::TokenContractNotSet) +} + +// ── Safe token transfer ─────────────────────────────────────────────────────── + +/// Execute a SEP-41 token transfer with all cross-contract call hardening. +/// +/// # Hardening steps (in order) +/// +/// 1. **Amount guard** – `amount` must be strictly positive. +/// 2. **Address guard** – `token_address` must not be `env.current_contract_address()`. +/// 3. **Reentrancy lock** – acquired before the external call. +/// 4. **Try-transfer** – uses `token::Client::try_transfer` rather than the +/// panicking `transfer`, so the error is captured and mapped to +/// [`EscrowError::ExternalCallFailed`] instead of aborting with an opaque +/// host panic. +/// 5. **Reentrancy unlock** – released unconditionally after the call returns. +/// +/// # Arguments +/// +/// | Name | Description | +/// |-----------------|----------------------------------------------------------------| +/// | `token_address` | The SEP-41 compatible token contract to invoke. | +/// | `from` | Source address; must have pre-authorized the transfer. | +/// | `to` | Destination address. | +/// | `amount` | Transfer amount in base units (must be `> 0`). | +/// +/// # Errors +/// +/// | Error | Condition | +/// |------------------------------------|----------------------------------------| +/// | [`EscrowError::AmountMustBePositive`] | `amount <= 0` | +/// | [`EscrowError::SelfReferentialAddress`] | `token_address == self` | +/// | [`EscrowError::ReentrancyDetected`] | Reentrant call detected | +/// | [`EscrowError::ExternalCallFailed`] | Token contract returned an error | +pub fn safe_token_transfer( + env: &Env, + token_address: &Address, + from: &Address, + to: &Address, + amount: i128, +) -> Result<(), EscrowError> { + // Guard 1: amount must be positive + if amount <= 0 { + return Err(EscrowError::AmountMustBePositive); + } + + // Guard 2: validate token contract address is not self + validate_external_address(env, token_address)?; + + // Guard 3: acquire reentrancy lock + acquire_call_lock(env)?; + + // Guard 4: execute transfer using try_transfer for explicit error handling + let token_client = token::Client::new(env, token_address); + let result = token_client + .try_transfer(from, to, &amount) + .map_err(|_| EscrowError::ExternalCallFailed) + .and_then(|inner| inner.map_err(|_| EscrowError::ExternalCallFailed)); + + // Guard 5: release lock unconditionally before returning + release_call_lock(env); + + result +} + +// ── Balance verification ────────────────────────────────────────────────────── + +/// Read the SEP-41 token balance for `addr` from `token_address`. +/// +/// This is a read-only call and does not acquire the reentrancy lock. +/// +/// # Errors +/// - [`EscrowError::SelfReferentialAddress`] – `token_address == self`. +/// - [`EscrowError::ExternalCallFailed`] – balance query failed. +pub fn query_token_balance( + env: &Env, + token_address: &Address, + addr: &Address, +) -> Result { + validate_external_address(env, token_address)?; + + let token_client = token::Client::new(env, token_address); + token_client + .try_balance(addr) + .map_err(|_| EscrowError::ExternalCallFailed) + .and_then(|inner| inner.map_err(|_| EscrowError::ExternalCallFailed)) +} + +/// Verify that the balance of `recipient` increased by exactly `expected_delta` +/// compared to `balance_before`. +/// +/// Call [`query_token_balance`] before the transfer (capturing `balance_before`), +/// execute the transfer, then call this function to confirm the delta. +/// +/// # Errors +/// - All errors from [`query_token_balance`]. +/// - [`EscrowError::TransferVerificationFailed`] – balance delta does not match. +pub fn verify_balance_received( + env: &Env, + token_address: &Address, + recipient: &Address, + balance_before: i128, + expected_delta: i128, +) -> Result<(), EscrowError> { + let balance_after = query_token_balance(env, token_address, recipient)?; + + let actual_delta = balance_after + .checked_sub(balance_before) + .ok_or(EscrowError::ArithmeticOverflow)?; + + if actual_delta != expected_delta { + return Err(EscrowError::TransferVerificationFailed); + } + + Ok(()) +} diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index aa5022e..1aee9d6 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -32,9 +32,11 @@ use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, String, - Symbol, Vec, + Map, Symbol, Vec, }; +pub(crate) mod cross_contract; + /// Persistent storage keys used by the Escrow contract. /// /// Each variant corresponds to a distinct piece of contract state: @@ -63,6 +65,10 @@ pub enum DataKey { GovernanceAdmin, PendingGovernanceAdmin, ProtocolParameters, + /// Registered SEP-41 token contract used for escrow transfers. + TokenContract, + /// Reentrancy lock set during guarded external calls. + CallLock, } /// The lifecycle status of an escrow contract. @@ -230,6 +236,18 @@ pub enum EscrowError { InvalidRating = 22, /// Invalid milestone ID InvalidMilestoneId = 23, + /// Token contract address is not configured + TokenContractNotSet = 24, + /// Cross-contract call failed or returned an error + ExternalCallFailed = 25, + /// Reentrant external call attempt detected + ReentrancyDetected = 26, + /// External address points to this contract + SelfReferentialAddress = 27, + /// Transfer verification failed due to balance delta mismatch + TransferVerificationFailed = 28, + /// Transfer amount must be strictly positive + AmountMustBePositive = 29, } // (ReleaseAuthorization enum moved) @@ -275,6 +293,52 @@ const DEFAULT_MILESTONE_TIMEOUT_SECS: u64 = 7 * 24 * 60 * 60; #[contractimpl] impl Escrow { + /// @notice Register or rotate the SEP-41 token contract used for escrow transfers. + /// @dev Security: the caller must authorize this operation. + /// @param caller Address authorizing token contract configuration. + /// @param token_contract External token contract address to persist. + /// @return bool True when the token contract address is stored. + pub fn set_token_contract(env: Env, caller: Address, token_contract: Address) -> bool { + caller.require_auth(); + + if token_contract == env.current_contract_address() { + panic!("Token contract cannot be escrow contract"); + } + + env.storage() + .instance() + .set(&DataKey::TokenContract, &token_contract); + + true + } + + /// @notice Return the configured SEP-41 token contract address, if present. + /// @return Option
Configured token address or `None`. + pub fn get_token_contract(env: Env) -> Option
{ + env.storage().instance().get(&DataKey::TokenContract) + } + + /// @notice Perform a guarded token transfer using the configured token contract. + /// + /// @dev This helper centralizes external call assumptions and failure mapping: + /// it requires a configured token contract, validates it is non-self, + /// applies a reentrancy lock, and surfaces external failures as + /// `EscrowError` values. + /// @param from Token source address. + /// @param to Token recipient address. + /// @param amount Transfer amount in token base units. + /// @return Result `Ok(true)` on successful guarded transfer. + pub fn guarded_external_transfer( + env: Env, + from: Address, + to: Address, + amount: i128, + ) -> Result { + let token_contract = cross_contract::get_required_token_contract(&env)?; + cross_contract::safe_token_transfer(&env, &token_contract, &from, &to, amount)?; + Ok(true) + } + /// Create a new escrow contract with milestone-based release authorization. /// /// Stores the contract record in persistent storage and returns a numeric diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index b08a865..5bd4f2a 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -14,6 +14,7 @@ pub(crate) const MILESTONE_THREE: i128 = 600_0000000; // ==================== CONTRACT CREATION TESTS ==================== mod timeout_tests; +mod cross_contract_hardening; #[test] fn test_create_contract_success() { diff --git a/contracts/escrow/src/test/cross_contract_hardening.rs b/contracts/escrow/src/test/cross_contract_hardening.rs new file mode 100644 index 0000000..7bbfc0f --- /dev/null +++ b/contracts/escrow/src/test/cross_contract_hardening.rs @@ -0,0 +1,52 @@ +use soroban_sdk::{testutils::Address as _, Address, Env}; + +use crate::{Escrow, EscrowClient, EscrowError}; + +#[test] +fn test_set_and_get_token_contract() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(Escrow, ()); + let client = EscrowClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let token = Address::generate(&env); + + assert!(client.set_token_contract(&admin, &token)); + assert_eq!(client.get_token_contract(), Some(token)); +} + +#[test] +fn test_guarded_transfer_requires_configured_token_contract() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(Escrow, ()); + let client = EscrowClient::new(&env, &contract_id); + + let from = Address::generate(&env); + let to = Address::generate(&env); + + let result = client.try_guarded_external_transfer(&from, &to, &100_i128); + assert_eq!(result, Err(Ok(EscrowError::TokenContractNotSet))); +} + +#[test] +fn test_guarded_transfer_rejects_non_positive_amount() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(Escrow, ()); + let client = EscrowClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let token = Address::generate(&env); + let from = Address::generate(&env); + let to = Address::generate(&env); + + assert!(client.set_token_contract(&admin, &token)); + + let result = client.try_guarded_external_transfer(&from, &to, &0_i128); + assert_eq!(result, Err(Ok(EscrowError::AmountMustBePositive))); +} diff --git a/docs/escrow/contract.md b/docs/escrow/contract.md index ed58629..7cf0912 100644 --- a/docs/escrow/contract.md +++ b/docs/escrow/contract.md @@ -54,6 +54,19 @@ status: ContractStatus – current state - Issues a reputation score for the freelancer after contract completion. - Returns true. +### set_token_contract(env, caller, token_contract) -> bool +- Registers the SEP-41 token contract used by guarded transfer paths. +- Requires `caller` authorization. +- Rejects self-referential token addresses. + +### get_token_contract(env) -> Option
+- Returns the configured token contract address, if set. + +### guarded_external_transfer(env, from, to, amount) -> Result +- Executes token transfer through guarded cross-contract call wrappers. +- Enforces configured token contract presence and positive amount. +- Uses reentrancy lock and explicit external-call failure mapping. + ### hello(env, to) -> Symbol - Simple test function to verify contract interaction. - Returns the same symbol passed in. diff --git a/docs/escrow/security.md b/docs/escrow/security.md index d8fb007..a7bdc52 100644 --- a/docs/escrow/security.md +++ b/docs/escrow/security.md @@ -37,6 +37,37 @@ Mitigation: all critical mutating endpoints check `ensure_not_paused`. 2. Add role separation for `pauser` and `resolver`. 3. Add on-chain event emission for pause state transitions. 4. Add optional time-delayed unpause for high-severity incidents. + +## Cross-Contract External Call Hardening + +The escrow module now uses guarded wrappers for external token-contract calls. + +### Assumptions validated + +- Token contract address must be explicitly configured. +- Token contract address must not equal the escrow contract address. +- Transfer amount must be strictly positive. + +### Runtime guards + +- A reentrancy lock is set before calling into an external token contract. +- The lock is always cleared after external call return. +- External call failures are converted into typed `EscrowError` values instead of silent assumptions. + +### Failure mapping + +- Missing token contract: `EscrowError::TokenContractNotSet` +- Self-referential target: `EscrowError::SelfReferentialAddress` +- Reentrancy attempt: `EscrowError::ReentrancyDetected` +- External call failure: `EscrowError::ExternalCallFailed` +- Invalid amount: `EscrowError::AmountMustBePositive` + +### Test coverage for this area + +- `contracts/escrow/src/test/cross_contract_hardening.rs` + - token contract set/get behavior + - guarded transfer rejection when token contract is missing + - guarded transfer rejection for non-positive amounts # Escrow Security Notes This document summarizes security assumptions and threat handling for escrow storage planning and core flows.