Skip to content

security: make Casper v2 settlement atomic and fail closed - #11

Draft
coinsecuritiescompany wants to merge 2 commits into
mainfrom
agent/casper-v2-atomic-settlement
Draft

security: make Casper v2 settlement atomic and fail closed#11
coinsecuritiescompany wants to merge 2 commits into
mainfrom
agent/casper-v2-atomic-settlement

Conversation

@coinsecuritiescompany

@coinsecuritiescompany coinsecuritiescompany commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Critical fixes

  • make pay_agent atomically transfer native CSPR before recording settlement
  • bind registration and payer authorization to runtime::get_caller()
  • reject forged payer, zero amount, self-payment, malformed IDs/wallets, replay and counter overflow
  • verify HTTP 402 proofs against exact execution success, contract, entry point, payer, recipient, amount and request ID
  • quarantine demo/MCP payment paths until a complete independently verified v2 deployment manifest is committed
  • require merchant self-registration with a distinct key and remove the legacy double-transfer flow
  • correct README/security/support/deployment claims for legacy v1 hashes

Verified

Release gate

Draft intentionally. Existing v1 deployments are unsafe payment proof. Independent review, signed deployment, bytecode/source provenance verification and on-chain negative/balance E2E are still required. Only then commit deployments/casper-v2.json with status: verified and update SDK/MCP. Environment variables cannot bypass the quarantine.

Tracked by AIFINP-65.

@syedhassan-aifinpay syedhassan-aifinpay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against the audit findings. The direction is right and most of it lands — but there is one blocking question that the PR does not answer, and it decides whether the whole route works.

Blocking — whose CSPR actually moves?

pay_agent now calls system::transfer_to_account(destination, amount, None) from an entry point declared EntryPointType::Called. Nothing in this PR establishes which purse that debits.

Two facts make this urgent rather than theoretical:

  • call() creates three dictionaries and two counters, but never creates a purse for the contract. There is no system::create_purse() and no purse in NamedKeys.
  • casper-contract 5.1.1 documents transfer_to_account as transferring "from the default purse of the account". In contract context that phrase is ambiguous, and EntryPointPayment::Caller does not resolve it — that enum governs who pays gas, not who funds the transfer.

The two outcomes are very different:

If it debits the caller's account purse — the design is correct and this just needs proving on chain.

If it debits the contract's purse — the contract has none, so every pay_agent call reverts and the route is dead on arrival. Worse, if anyone ever funds the contract, the caller authorization does not protect those funds: it proves who the payer claims to be, not whose money moves. A registered agent could then call pay_agent with themselves as from_agent and a second self-registered wallet as to_agent, and drain the contract.

Note that v1's mainnet demo moved value in a separate transaction. That is consistent with the in-contract transfer not having worked, and is a reason to treat this as unproven rather than assumed.

This is empirically settleable in minutes and I do not think anything else should merge ahead of it:

  1. Deploy the v2 Wasm to testnet.
  2. Register two agents from two separately funded keys.
  3. Call pay_agent and record the balance of all three parties — payer, recipient and the contract entity — before and after.
  4. If the payer's balance drops by exactly amount plus gas, the design is confirmed. If the deploy reverts with ERR_TRANSFER_FAILED (108), the transfer must be redesigned so the caller passes a purse URef and the contract draws from it.

I can run this if someone shares testnet keys, or review the output if you run it.

What is correct and worth keeping

  • Registration bound to runtime::get_caller() closes the arbitrary-wallet registration hole.
  • Payment authorization comparing from_wallet to the caller closes the unauthenticated-payer hole. Both were real v1 defects.
  • The transfer preceding the receipt write is the right ordering for atomicity. Interaction-before-effect is normally a reentrancy smell, but transferring to an account cannot execute code, so it is safe here.
  • settlement-verifier.js is a genuine improvement over the old readSessionArgs path. The previous code accepted a deploy when it could not parse the session arguments; this one returns unparseable_session_args and refuses. That fail-open branch was the finding, and it is properly closed.
  • Binding contract_hash in the verifier closes the gap where a deploy against a different contract could satisfy the check.
  • trusted-contract.js reading a manifest that env vars cannot override is the right shape, and deployments/casper-v2.json shipping as status: "source_only" with null hashes means the route is quarantined by default. That is exactly right and should not be softened to get a demo working.

Smaller points

  • valid_identifier caps identifiers at 64 bytes and restricts the charset. Good, but wallet is not length-checked before AccountHash::from_formatted_str — it is bounded in practice by the format check, so this is a note rather than a defect.
  • The self-payment check is correctly doubled: from_agent == to_agent on the identifiers and from_wallet == to_wallet on the resolved wallets. The second catches two identifiers pointing at one wallet.
  • The verifier does not assert the deploy's network name. The node URL implies it today, but a mainnet bridge pointed at a testnet node would accept testnet settlements. Cheap to add.
  • CI does not execute pay_agent anywhere — the Rust job builds Wasm and runs clippy, and the Node job tests the JS verifier. Given the release gate requires on-chain negative tests, worth stating plainly that no test in this PR exercises the contract's payment path.

On the release gate

The acceptance criteria on AIFINP-65 are the right ones, particularly "valid payment changes merchant balance by exact amount". That single test also answers the blocking question above, so I would run it first rather than last.

One process note: the ticket asks for independent review before deployment. This review is a peer review of the author's own lane, not an independent one, and should not be counted as satisfying that criterion on its own.

@syedhassan-aifinpay

Copy link
Copy Markdown
Member

Tested on testnet — the atomic transfer does not work, and I have the on-chain evidence

Rather than leave the purse question open, I built this branch's Wasm and ran it on Casper testnet. Four deploys. The result is unambiguous: pay_agent cannot move CSPR as designed. No payment succeeded and the recipient's balance never changed.

Everything below is reproducible from the branch as it stands.

# Experiment Deploy hash Result
1 Install from an account that had installed v1 before c946036f04afafea859508a0086a31a3bb32db724a43346eb416ca6808c80fd5 FAILEDApiError::InvalidArgument [3]
2 Install from a pristine account b7ebd2ca9da460353a7741d06dc4e60197b607e15faf3fc1da0500fa2ffbbd72 SUCCESS, block 8730823
3 pay_agent, exactly as in this PR faf4b96d2c17f9bd56f1f55c4d7eba26da1b3d23907606186efb5fa619a75473 FAILEDForged reference: URef(722bbec5…, READ_ADD_WRITE)
4 pay_agent patched to take a caller-supplied purse 9cd7d15e8142b67033906a02e8761783daa32d94a550249bde63fa554254c8ca FAILEDUser error: 108 (ERR_TRANSFER_FAILED)

Build: nightly-2025-02-04, --release --locked --target wasm32-unknown-unknown, 71,627 bytes, SHA-256 4c606ff8c36fd024a35ed56bef7145db2da323a6184585ba9038e548301c2a59.
Installed contract (unpatched): hash-171e55d862beaf45d9fa6d7d2881626d86f706759d7b2d1687945b809933ff5b.

Measured on the payment attempt: buyer −5.0000 CSPR (gas only), provider +0.0000, payment_count still 0.

What each failure means

Experiment 1 — installs break on any account that has installed before. call() runs storage::new_dictionary("agents") and the name already exists in that account's named keys, so the install aborts having consumed 0.0016 CSPR. Both of our funded testnet accounts carry agents, payments and events from earlier installs. This is not specific to v2, but it means the deployment runbook must state that installs require a pristine account — otherwise the mainnet deploy will fail the same way.

Experiment 3 — this is the blocker. system::transfer_to_account internally resolves the account's main purse. Under EntryPointType::Called the contract's access rights do not include that URef, so the runtime rejects it as a forged reference. Confirmed in state: the contract's named keys are agents, event_count, events, payment_count, paymentsit has no purse of its own. So there is no purse the contract may legitimately debit, and every pay_agent call reverts.

Experiment 4 — passing the purse explicitly is not sufficient either. I patched pay_agent to take a purse: URef argument and call transfer_from_purse_to_account, deployed it (4ac265aaa769acdf28d95f34fa91a267b8e5671b661c73ff4b7c5959c80c7fae) and passed the buyer's main purse. The forged-reference error disappears — so the argument does grant a reference — but the transfer still fails. This is consistent with Casper attenuating a main purse handed to a contract down to deposit-only rights: the contract may add to it, not withdraw from it. That attenuation is a deliberate protection and should not be worked around.

One thing this proves positively

The revert path is correct. On every failure payment_count stayed 0 and no settlement record or event was written. The "transfer first, then record" ordering does what it is supposed to: no receipt can exist without a completed transfer. That part of the design is sound — it is the transfer mechanism underneath it that does not work.

What the fix has to be

Atomic settlement on Casper needs the funds to arrive in a purse the contract may legitimately debit, established in the same transaction. The standard pattern:

  1. Session Wasm, run by the payer, calls system::create_purse().
  2. The same session transfers amount from the payer's main purse into that temporary purse.
  3. The session calls pay_agent(from_agent, to_agent, amount, request_id, purse).
  4. pay_agent calls transfer_from_purse_to_account(purse, destination, amount, None) and only then writes the receipt.

All four steps are one deploy, so atomicity holds and a partial payment is impossible.

This is a real design change, not a patch: it needs a session Wasm crate, its own build and pinning in CI, and the bridge/SDK updated to submit the session rather than a direct contract call. The alternative — depositing to a contract purse in one transaction and forwarding in another — reintroduces exactly the non-atomicity that this PR exists to remove, so I do not think it should be considered.

Recommendation

Do not merge as-is; the payment path does not function. I would keep everything else in this PR, because it is genuinely good — the caller-bound registration, the payer authorization, the fail-closed verifier, and the manifest quarantine are all correct and worth keeping — and treat the transfer mechanism as the remaining work.

The quarantine is doing its job in the meantime: deployments/casper-v2.json ships as status: "source_only", so assertTrustedContract refuses to start the bridge. Nothing can route Casper payments until this is genuinely fixed and verified, which is the right default.

Scripts and keys for reproducing all four experiments are local; happy to push them as a testnet-verify script if that is useful.

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