payment-router: guard value paths against double-release and reentrancy

[definurse13]

Closes #58

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, and asserts the result rather than arguing for it.

The bug

open_escrow transferred the deposit before recording the escrow:

token::Client::new(&env, &token_address).transfer(&payer, &contract, &total);
let id = Self::next_id(&env);
// ...escrow row written afterwards

The payer chooses the token address, so transfer is untrusted code running before any state exists. Reordered so the escrow row is committed first.

Checks-effects-interactions

Path	Effect committed	Then
claim	claimed += amount	transfer out
close_escrow	closed = true	refund transfer
open_escrow	escrow row written	deposit transfer in (reordered)
release_partial	released += amount	no token call at all

claim and close_escrow already had this right; the reasoning is now written down at each site so it survives future edits. release_partial is the one worth noting — it makes funds claimable and calls no token, so an untrusted destination is never on the settlement path.

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. Collapsing them would tell an integrator to retry forever.

Reentrancy guard

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.

Per the storage-TTL review bar: temporary is the right class because 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. On a panic the transaction reverts, which unwinds the marker — a failed call cannot leave the contract wedged.

The guard is not redundant with the ordering. This is the part worth reviewing closely. 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. Ordering alone does not close that; the guard does. There is a test for exactly this.

Tests

In a new invariants.rs module, so the existing behavioural suite is untouched:

• Conservation asserted directly — the router's balance always equals the sum of outstanding escrow claims — across a full lifecycle, across a dispute, and with three escrows sharing the contract balance.
• claimed <= released and released + disputed <= total checked at 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, and the reverted transaction takes the state write with it.

docs/payment-router-value-safety.md writes down the invariants, the storage-class reasoning, and a table of what a hostile token can and cannot do.

Acceptance criteria

• Checks-effects-interactions applied to all value paths
• Double-release returns an error and moves no funds
• Balance-conservation invariant asserted in tests
• Adversarial token-contract case considered and documented

One drive-by, easy to drop

cargo test generates test_snapshots/ into every crate — about 30k lines of JSON that is not currently gitignored and lands in the working tree on any test run. Added one .gitignore line. Happy to pull it out into its own PR if you would rather keep this one purely about the security fix.

Verification

cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace, and cargo build --release --target wasm32v1-none all pass.

This PR and #59 are conflict-free in either merge order. I merged them both ways locally against main: zero conflicts each way, both orders produce an identical tree, and the merged result passes all four CI gates. Whichever of the two you merge first, the other needs no rebase.