security(high): refund nonce derives from refundableAmount — nonce collision permanently bricks refunds

[pviti]

Severity: High. From the 2026-07-29 cross-repo audit.

The refund nonce is derived from the current refundable balance (contracts/src/RAIL0.sol:480, _refundNonce at :566-572):

_refundNonce(paymentId, _configHash[paymentId], st.refundableAmount)
// = keccak256(abi.encode(_REFUND_NONCE_PREFIX, paymentId, configHash, refundableAmount))

It includes neither the refund amount nor a counter — and refundableAmount is not monotonic: capture increases it (:391).

Failure scenario (verified against the code): authorize 100 → capture(50) (refundable = 50) → refund(50) (token permanently burns nonce N(…, 50); refundable = 0) → capture(50) (refundable = 50 again) → any subsequent refund derives the same N(…, 50) and the token reverts "authorization is used". The payee cannot vary the amount to escape (amount isn't in the nonce) and cannot change the state once capturableAmount is exhausted: that residual is permanently non-refundable, and an open dispute on it can never be resolved by the merchant.

RAIL0.sol:39-41 and README.md:157 assert nonce uniqueness "per partial refund", which only holds for a strictly-decreasing balance. No test exercises a revisited refundableAmount (test_Refund_Partial_Multiple uses 100 → 70 → 0, all distinct).

Suggested fix: include a per-payment refund counter (or counter + amount) in the nonce derivation. Fund-moving change → version bump + coordinated rollout. Until then the gateway can guard the sequence app-side (refuse a refund whose derived nonce was already consumed — authorizationState is even declared in IEIP3009).

[pviti]

Label note — security-improves + perf-regresses. Making the refund nonce collision-free means adding a per-payment refund counter (or counter + amount) to the derivation, which is an extra storage slot written on every refund — real gas on a fund-moving path.

Worth stating plainly: that cost is not a reason to hesitate. The current derivation permanently bricks refunds on a reachable sequence (capture → refund → capture restores the same refundableAmount, so the nonce repeats and the token rejects it forever), and an open dispute on that residual can then never be resolved. A storage slot is a cheap price for that.

[pviti]

Verified — reproduced end to end, and reachable through normal product flows

Ran the reported sequence as a Foundry test. Every assertion holds:

authorize 100
capture(50)   → refundable 50        nonce N = refundNonce(pid, cfg, 50)
refund(50)    → refundable 0         token marks N spent  ✓ authorizationState == true
capture(50)   → refundable 50 again  capturable 0
              → refundNonce(pid, cfg, 50) == N            ✓ SAME nonce
refund(10 | 30 | 50) → all revert "EIP3009: nonce used"   ✓
              → refundable stays 50, permanently unrefundable

Three details the reproduction settles:

• The nonce uses the PRE-refund balance. refund derives it from st.refundableAmount, where st is the memory copy taken by _loadAndVerify — so it is the balance before the draw-down. That is what makes the value revisitable.
• Varying the amount does not help. The amount is not in the derivation, so a different refund amount produces a different signature over the same nonce. The token checks authorizationState[from][nonce] and refuses regardless.
• There is no way out afterwards. By that point capturableAmount == 0, so the payee cannot move refundableAmount to a fresh value. The residual is stranded, and an open dispute on it can never be resolved by a refund.

Reachable without touching the contract directly

The gateway permits the whole sequence: CAPTURABLE_FROM includes partially_captured, and a partial refund leaves the status unchanged — so capture-refund-capture is an ordinary merchant flow, not an adversarial one. This is a bug a merchant can hit by accident on a two-shipment order.

Fidelity of the reproduction

The mock token's rejection matches the real one: FiatTokenV2 tracks _authorizationStates[authorizer][nonce] and reverts on reuse, and authorizationState(address,bytes32) is live on the deployed USDC (queried on Base Sepolia). So the mechanism is not a mock artefact.

Noting one place where this suite's mock is not faithful, since it matters for anyone testing nearby: it uses TRANSFER_WITH_AUTHORIZATION_TYPEHASH for both EIP-3009 functions, while real USDC has two distinct typehashes (see #35). That gap does not affect this issue — the nonce check happens before signature recovery.

On the suggested fix

A per-payment refund counter in the derivation is right and sufficient. Worth stating what it costs: the counter has to live in storage, and PaymentState is currently exactly one 256-bit slot (bool + two uint120 + bool). There is no room, so a counter means either a second slot — ~20,000 gas on the first refund of every payment — or narrowing the balances to make space, which changes the EIP-712 Payment typehash and therefore every client signature.

That trade deserves its own decision, so I have not assumed one. Given #35 also forces a signing-format change, both belong in the same major version.

The interim app-side guard the issue suggests is sound and cheap: the gateway can call authorizationState(payee, derivedNonce) before preparing a refund and refuse with a clear error instead of letting the transaction revert on chain. That needs no contract change and no version bump.

Reproduction test available on request — kept out of main since it pins current (broken) behaviour; it belongs in the fix PR as a regression test.

[pviti]

Analysis of an alternative fix: derive the nonce from a monotonic counter, with no new storage

The suggested fix is a per-payment refund counter, which costs a storage slot — PaymentState is exactly one 256-bit word today, so a counter means a second slot (~20,000 gas on every payment's first refund) or narrowing the balances and rewriting the EIP-712 Payment typehash.

There is a counter already there, derivable from existing state:

settledAmount = p.amount - capturableAmount - refundableAmount

p.amount is immutable and already in the signed struct; both balances are already loaded by _loadAndVerify. No new storage.

Why it restores the invariant

The current design is not arbitrary — the refundable balance is the counter, and it works because a strictly-decreasing sequence never repeats a value. capture is the single operation that breaks that, by putting the balance back to a value already used.

settledAmount cannot be put back:

step	cap	ref	settled
authorize 100	100	0	0
capture(50)	50	50	0 — flat
refund(50)	50	0	50 — rises
capture(50) ← old collision point	0	50	50 — flat
refund(30)	0	20	80 — rises

A capture moves value between the buckets, so it leaves the sum — and therefore the counter — untouched. Only a refund raises it, and it raises it by exactly the refunded amount. So no two refunds can ever share a value.

Verified, not reasoned

Implemented on a scratch branch and run:

• the security(high): refund nonce derives from refundableAmount — nonce collision permanently bricks refunds #36 sequence now succeeds. At the old collision point the derived nonce differs from the first (settled is 50, not 0), and the refund that was previously impossible goes through, draining the residual to 0.
• the counter is monotonic across interleaving. A loop of four capture/refund pairs asserts a capture leaves it flat and a refund raises it, every time.
• the existing suite passes unchanged — 120 tests, once the nonce-derivation call sites in the tests were updated.

Costs, measured

• gas: +598 on refund (204,193 → 204,791), and nothing on any other entrypoint. Two extra subtractions on values already in hand, versus ~20,000 for a storage counter.
• off-chain: no extra call. A signer needs configHash plus the three values; amount is in the Payment it already holds, and both balances come from the same single getPaymentState it already makes today.

What it still breaks

refundNonce(bytes32, bytes32, uint120) keeps its signature but its third parameter changes meaning, so every off-chain deriver must change with it — gateway, CLI, SDKs. Signature-format-breaking either way, so it belongs with #35 in the same major version.

One honest caveat about the name

After a void or release, settledAmount jumps (escrow leaves the buckets without being refunded), so it is not a faithful "total refunded". It is "how much has left the live buckets". That is fine for nonce uniqueness — the only property required is that it never decreases — but the parameter should be named for what it is, which is why the scratch implementation calls it settledAmount and says so in the NatSpec.

Scratch branch discarded; main untouched. Happy to turn this into a real PR against v2.0.0 if the direction is right.