From ee1dbdadd781c5390aefce130e9466f8fc386f2d Mon Sep 17 00:00:00 2001 From: definurse Date: Mon, 24 Aug 2026 20:10:57 +0100 Subject: [PATCH] payment-router: guard value paths against double-release and reentrancy The settlement paths were relying on ordering alone to prevent a second payout against the same escrow, and one path did not have the ordering right. This closes both halves. Checks-effects-interactions: - open_escrow now writes the escrow row before calling the token, rather than transferring first and recording afterwards. - claim and close_escrow already committed before transferring; the reasoning is now written down at each site so it survives future edits. - release_partial calls no token at all - it makes funds claimable and the payee pulls separately, which keeps an untrusted destination off the settlement path entirely. Idempotence: - A second close_escrow returns AlreadySettled and refunds nothing, where previously it reported the generic EscrowClosed. - A claim against a settled escrow with nothing left to pull returns AlreadySettled, kept distinct from NothingToClaim - the latter is worth retrying once more has been released, the former never is. Reentrancy: - A guard over the value-moving entry points takes a marker in temporary storage for the duration of the call; a second entry gets ReentrantCall. Temporary is the right storage class: the marker only means anything inside the transaction that set it, so the host drops it at end of ledger and no rent is paid on it. It is kept out of DataKey because it is not escrow state. The guard is not redundant with the ordering. open_escrow must write the escrow row before the deposit arrives, so for the duration of the token call the ledger holds a claim on funds that have not landed - a hostile token calling back into close_escrow on that row would be refunded its total out of other escrows' deposits. The guard is what closes that. Tests, in a new invariants module so the behavioural suite stays as it is: - Conservation asserted directly - the router's balance always equals the sum of outstanding escrow claims - across a full lifecycle, across a dispute, and with several escrows sharing the contract balance. - claimed <= released and released + disputed <= total checked on every step of those walks. - Double-close and double-claim assert both the error and that no value moved, which is the part that actually matters. - Two adversarial cases run against a real token contract that re-enters the router from inside transfer: one during a claim, one during a deposit while a second escrow is open. Neither moves funds. docs/payment-router-value-safety.md writes down the invariants, the storage-class reasoning, and what a hostile token can and cannot do. Also gitignores test_snapshots/, which cargo test generates into each crate and which is not currently ignored - it is 30k lines of generated JSON that lands in the working tree on any test run. Closes #58 --- .gitignore | 1 + docs/payment-router-value-safety.md | 81 +++++++ payment-router/src/invariants.rs | 359 ++++++++++++++++++++++++++++ payment-router/src/lib.rs | 85 ++++++- 4 files changed, 517 insertions(+), 9 deletions(-) create mode 100644 docs/payment-router-value-safety.md create mode 100644 payment-router/src/invariants.rs diff --git a/.gitignore b/.gitignore index 8b6ef78..d4ee993 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ target/ .DS_Store .soroban/ *.wasm +test_snapshots/ diff --git a/docs/payment-router-value-safety.md b/docs/payment-router-value-safety.md new file mode 100644 index 0000000..d37bbf1 --- /dev/null +++ b/docs/payment-router-value-safety.md @@ -0,0 +1,81 @@ +# payment-router: value safety + +The invariants the settlement paths hold, why each one holds, and what an +untrusted token contract can and cannot do to them. Written to be read before +the implementation, and asserted directly in `payment-router/src/invariants.rs`. + +## The invariants + +1. **Conservation.** The router's token balance equals the sum of what every + escrow still owes: released-but-unpulled, plus unreleased remainder, plus + anything frozen. No path creates or destroys value. +2. **An escrow never pays out more than it took in.** `claimed <= released` and + `released + disputed <= total`, on every path, including after a dispute is + resolved and after the escrow is closed. +3. **Settled paths are idempotent.** A second `close_escrow`, or a `claim` + against a settled escrow with nothing left to pull, returns `AlreadySettled` + and moves no funds. + +## Checks-effects-interactions + +Every path that moves value validates first, commits the state transition +second, and calls the token last. + +| Path | Effect committed | Then | +| --- | --- | --- | +| `claim` | `claimed += amount` | `transfer` out | +| `close_escrow` | `closed = true` | refund `transfer` | +| `open_escrow` | escrow row written | deposit `transfer` in | +| `release_partial` | `released += amount` | no token call at all | + +`release_partial` is the important one: it makes funds *claimable* and calls no +token. The payee pulls separately. A push-based release would put an untrusted +destination on the settlement path and let a broken payee fail everyone's +settlement; pull keeps that blast radius to the payee alone. + +`open_escrow` is the one place where ordering alone is not sufficient. Writing +the escrow row before the deposit lands means that, for the duration of the +token call, the ledger records a claim on funds that have not arrived. A +hostile token could call back into `close_escrow` on that row and be refunded +its `total` out of *other escrows'* deposits. That is why the deposit also runs +inside the reentrancy guard, and why the guard is not merely belt-and-braces. + +## The reentrancy guard + +`enter`/`leave` take a marker in **temporary** storage under the `in_fligh` +symbol, held for the duration of a value-moving call. A second entry while one +is in flight returns `ReentrantCall`. + +Temporary storage is the right class here: the marker is meaningful only within +the transaction that sets it, so the host drops it at the end of the ledger and +nobody pays rent on it. It is kept out of `DataKey` for the same reason — it is +not escrow state. On a panic the transaction reverts, which unwinds the marker +along with everything else, so a failed call cannot leave the contract wedged. + +## The adversarial token + +The payer chooses the token address when opening an escrow. The router must +therefore treat the token as hostile code that runs on every `transfer`. The +router's entire dependency on the token interface is `transfer`, which keeps +the surface small. + +What a hostile token can do: + +- **Fail the transfer.** Its own settlement reverts. No other escrow is + affected, because each escrow names its own token. +- **Call back during `transfer`.** It gets `ReentrantCall`, which fails the + outer call too. The reverted transaction takes the state write with it, so + the escrow is left exactly as it was. +- **Lie about balances.** It can report anything; the router never reads the + token's balance, only its own accounting, so a lie changes nothing. + +What it cannot do: + +- **Pull twice.** `claimed` is advanced before the transfer, so a re-entrant + `claim` finds nothing claimable even if the guard were absent. +- **Reach another escrow's funds.** Covered by + `test_a_token_that_calls_back_during_a_deposit_cannot_drain_another_escrow`. +- **Strand a third party.** Its damage is confined to escrows that named it. + +Both callback cases are exercised against a real re-entering token contract in +`invariants.rs` rather than argued for in prose. diff --git a/payment-router/src/invariants.rs b/payment-router/src/invariants.rs new file mode 100644 index 0000000..14a648c --- /dev/null +++ b/payment-router/src/invariants.rs @@ -0,0 +1,359 @@ +#![cfg(test)] +//! Value-safety invariants for the settlement paths. +//! +//! These are the properties an auditor checks first, so they are asserted +//! directly rather than being implied by the behavioural tests in `test`: +//! +//! 1. **Conservation.** The router's token balance always equals the sum of +//! what every escrow still owes — nothing is created, nothing evaporates. +//! 2. **No escrow pays out more than it took in.** `claimed <= released <= +//! total` on every path, including after a dispute and after a close. +//! 3. **Settled paths are idempotent.** A second `close_escrow` or a second +//! `claim` on a settled escrow returns an error and moves no funds. +//! 4. **A hostile token cannot re-enter a value path.** The token contract is +//! supplied by the payer and is therefore untrusted code on the callback. + +use super::*; +use soroban_sdk::{ + contract as test_contract, contractimpl as test_contractimpl, + testutils::Address as _, + token::{Client as TokenClient, StellarAssetClient}, + Env, +}; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +struct Fixture { + env: Env, + router: Address, + token: Address, + payer: Address, + payee: Address, +} + +fn setup(funding: i128) -> Fixture { + let env = Env::default(); + env.mock_all_auths(); + + let arbiter = Address::generate(&env); + let router = env.register(PaymentRouter, ()); + PaymentRouterClient::new(&env, &router).initialize(&arbiter); + + let issuer = Address::generate(&env); + let token = env.register_stellar_asset_contract_v2(issuer).address(); + + let payer = Address::generate(&env); + let payee = Address::generate(&env); + StellarAssetClient::new(&env, &token).mint(&payer, &funding); + + Fixture { + env, + router, + token, + payer, + payee, + } +} + +impl Fixture { + fn client(&self) -> PaymentRouterClient<'_> { + PaymentRouterClient::new(&self.env, &self.router) + } + + fn token(&self) -> TokenClient<'_> { + TokenClient::new(&self.env, &self.token) + } + + fn open(&self, total: i128) -> u64 { + self.client() + .open_escrow(&self.payer, &self.payee, &self.token, &total) + } + + /// What the router still owes on one escrow: the released-but-unpulled + /// portion, plus whatever remains escrowed or frozen against it. + fn outstanding(&self, id: u64) -> i128 { + let e = self.client().get_escrow(&id); + let unreleased = if e.closed { + 0 + } else { + e.total - e.released - e.disputed + }; + (e.released - e.claimed) + unreleased + if e.closed { 0 } else { e.disputed } + } + + /// Conservation: the contract holds exactly what it still owes, and no + /// escrow has paid out more than was ever deposited into it. + fn assert_conserved(&self, ids: &[u64]) { + let owed: i128 = ids.iter().map(|id| self.outstanding(*id)).sum(); + assert_eq!( + self.token().balance(&self.router), + owed, + "router balance must equal the sum of outstanding escrow claims" + ); + + for id in ids { + let e = self.client().get_escrow(id); + assert!( + e.claimed <= e.released, + "escrow {id} paid out unreleased funds" + ); + assert!( + e.released + e.disputed <= e.total, + "escrow {id} released more than it took in" + ); + assert!(e.claimed >= 0 && e.released >= 0 && e.disputed >= 0); + } + } +} + +fn raised(e: Error) -> soroban_sdk::Error { + soroban_sdk::Error::from_contract_error(e as u32) +} + +// --------------------------------------------------------------------------- +// Conservation +// --------------------------------------------------------------------------- + +#[test] +fn test_balances_are_conserved_across_a_full_escrow_lifecycle() { + let f = setup(10_000); + let id = f.open(10_000); + f.assert_conserved(&[id]); + + f.client().release_partial(&id, &2_500); + f.assert_conserved(&[id]); + + f.client().claim(&id); + f.assert_conserved(&[id]); + + f.client().release_partial(&id, &1_500); + f.assert_conserved(&[id]); + + f.client().close_escrow(&id); + f.assert_conserved(&[id]); + + f.client().claim(&id); + f.assert_conserved(&[id]); + + // Every unit deposited ends up with exactly one party. + assert_eq!(f.token().balance(&f.payee), 4_000); + assert_eq!(f.token().balance(&f.payer), 6_000); + assert_eq!(f.token().balance(&f.router), 0); +} + +#[test] +fn test_balances_are_conserved_across_a_dispute() { + let f = setup(10_000); + let id = f.open(10_000); + + f.client().release_partial(&id, &1_000); + f.client().dispute(&id, &4_000); + f.assert_conserved(&[id]); + + f.client().resolve_dispute(&id, &true); + f.assert_conserved(&[id]); + + f.client().claim(&id); + f.assert_conserved(&[id]); + assert_eq!(f.token().balance(&f.payee), 5_000); +} + +#[test] +fn test_conservation_holds_with_several_escrows_sharing_the_contract_balance() { + let f = setup(30_000); + let a = f.open(10_000); + let b = f.open(12_000); + let c = f.open(8_000); + + f.client().release_partial(&a, &4_000); + f.client().release_partial(&b, &12_000); + f.client().claim(&b); + f.client().close_escrow(&c); + + f.assert_conserved(&[a, b, c]); + assert_eq!(f.token().balance(&f.router), 10_000); +} + +// --------------------------------------------------------------------------- +// Idempotence of the settled paths +// --------------------------------------------------------------------------- + +#[test] +fn test_a_second_close_returns_already_settled_and_refunds_nothing() { + let f = setup(10_000); + let id = f.open(10_000); + + f.client().release_partial(&id, &2_000); + f.client().close_escrow(&id); + + let payer_after_first = f.token().balance(&f.payer); + assert_eq!(payer_after_first, 8_000); + + assert_eq!( + f.client().try_close_escrow(&id), + Err(Ok(raised(Error::AlreadySettled))) + ); + // The decisive assertion: the error is not merely returned, no value moved. + assert_eq!(f.token().balance(&f.payer), payer_after_first); + assert_eq!(f.token().balance(&f.router), 2_000); + f.assert_conserved(&[id]); +} + +#[test] +fn test_a_second_claim_on_a_settled_escrow_returns_already_settled() { + let f = setup(10_000); + let id = f.open(10_000); + + f.client().release_partial(&id, &3_000); + f.client().close_escrow(&id); + assert_eq!(f.client().claim(&id), 3_000); + + assert_eq!( + f.client().try_claim(&id), + Err(Ok(raised(Error::AlreadySettled))) + ); + assert_eq!(f.token().balance(&f.payee), 3_000); + assert_eq!(f.token().balance(&f.router), 0); +} + +#[test] +fn test_an_open_escrow_with_nothing_released_still_reports_nothing_to_claim() { + let f = setup(10_000); + let id = f.open(10_000); + + // Distinct from AlreadySettled: this one is worth retrying later. + assert_eq!( + f.client().try_claim(&id), + Err(Ok(raised(Error::NothingToClaim))) + ); +} + +#[test] +fn test_releases_can_never_sum_past_the_deposit() { + let f = setup(10_000); + let id = f.open(10_000); + + f.client().release_partial(&id, &6_000); + assert_eq!( + f.client().try_release_partial(&id, &4_001), + Err(Ok(raised(Error::InsufficientEscrow))) + ); + f.client().release_partial(&id, &4_000); + assert_eq!( + f.client().try_release_partial(&id, &1), + Err(Ok(raised(Error::InsufficientEscrow))) + ); + f.assert_conserved(&[id]); +} + +// --------------------------------------------------------------------------- +// Adversarial token +// --------------------------------------------------------------------------- + +/// A token whose `transfer` calls back into the router. +/// +/// The payer chooses the token address when opening an escrow, so this is code +/// the router must assume is hostile. Only `transfer` is implemented because +/// that is the whole of the router's dependency on the token interface. +#[test_contract] +pub struct HostileToken; + +#[contracttype] +#[derive(Clone)] +pub enum Hostile { + Target, + Escrow, + Armed, +} + +#[test_contractimpl] +impl HostileToken { + /// Point the callback at a router and escrow. Disarmed until `arm` is + /// called, so the deposit that funds the escrow can complete normally. + pub fn arm(env: Env, router: Address, id: u64) { + env.storage().instance().set(&Hostile::Target, &router); + env.storage().instance().set(&Hostile::Escrow, &id); + env.storage().instance().set(&Hostile::Armed, &true); + } + + pub fn transfer(env: Env, _from: Address, _to: Address, _amount: i128) { + let armed: bool = env + .storage() + .instance() + .get(&Hostile::Armed) + .unwrap_or(false); + if !armed { + return; + } + // Fire once, so the callback cannot recurse forever on its own. + env.storage().instance().set(&Hostile::Armed, &false); + + let router: Address = env.storage().instance().get(&Hostile::Target).unwrap(); + let id: u64 = env.storage().instance().get(&Hostile::Escrow).unwrap(); + // The double-release attempt: pull again from inside the first pull. + PaymentRouterClient::new(&env, &router).claim(&id); + } +} + +#[test] +fn test_a_token_that_calls_back_during_a_claim_cannot_pull_twice() { + let env = Env::default(); + env.mock_all_auths(); + + let arbiter = Address::generate(&env); + let router = env.register(PaymentRouter, ()); + let client = PaymentRouterClient::new(&env, &router); + client.initialize(&arbiter); + + let token = env.register(HostileToken, ()); + let payer = Address::generate(&env); + let payee = Address::generate(&env); + + // The deposit runs while the token is disarmed. + let id = client.open_escrow(&payer, &payee, &token, &10_000); + client.release_partial(&id, &4_000); + + HostileTokenClient::new(&env, &token).arm(&router, &id); + + // The payout re-enters. It is refused, which fails the outer call too — + // a hostile token can break its own settlement, but it cannot double-pull. + assert!(client.try_claim(&id).is_err()); + + // Nothing was recorded: the reverted transaction took the write with it. + let escrow = client.get_escrow(&id); + assert_eq!(escrow.claimed, 0); + assert_eq!(escrow.released, 4_000); + assert_eq!(client.claimable(&id), 4_000); +} + +#[test] +fn test_a_token_that_calls_back_during_a_deposit_cannot_drain_another_escrow() { + let env = Env::default(); + env.mock_all_auths(); + + let arbiter = Address::generate(&env); + let router = env.register(PaymentRouter, ()); + let client = PaymentRouterClient::new(&env, &router); + client.initialize(&arbiter); + + let token = env.register(HostileToken, ()); + let payer = Address::generate(&env); + let payee = Address::generate(&env); + + let victim = client.open_escrow(&payer, &payee, &token, &5_000); + client.release_partial(&victim, &5_000); + + // Arm before opening the second escrow: the callback now fires from inside + // the deposit, at the moment the new escrow row exists but is unfunded. + HostileTokenClient::new(&env, &token).arm(&router, &victim); + + assert!(client + .try_open_escrow(&payer, &payee, &token, &1_000) + .is_err()); + + // The victim escrow is untouched. + assert_eq!(client.get_escrow(&victim).claimed, 0); + assert_eq!(client.claimable(&victim), 5_000); +} diff --git a/payment-router/src/lib.rs b/payment-router/src/lib.rs index 7d9630a..03d6035 100644 --- a/payment-router/src/lib.rs +++ b/payment-router/src/lib.rs @@ -8,8 +8,8 @@ //! payee pulls what has been released on their own schedule. use soroban_sdk::{ - contract, contracterror, contractevent, contractimpl, contracttype, panic_with_error, token, - Address, Env, IntoVal, + contract, contracterror, contractevent, contractimpl, contracttype, panic_with_error, + symbol_short, token, Address, Env, IntoVal, Symbol, }; /// Persistent entries are bumped to ~30 days, renewed once inside ~15 days. @@ -43,6 +43,40 @@ pub enum Error { DisputeAlreadyOpen = 10, NotTheArbiter = 11, SamePayerAndPayee = 12, + /// This path has already settled. Returned instead of moving value a + /// second time, so a repeated close or a repeated claim against a settled + /// escrow is an inert no-op rather than a second payout. + AlreadySettled = 13, + /// A value-moving entry point was re-entered while an earlier call was + /// still in flight. This is the shape a hostile token takes when it calls + /// back into the router from inside `transfer`. + ReentrantCall = 14, +} + +/// Marker for a value-moving call that is currently in flight. +/// +/// Deliberately kept out of `DataKey` and in *temporary* storage: it exists +/// only for the duration of the transaction that sets it, so it is dropped by +/// the host at the end of the ledger rather than being paid for forever. On a +/// panic the whole transaction reverts, which unwinds the marker with it. +const IN_FLIGHT: Symbol = symbol_short!("in_fligh"); + +/// Take the in-flight marker, or reject the call as re-entrant. +/// +/// Checks-effects-interactions already makes a second entry unprofitable — +/// state is committed before any token call — but the two together are what +/// makes the property hold under a token contract we do not control, rather +/// than only under one that behaves. +fn enter(env: &Env) { + if env.storage().temporary().has(&IN_FLIGHT) { + panic_with_error!(env, Error::ReentrantCall); + } + env.storage().temporary().set(&IN_FLIGHT, &true); +} + +/// Release the in-flight marker on the normal exit path. +fn leave(env: &Env) { + env.storage().temporary().remove(&IN_FLIGHT); } #[contracttype] @@ -174,16 +208,21 @@ impl PaymentRouter { panic_with_error!(&env, Error::SamePayerAndPayee); } payer.require_auth_for_args((payee.clone(), token_address.clone(), total).into_val(&env)); - - let contract = env.current_contract_address(); - token::Client::new(&env, &token_address).transfer(&payer, &contract, &total); - + enter(&env); + + // Effects first: the escrow row is written before the token is asked + // to move anything. The deposit is the one call where that ordering is + // not sufficient on its own — a row recorded at `total` is briefly a + // claim on funds that have not arrived — which is why the deposit runs + // inside the in-flight guard as well. A token that calls back here + // finds `ReentrantCall`, not an escrow it can close for a refund out + // of somebody else's deposit. let id = Self::next_id(&env); let escrow = Escrow { id, payer: payer.clone(), payee: payee.clone(), - token: token_address, + token: token_address.clone(), total, released: 0, claimed: 0, @@ -194,6 +233,10 @@ impl PaymentRouter { Self::save(&env, &escrow); Self::bump_instance(&env); + let contract = env.current_contract_address(); + token::Client::new(&env, &token_address).transfer(&payer, &contract, &total); + leave(&env); + EscrowOpened { id, payer, @@ -256,12 +299,23 @@ impl PaymentRouter { pub fn claim(env: Env, id: u64) -> i128 { let mut escrow = Self::load(&env, id); escrow.payee.require_auth(); + enter(&env); let amount = escrow.claimable(); if amount <= 0 { + // A closed escrow with nothing left to pull is finished, not + // merely empty for the moment. Saying so distinctly is what makes + // a repeated `claim` a diagnosable no-op instead of an error the + // caller might reasonably retry. + if escrow.closed { + panic_with_error!(&env, Error::AlreadySettled); + } panic_with_error!(&env, Error::NothingToClaim); } + // Effects: the pull is recorded before the token is called, so a + // callback that re-enters sees `claimed` already advanced and has + // nothing left to draw. The guard above closes the same door twice. escrow.claimed += amount; Self::save(&env, &escrow); @@ -270,6 +324,7 @@ impl PaymentRouter { &escrow.payee, &amount, ); + leave(&env); Claimed { id, @@ -352,13 +407,21 @@ impl PaymentRouter { /// claimable by the payee afterwards. pub fn close_escrow(env: Env, id: u64) { let mut escrow = Self::load(&env, id); - Self::assert_open(&env, &escrow); + // A second close is the double-release case wearing a different hat: + // `releasable()` would still read as the remainder if the flag had not + // been written first. It has been, so this returns and moves nothing. + if escrow.closed { + panic_with_error!(&env, Error::AlreadySettled); + } if escrow.disputed > 0 { panic_with_error!(&env, Error::DisputeAlreadyOpen); } escrow.payer.require_auth_for_args((id,).into_val(&env)); + enter(&env); + // Effects: closed is committed and the remainder is zeroed out by that + // commit before the refund leaves, so the refund cannot be taken twice. let refund = escrow.releasable(); escrow.closed = true; Self::save(&env, &escrow); @@ -370,6 +433,7 @@ impl PaymentRouter { &refund, ); } + leave(&env); EscrowClosed { id, @@ -401,7 +465,7 @@ impl PaymentRouter { } pub fn version(_env: Env) -> u32 { - 2 + 3 } // ----------------------------------------------------------------------- @@ -449,3 +513,6 @@ impl PaymentRouter { #[cfg(test)] mod test; + +#[cfg(test)] +mod invariants;