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;