security: make Casper v2 settlement atomic and fail closed - #11
security: make Casper v2 settlement atomic and fail closed#11coinsecuritiescompany wants to merge 2 commits into
Conversation
syedhassan-aifinpay
left a comment
There was a problem hiding this comment.
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 nosystem::create_purse()and no purse inNamedKeys.casper-contract5.1.1 documentstransfer_to_accountas transferring "from the default purse of the account". In contract context that phrase is ambiguous, andEntryPointPayment::Callerdoes 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:
- Deploy the v2 Wasm to testnet.
- Register two agents from two separately funded keys.
- Call
pay_agentand record the balance of all three parties — payer, recipient and the contract entity — before and after. - If the payer's balance drops by exactly
amountplus gas, the design is confirmed. If the deploy reverts withERR_TRANSFER_FAILED(108), the transfer must be redesigned so the caller passes a purseURefand 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_walletto 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.jsis a genuine improvement over the oldreadSessionArgspath. The previous code accepted a deploy when it could not parse the session arguments; this one returnsunparseable_session_argsand refuses. That fail-open branch was the finding, and it is properly closed.- Binding
contract_hashin the verifier closes the gap where a deploy against a different contract could satisfy the check. trusted-contract.jsreading a manifest that env vars cannot override is the right shape, anddeployments/casper-v2.jsonshipping asstatus: "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_identifiercaps identifiers at 64 bytes and restricts the charset. Good, butwalletis not length-checked beforeAccountHash::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_agenton the identifiers andfrom_wallet == to_walleton 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_agentanywhere — 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.
Tested on testnet — the atomic transfer does not work, and I have the on-chain evidenceRather than leave the purse question open, I built this branch's Wasm and ran it on Casper testnet. Four deploys. The result is unambiguous: Everything below is reproducible from the branch as it stands.
Build: Measured on the payment attempt: buyer −5.0000 CSPR (gas only), provider +0.0000, What each failure meansExperiment 1 — installs break on any account that has installed before. Experiment 3 — this is the blocker. Experiment 4 — passing the purse explicitly is not sufficient either. I patched One thing this proves positivelyThe revert path is correct. On every failure What the fix has to beAtomic settlement on Casper needs the funds to arrive in a purse the contract may legitimately debit, established in the same transaction. The standard pattern:
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. RecommendationDo 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: Scripts and keys for reproducing all four experiments are local; happy to push them as a |
Critical fixes
pay_agentatomically transfer native CSPR before recording settlementruntime::get_caller()Verified
a7120cc98ee67bd5218d6647f4c951662b0f7c60git diff --check: successRelease 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.jsonwithstatus: verifiedand update SDK/MCP. Environment variables cannot bypass the quarantine.Tracked by AIFINP-65.