Skip to content

feat(auth-capture/facilitator): trace-level simulation + gas cap on contract-path captureAuthorizer#61

Open
A1igator wants to merge 2 commits into
mainfrom
A1igator/auth-capture-contract-operator-hardening-impl
Open

feat(auth-capture/facilitator): trace-level simulation + gas cap on contract-path captureAuthorizer#61
A1igator wants to merge 2 commits into
mainfrom
A1igator/auth-capture-contract-operator-hardening-impl

Conversation

@A1igator
Copy link
Copy Markdown
Contributor

Summary

Implementation of the contract-path captureAuthorizer hardening that the spec PR (#60) added. Built side-by-side with that PR so we can verify the spec is actually implementable as written.

When extra.captureAuthorizer is a smart contract, the facilitator now:

  • Trace-level simulation: uses eth_simulateV1 (viem simulateCalls) instead of a revert-only readContract, with traceTransfers: true and the gas cap applied to the call.
  • Escrow event check: verifies the trace contains the matching PaymentAuthorized / PaymentCharged event emitted by AUTH_CAPTURE_ESCROW_ADDRESS with the on-chain paymentInfoHash. Added a new computeOnchainPaymentInfoHash helper that mirrors the contract's getHash (the existing computePayerAgnosticPaymentInfoHash produces the wire nonce — different hash).
  • Asset-delta check: reconstructs ERC-20 Transfer deltas for requirements.asset and asserts the signed PaymentInfo:
    • payer pays amount
    • on authorize: receiver / feeReceiver untouched; the residual nets to +amount
    • on charge: receiver + feeReceiver split amount with the implied feeBps ∈ [minFeeBps, maxFeeBps]; nothing else nets non-zero
  • Gas cap: applied to both the simulation call and the broadcast writeContract, so a wrapper that simulates within budget but spikes at execution time still can't drain the facilitator. Exposed as CAPTURE_AUTHORIZER_GAS_LIMIT = 400_000n.

New error reasons

Constant Wire string
ErrCaptureAuthorizerEscrowCallMissing capture_authorizer_escrow_call_missing
ErrCaptureAuthorizerPaymentInfoMismatch capture_authorizer_payment_info_mismatch
ErrCaptureAuthorizerAssetDivergence capture_authorizer_asset_divergence
ErrCaptureAuthorizerGasExceeded capture_authorizer_gas_exceeded

All four are listed in the spec's Error Codes section.

EOA path

Unchanged. Revert-only readContract simulation against the audited escrow. No gas override on writeContract. The new code only kicks in when getCode(captureAuthorizer) returns non-empty bytecode.

Signer feature-detection

simulateCalls isn't declared on FacilitatorEvmSigner in @x402/evm, so we feature-detect at use. A signer that doesn't expose it gets simulation_failed on the contract path — we cannot satisfy the spec's MUST without it.

Test plan

  • pnpm build — all targets compile (ESM + CJS + DTS).
  • pnpm lint — clean.
  • pnpm format — clean.
  • pnpm test — 150 passing (was 141; +9 new contract-path tests).

New tests cover, end-to-end against the public verify / settle surface:

  • Honest passthrough → ok.
  • Missing escrow event → capture_authorizer_escrow_call_missing.
  • Wrong paymentInfoHashcapture_authorizer_payment_info_mismatch.
  • Asset siphon to attacker address on chargecapture_authorizer_asset_divergence.
  • Implied feeBps outside [minFeeBps, maxFeeBps]capture_authorizer_asset_divergence.
  • Gas hog (gasUsed > 400_000n) → capture_authorizer_gas_exceeded.
  • Signer without simulateCallssimulation_failed.
  • gas: 400_000n passed to writeContract on contract path only (EOA path leaves it unset for eth_estimateGas).

Links

Companion spec PR: #60.

…ontract-path captureAuthorizer

Implements the contract-path hardening from the merged auth-capture EVM
spec section "Smart Contract captureAuthorizer". When extra.captureAuthorizer
is a smart contract (getCode returns non-empty bytecode), the facilitator now:

- Routes simulation through `eth_simulateV1` (viem `simulateCalls`) instead
  of a revert-only `readContract`, with `traceTransfers: true` and the gas
  cap applied to the call.
- Verifies the trace contains the matching `PaymentAuthorized` /
  `PaymentCharged` event emitted by AUTH_CAPTURE_ESCROW_ADDRESS with the
  on-chain `paymentInfoHash` (via the new `computeOnchainPaymentInfoHash`
  helper, mirroring the contract's `getHash`).
- Reconstructs ERC-20 Transfer deltas for `requirements.asset` and asserts
  they match the signed PaymentInfo — payer pays `amount`, on `authorize`
  the receiver/feeReceiver are untouched and the residual sums to +amount
  (escrow side), on `charge` receiver + feeReceiver split `amount` with
  the implied `feeBps ∈ [minFeeBps, maxFeeBps]`, no other address nets
  non-zero.
- Applies the gas cap on the broadcast settle tx as well, so a wrapper
  that simulates within budget but spikes at execution time still cannot
  drain the facilitator.

Cap is exposed as `CAPTURE_AUTHORIZER_GAS_LIMIT = 400_000n` so it can be
referenced from tests and tuned per-deployment.

New error reasons (mirroring the spec's "Error Codes" section):

- `capture_authorizer_escrow_call_missing`
- `capture_authorizer_payment_info_mismatch`
- `capture_authorizer_asset_divergence`
- `capture_authorizer_gas_exceeded`

EOA path is unchanged: revert-only `readContract` simulation against the
audited escrow, no gas override.

Tests:
- All 141 existing tests pass.
- 9 new contract-path tests cover: honest passthrough → ok; missing
  escrow event; wrong paymentInfoHash; asset siphon to attacker; fee
  outside [min, max]; gas hog over the cap; signer without simulateCalls
  → simulation_failed; gas: 400_000n passed to writeContract on contract
  path only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vraspar
Copy link
Copy Markdown
Contributor

vraspar commented May 23, 2026

Pulled the branch and read the implementation against the deployed AuthCaptureEscrow.sol. Event ABIs and hash function are byte-correct against the on-chain contract — ESCROW_EVENTS_ABI matches PaymentAuthorized / PaymentCharged exactly, and computeOnchainPaymentInfoHash matches getHash() (chainId + escrow address + inner typehash encoding). Tests cover the right attack matrix.

Three correctness bugs to fix before merge.

Bug 1 (high) — feeReceiver == address(0) false-positive on charge

Per AuthCaptureEscrow.sol:191 and :255 docstrings, _validateFee allows: "should match the paymentInfo.feeReceiver unless that is 0 in which case it can be any address". So a merchant can sign paymentInfo.feeReceiver = 0x0 and the wrapper legitimately passes a non-zero feeReceiver to escrow.

In verifyAssetDeltas (charge path), feeReceiverDelta = deltas.get(zeroAddress) ?? 0n reads 0n. If the simulated trace shows a real fee transfer to some non-zero address, that delta lands in the "other addresses" loop and trips if (delta !== 0n) return ErrCaptureAuthorizerAssetDivergence. The check rejects an on-chain-valid configuration.

Two ways out:

  • Reject paymentInfo.feeReceiver == 0 at verification step 12 entirely (probably cleaner — matches x402r's "merchant explicitly picks fee recipient" model).
  • Special-case the trace check: when paymentInfo.feeReceiver == 0, accept any single non-payer / non-receiver address as the fee bucket and run the [minFeeBps, maxFeeBps] check against that address's delta.

Bug 2 (medium) — authorize-path asset-delta check is tautological

In verifyAssetDeltas for authorize, after enforcing payerDelta === -amount, receiverDelta === 0n, feeReceiverDelta === 0n, the final check is otherSum === amount.

ERC-20 Transfer events conserve mass per token in a single tx (modulo mints/burns, which require special privileges these wrappers won't have). So sum(all deltas) === 0, which means otherSum === -(payerDelta + receiverDelta + feeReceiverDelta) === amount always, given the first three checks pass. The fourth check can never fail.

In practice the ERC-3009/Permit2 signature semantics save us — the to is locked to the canonical token collector, so the wrapper can't redirect funds even if the trace check would let it. But the check isn't doing what its presence suggests it's doing, and that's load-bearing in the wrong place: if collectorData semantics ever evolve (e.g. a future collector that delivers to a configurable destination), this check is what's supposed to catch a redirect — and it can't.

Replace with an explicit allowed-recipient enumeration. The right recipient set on authorize is {payer, getTokenStore(operator)} plus zero-delta tolerance for intermediates:

```ts
// Derived once at module scope or per-call
const tokenStore = predictTokenStore(paymentInfo.operator);
// In the loop:
const allowed = new Set([payerKey, receiverKey, feeReceiverKey, tokenStore.toLowerCase()]);
for (const [addr, delta] of deltas) {
if (allowed.has(addr)) continue;
if (delta !== 0n) return ErrCaptureAuthorizerAssetDivergence;
}
// Then assert tokenStore delta is +amount.
```

The TokenStore address derivation is in AuthCaptureEscrow.sol:371 — deterministic CREATE2 from (implementation, salt=bytes20(operator), deployer=escrow). Add a TS mirror in nonce.ts or alongside computeOnchainPaymentInfoHash.

Bug 3 (low) — `feeReceiver == receiver` double-count

If a merchant sets `paymentInfo.receiver == paymentInfo.feeReceiver`, the `deltas` Map has one entry; both `receiverDelta` and `feeReceiverDelta` read the same value. The check `receiverDelta + feeReceiverDelta === amount` becomes `2*delta === amount` and fails the honest case.

Unlikely configuration but not prohibited by escrow. Fix: when `receiver == feeReceiver`, check the merged delta equals `amount` and the feeBps range still applies to whatever portion is implied. Or reject this configuration upstream.

Smaller things

`AUTH_CAPTURE_ESCROW_ADDRESS` in `computeOnchainPaymentInfoHash`: Hardcoded constant is fine today because canonical CREATE2 deploys to the same address on Base + Base Sepolia. Worth a comment marking that this function's correctness depends on that invariant — if a chain ever ships with a non-canonical escrow address, this hash will diverge from on-chain silently.

`computePayerAgnosticPaymentInfoHash` vs `computeOnchainPaymentInfoHash` — 50+ lines of near-duplicate code. Maintainability risk: future PaymentInfo struct changes need to be applied in two places. Consider:
```ts
function computePaymentInfoHash(chainId, paymentInfo, opts: { payerAgnostic: boolean })
```
Not a security issue but it's a foot-gun.

Gas cap framing: As noted on the spec PR, the 400k cap doesn't actually bound inner-call gas (EIP-150 63/64). The event-presence check is what catches OOG'd escrow calls. The `CAPTURE_AUTHORIZER_GAS_LIMIT` docstring should say this explicitly so a future reader doesn't think they can drop the event check because "the gas cap protects us."

Trace `gasUsed > cap` check: Effectively redundant with the broadcast cap (the broadcast can't exceed the cap by construction), but useful as an early signal in the verify endpoint. Fine to keep.

`simulateCalls` feature detection: Returning `simulation_failed` when the signer doesn't expose `simulateCalls` is the right degradation — passing the MUST is impossible without it, and the spec is explicit about that.

What's solid

  • EOA path correctly untouched (no gas override on `writeContract`, normal `eth_estimateGas`).
  • `buildSettleArgs` factored out so simulate + settle build identical calldata — eliminates a whole class of "we sim-checked X but submitted Y" bugs.
  • Test fixtures (`escrowEventLog`, `buildHonestTrace`, `transferLog`) are reusable and the negative tests cover the right attack matrix: missing event, wrong hash, asset siphon, out-of-range feeBps, gas hog.
  • `traceTransfers: true` + `gas: CAPTURE_AUTHORIZER_GAS_LIMIT` on the `simulateCalls` call matches what the spec requires.

…as cap

Three correctness bugs from review, plus matching cap raise to follow the
spec PR change.

Bug 1: paymentInfo.feeReceiver == address(0) (spec-valid: delegates fee
recipient choice to the captureAuthorizer at charge time) was tripping the
asset-divergence check because the fee transfer landed on a non-zero address
the check treated as unauthorized. Fix: `verifyEscrowEvent` now decodes and
returns the actual `feeReceiver` and `feeBps` from the `PaymentCharged`
event, and `verifyAssetDeltas` uses those as the source of truth for the
allowed-recipient set (instead of `paymentInfo.feeReceiver`).

Bug 2: authorize-path asset check was tautological. After enforcing
payer = -amount + receiver = 0 + feeReceiver = 0, ERC-20 mass conservation
makes "sum of other deltas == amount" always true. Couldn't catch a
wrapper that bypassed escrow's TokenStore entirely. Fix: query
`escrow.getTokenStore(operator)` (new view in ESCROW_VIEW_ABI) and use
an allowed-recipient enumeration — any non-zero net delta to an address
outside `{payer, receiver, feeReceiver, tokenStore}` fails the check,
and `tokenStore` is required to net +amount on authorize.

Bug 3: receiver == feeReceiver double-counted the delta (same Map key,
read twice). Honest cases failed with the assertion 2*delta == amount.
Fix: explicit branch for the merged case checks combined delta == amount
and still validates feeBps from the event against `[minFeeBps, maxFeeBps]`.

Also addresses the smaller review notes:

- Raise `CAPTURE_AUTHORIZER_GAS_LIMIT` from 400_000n to 3_000_000n to match
  the spec PR. Cap docstring rewritten to explicitly call out that this is
  a DoS bound (per EIP-150 63/64) and that correctness comes from the
  escrow event check, so future readers don't drop the event check thinking
  the cap protects against escrow OOG.
- Refactor `computePayerAgnosticPaymentInfoHash` + `computeOnchainPaymentInfoHash`
  to share a single `computePaymentInfoHash(..., { payerAgnostic })` core,
  eliminating ~50 lines of near-duplicate code. Public surface unchanged.
- Comment marking `AUTH_CAPTURE_ESCROW_ADDRESS` as the canonical CREATE2
  invariant the on-chain hash function depends on.
- Tests: new mock signer dispatches `readContract` by `functionName` so
  `getTokenStore` returns a stable stand-in address. `buildHonestTrace`
  takes optional `tokenStore` / `actualFeeReceiver` overrides. Added
  positive tests for `feeReceiver == 0` and `receiver == feeReceiver`,
  plus a negative test for authorize-path siphon to confirm the new
  allowed-recipient check is actually load-bearing (158 tests, +8 from
  the previous commit).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@A1igator
Copy link
Copy Markdown
Contributor Author

Thanks for the deep read. Pushed 2901cb1 addressing all three bugs and most of the smaller things. Walking through each:

Bug 1 — feeReceiver == address(0)

Went with your second option (special-case the trace check). verifyEscrowEvent now also returns the actual feeReceiver and feeBps decoded from the PaymentCharged event, and verifyAssetDeltas uses those as the source of truth for the allowed-recipient set on charge. This preserves spec-valid feeReceiver == 0 configurations without weakening the check — if the wrapper picks some address X as the fee recipient, X has to actually be what escrow emitted, and the implied feeBps still has to land in [minFeeBps, maxFeeBps].

New positive test: should accept a charge where paymentInfo.feeReceiver == 0 and event specifies a non-zero feeReceiver.

Bug 2 — authorize-path tautology ✅

You're right that the previous otherSum === amount check was riding on ERC-20 mass conservation and would have done nothing if a future collector ever delivered to a configurable destination. Fixed by:

  • Adding getTokenStore to ESCROW_VIEW_ABI (the function exists on the deployed escrow, we just hadn't pulled it in).
  • simulateAuthorizerPassthrough now queries escrow.getTokenStore(paymentInfo.operator) up front and threads it through.
  • verifyAssetDeltas does the allowed-recipient enumeration you sketched: {payer, receiver, feeReceiver, tokenStore}, and asserts tokenStore == +amount on authorize / tokenStore == 0n net on charge.

Used the on-chain read rather than mirroring LibClone.predictDeterministicAddress in TS — fewer moving parts, and the escrow is the source of truth for what it considers its own TokenStore.

New negative test: should fail with capture_authorizer_asset_divergence when the authorize trace routes funds to an unexpected address instead of the tokenStore. Confirms the new check is actually load-bearing.

Bug 3 — receiver == feeReceiver double-count ✅

Explicit branch: if receiver === actualFeeReceiver, check combined delta == amount and validate feeBps from the event against [minFeeBps, maxFeeBps] instead of trying to split the delta two ways from a single Map entry.

New positive test: should accept a charge where receiver == feeReceiver (combined delta == amount).

Smaller things

  • AUTH_CAPTURE_ESCROW_ADDRESS hardcode comment: added. The new computePaymentInfoHash core function carries a NOTE block calling out the canonical CREATE2 invariant and recommending fail-loud at deploy time over relying on this function.
  • Duplicate hash code: refactored. computePayerAgnosticPaymentInfoHash and computeOnchainPaymentInfoHash are now thin wrappers around a single computePaymentInfoHash(chainId, paymentInfo, { payerAgnostic }) core. ~50 lines removed. Public surface unchanged.
  • Gas cap framing in docstring: rewritten to explicitly call out the EIP-150 63/64 issue and name verifyEscrowEvent as the correctness primitive that catches OOG'd escrow calls.
  • Trace gasUsed > cap check: kept (as you noted, useful as an early signal at verify time).
  • simulateCalls feature detection: unchanged.

Cap raise to 3M

Matched the spec PR's cap change. Tests updated: the assertion for gas passed to writeContract now expects 3_000_000n, and the gas-exceeded fixture uses gasUsed: 4_000_000n. The 3M figure covers the same authorize/charge baseline plus on-chain logic up to modern zk verifiers — Groth16, PLONK, Halo2, most STARK constructions.

Test count

150 → 154 (+4 new bug-fix tests; existing 9 contract-path hardening tests updated to use the new tokenStore mock). All passing, lint clean, format clean. (Commit message claims +8 / 158, which was wrong on my part — actual is +4 / 154.)

@vraspar
Copy link
Copy Markdown
Contributor

vraspar commented May 24, 2026

Round-2 follow-up. All three round-1 bugs are correctly resolved with load-bearing tests — verifyAssetDeltas allowed-set enumeration at scheme.ts:972 (authorize) and :1004 (charge), verifyEscrowEvent surfacing feeReceiver + feeBps at :881-886, and the receiverKey === actualFeeReceiverKey combined-delta branch at :1026-1031. Four polish items.

1. (tests) Magic-number gas literals will silently desync from production on the next cap tune

test:485expect(call.calls[0].gas).toBe(3_000_000n)
test:1195expect(call.gas).toBe(3_000_000n)
test:906{ gasUsed: 4_000_000n } in the over-cap fixture

Round-2's cap raise from 400k → 3M only worked because these were hand-updated alongside the constant. The test file does not import CAPTURE_AUTHORIZER_GAS_LIMIT. Import and assert against it:

import { CAPTURE_AUTHORIZER_GAS_LIMIT } from "../../../src/auth-capture/facilitator/scheme";
expect(call.calls[0].gas).toBe(CAPTURE_AUTHORIZER_GAS_LIMIT);
// Over-cap fixture:
{ gasUsed: CAPTURE_AUTHORIZER_GAS_LIMIT + 1_000_000n }

The 4_000_000n is semantically "just over the cap" — expressing that intent in the literal stops future tunes from silently breaking the test's meaning.

2. (convention) SimulateCallResult.status typed "success" | "failure" | string defeats the discriminated check

The string widening makes the if (traceResult.status !== "success") branch at scheme.ts:673 untyped; any future status value silently flows through decodeRevertReason. Tighten the type to "success" | "failure". If a non-standard RPC variant shows up in the wild, add the specific literal then.

3. (tests) No coverage for the traceResult.status === "failure" branch

scheme.ts:673-678:

if (traceResult.status !== "success") {
  if (traceResult.error) return decodeRevertReason(traceResult.error);
  return ErrSimulationFailed;
}

Suite covers gas-exceeded, escrow-missing, hash-mismatch, asset-divergence, and the simulateCalls-throws path — nothing exercises a status: "failure" trace result (which is what some RPCs return for inner reverts instead of throwing). Add a fixture returning { status: "failure", error: <encoded revert> } and assert a decoded invalidReason lands.

4. (docstring) Same "most STARK" overclaim as the spec PR

scheme.ts:152-159 (the CAPTURE_AUTHORIZER_GAS_LIMIT docstring) reuses the spec's "Groth16, PLONK, Halo2, most STARK constructions" wording. Trim to match whatever the spec PR settles on; STARK verifiers routinely exceed 3M.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants