Freeze the contract once anything is staked on it, and bind funding to what it pays (F05) - #7
Conversation
…o what it pays (F05) Audit finding F05 (Medium): a session peer could send a second, valid contract after the first was funded. AuditContract re-audited it, stored it beside the original funding outpoint, and every spend from then on was built for a script the output does not pay -- refused by the network, and not by the local engine, which was fed the record's contract rather than the chain's. The initiator kept the refund branch of the funded contract. Two rules, one at each end. At the door: Swap.ContractCommitted says whether anything has been staked on the contract as it stands -- funding seen or sent to its address, a refund pre-signed, a Zenon HTLC created against its locktime, a state past waiting for funding -- and what. Past that point the contract's bytes are the swap's identity. AuditContract answers a byte-identical resend (a session retransmission, a re-sync, a second paste) with the swap as it is, logging nothing, and refuses different bytes however well they audit, naming the stake. Before a commitment a re-audit still replaces, which is how a counterparty rebuilding with a better locktime gets a second chance. SetCounterpartyPKH, the funding side's mirror, freezes the same way. At the signer: FundingOutput records the script the output actually pays, read from the funding transaction when the funding is adopted and filled in from the chain before signing when an older record lacks it; a chain that cannot be read is a refusal. buildSpend refuses a contract that does not hash to that script, so the check holds for the Recover page and a restored backup too. Redeem and Refund run the binding before building. Tests: identical resend idempotent, different contract refused with the record untouched, every kind of stake freezing, re-audit before a stake still replacing; the pubkey-hash mirror; the funding bound on adoption from a real transaction, a swapped-out contract refused before signing both through Redeem and through the bare signer, an unbound record bound from the chain before signing, and an unreadable chain refusing. The smoke suite checks a repeated audit adds nothing. README's session section says a staked contract's bytes are the swap's identity. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
…here From the Codex Security review of 10aa139 (Daybreak, REQUEST CHANGES): 1. Every call into this module runs on a goroutine of its own, and the store locked Load and Save separately, so an audit that loaded before a refresh recorded the funding could save after it -- erasing the stake, and the freeze with it. Swap.Version now counts saves and Save is a compare-and-set on it under the store's lock: a record whose version is not the one in the store is refused with ErrStaleWrite, and the loser looks again. Every load-decide-save path is covered at once. A test makes the interleaving happen on demand, through a storage that lets a refresh through mid-audit, and shows the audit losing and the funding and original contract standing. 2. An absent binding was treated as a pass. Now the refund is not pre-signed until the funding is bound (a chain that cannot say what the output pays holds it, with one log line, and it is tried on every poll); Refund binds before broadcasting even a pre-signed refund, which may date from a release that did not bind; and the Recover page, which reaches no node, refuses a file that does not record the binding unless the user ticks "build anyway", and then says on the result what it assumed. Recovery files written from now on carry the binding. Tests, as the review listed them: a stale copy refused and a reloaded one accepted; the audit-versus-refresh race; identical contract and pubkey hash resends performing no write, counted; Refresh with an unreadable funding transaction not pre-signing, once logged, then binding and signing when readable; a covering output replacing a short one, unbound and un-signed while unreadable, bound and signed when not; a pre-signed refund over a mismatched or unreadable funding not broadcast; a recovery file without the binding refused unasked, built with a warning on consent, and refused regardless when its binding names another script. The smoke suite covers the consent path offline. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
…mage out of unbound redeems From the second Codex Security review of this branch (Daybreak, REQUEST CHANGES), four required items: 1. Two tabs are two modules over one localStorage, each with a mutex of its own, so the version check could not see the other tab's write. The Web Locks API is the one primitive a browser offers across tabs of an origin, so every call into the module is now made under a lock named for this instance's storage prefix (main_js.go underStoreLock): one at a time, across every tab. The callback hands the browser a promise and does the work on a goroutine, because Go that blocks on the event loop deadlocks the module. Where the API is absent -- Node in the smoke test -- the call runs unlocked as before. A Go test cannot exercise a browser lock; what Go proves is the in-module version check, and the lock is what extends it. 2. The Recover page's "build anyway" reached BuildRedeem, and an invalid redeem submitted anywhere still shows its preimage. Consent now covers refunds only, which reveal nothing; a redeem from a file without the binding is refused with no way round, and the page's wording says so. Consent resets whenever the file changes. 3. Import decided "absent" and wrote separately; delete judged the risk and removed separately. Store.SaveIfAbsent and Store.DeleteIf do each under the store's lock, and Import and handleDelete use them. 4. A stale write after a broadcast reported failure for a transaction the chain already had. Redeem and Refund now record the outcome through saveOutcome, which on a stale write reloads, applies the outcome to the record as it now is, and saves that -- the concurrent change kept, the outcome not lost, nothing broadcast twice. The session applies a value again on a stale write before reporting a refusal; the background refresh lets the next tick take it. Nits taken: the version advances on the caller only once the write has happened, so a failed write does not poison the next attempt; the funding transaction a backend serves must hash to the id asked for. bindsTo stays open on an absent script, because every path that signs binds first and the bare signer is what the tests call directly. Tests: add-if-absent not overwriting an existing record; delete refusing under its check and removing when allowed; a failed write not advancing the version; an outcome surviving a concurrent archive with exactly one broadcast; a substituted transaction not binding; an unbound redeem refused with and without consent while an unbound refund builds with a warning and a bound redeem builds. The script-mismatch fixture now uses a real transaction under its own id. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
…o resurrect From the third Codex Security review of this branch (Daybreak, REQUEST CHANGES), five items: 1. A browser without the Web Locks API ran calls unlocked, which is the two-tab race this branch exists to close. Now only an environment with no document -- Node running the smoke test -- runs unlocked; a browser without the API is refused, by name. 2. The receiving leg's funding was marked and shown as funded before it was bound, and only the sending leg's refund block retried the binding. Refresh now tries to bind on every poll for either leg, once in the log, and the swap view carries fundingBound: the card offers no Zenon action and no redeem against a funding that is not yet read off its own transaction, and says so beside it. 3. Add-if-absent treated a stored value that no longer parses as a record, so a backup could not restore it. A corrupt value is absent. 4. saveOutcome re-applied a spend to whatever the reload returned. It now refuses a record naming a different funding output or contract, refuses to bring back a record deleted in the meantime -- Save itself refuses a versioned record whose key is gone -- and carries the transaction id in the error either way, so what happened on the chain is not lost. 5. The stale-write error is named on the wire (code "stale"), the client throws an EngineError carrying it, and the session retries exactly once on that code rather than on the text. Nit taken: the output index is compared without a narrowing conversion. Tests: the receiving leg's funding not actionable until bound and bound on a later poll; an outcome refused over a swapped funding and over a deleted record, one broadcast either way; a backup restoring a corrupt record and leaving a healthy one alone; the stale code on the wire and absent from an ordinary error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
From the fourth Codex Security review of this branch (Daybreak, REQUEST CHANGES), one required item and a real bug: bindFunding recorded the script it read BEFORE checking it against the contract, so on a mismatch the error was logged but the record was saved with the script set, the view called it bound, and the card offered the Zenon create -- the loss the finding describes, for a record whose contract had already been replaced or a backend that lies. Now the script lands on the record only once it is known to be this contract's, and a mismatch is said once in its own words. Swap.FundingBound is the one predicate -- script present AND this contract's, judged against the contract the record holds now -- and the view reports that. planCreate refuses to lock ZNN while a funding on the record is not bound, whether unread or mismatched, before any node is asked. Tests: a real transaction paying another contract leaves the funding unbound and unrecorded, logs the mismatch once over two polls, and is refused by planCreate, as is a funding merely unread; a script recorded for one contract does not bind another; the outcome identity check exercised for the output index alone and the contract alone. Nit taken: main_js.go's comment says what the fallback now is. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
Nit from the approving review: the adoption block logged every binding failure as a transaction that could not be read, a mismatch included, on top of the mismatch's own one-time note. A mismatch is now its own error kind and the misleading line is not written for it; tested. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
The branch is rebased onto 092026-audit, where F01 through F04 have been merged. Where two branches inserted at one anchor both sides are kept; this commit is what the overlap needed beyond that: - planCreate asks whether the funding is even this contract's (F05's binding) before how deep it is (F02's commitment), so a mismatched or unread funding is refused by name rather than as unconfirmed. - F01's redeem fixtures carry the binding, and its covering-output test serves real transactions so the adopted outputs bind; F05's freeze test gives the swap complete Zenon terms so F03 does not withdraw the verdict on load; F03's amount loop saves one record per case under versioned saves. - The swap view lists each field once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg
166bd77 to
bb4f9b2
Compare
|
Reviewed
These are local results; I have not validated two real browser tabs or live wallet/chain behavior. I would address the rejection path before merging and resolve the runner compatibility when reconciling #6. I can contribute focused regression tests once we align on the lock fix, and help reconcile #6 without expanding its relay-delivery scope. |
|
@0x3639 would you like me to handle @edgepillar's concerns? |
…pe stripping Two findings from review of the branch (edgepillar, PR sol-znn#7). The Web Lock wrapper discarded the promise that locks.request returns. A request the manager rejects before granting -- the specification allows a SecurityError -- never invokes the holder, so nothing answered the call and it hung past the API timeout, which starts only once the lock is held. The rejection is now handled: before a grant it answers the call as a refusal, naming the reason; once the holder is running it changes nothing, because the work is under way and its save may already have landed, so the only true answer is the work's own. The answer goes through a once, so no path can send twice on the channel from a browser callback, and a manager that throws synchronously releases both callbacks it never took. The smoke suite stands in a lock manager for each shape: refused before grant, granting in order and serialising two calls, rejecting after the holder has released, rejecting while it still runs, and throwing; and it counts any rejection left unhandled as a failure. EngineError declared its code as a constructor parameter property, which Node's type stripping refuses. Nothing under Node reached wasm.ts on this branch alone, but sol-znn#6's relay test loads useSession.ts, which now imports isStaleWrite from it, so the combined tree failed to load. The field is declared and assigned instead. Verified by merging sol-znn#6 on top and running its relay:delivery on Node 24: fails before, 12/12 after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TiMc1yHDcGagiEPA6udCjm
Codex's review of the previous commit found that the rejection handler given to locks.request was released only when it fired, so every successful call left one callback registered in Go for the life of the tab. The request now gets a handler for each outcome and whichever fires lets both go, under a once that the synchronous-throw path shares. The smoke suite stands in a thenable manager that keeps the handlers the bridge attaches and invokes them after the call has answered: Go logs "call to released function" for a released callback rather than running it, and the check counts both. Scenario 3's comment no longer claims a conforming manager rejects after the holder has completed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TiMc1yHDcGagiEPA6udCjm
im working on these now. |
Round two of Codex's review found the synchronous-throw path did not ask whether the lock had been granted: a manager that invokes the holder and then throws left the work running while the call answered with the exception, which is the late-rejection shape by another route. The recovery now releases the request's handlers and, once granted, leaves the answer to the work. The smoke suite adds that manager over a call that reaches for a node, so the work yields and the throw really does arrive first; a thenable that refuses, to probe that both handlers are released on that path too; and narrower wording for what the section claims, since a request that never settles is the one shape nothing in the bridge can rescue. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TiMc1yHDcGagiEPA6udCjm
Round three of Codex's review approved with one nit: the smoke suite showed the request's handlers released but took the holder's release on trust from the source. The thenable managers now keep the holder as well and invoke all three after the call, on fulfilment and on refusal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TiMc1yHDcGagiEPA6udCjm
|
Both addressed, in 1. Lock request rejection. 2. Three further Codex Security review rounds on the fix itself: the first found the rejection handler leaked one callback per successful call, the second found the synchronous-throw path ignored the grant, the third approved. Each is its own commit. Still not covered: two real browser tabs, and a request that never settles at all, which nothing in the bridge can rescue. @sol-znn handled here; nothing needed on your side beyond the reconciliation with #6, which should now load cleanly. |
|
Thanks for addressing these. I independently rechecked
WASM-target Go vet and the combined branch's typecheck/lint passed. The standalone development smoke suite and the combined production smoke suite each passed 163/163 with JavaScript fetch disabled. This addresses the two findings in my earlier comment within the scope of this local validation. Two real browser tabs remain untested, and a lock request that never settles remains a separate limitation. No live wallet/chain or hosted-CI validation is claimed. |
|
Thanks guys, you're awesome :) |
3 participants
Read this first: it changes the module's concurrency model
This PR started as a Medium finding about a session peer replacing a funded swap's contract. The fix for that is the first commit and is small. The five commits after it are what four rounds of security review asked for once they pulled on the thread, and they change how every call into the engine behaves, not just the audit:
wasm/main_js.go,underStoreLock): one call at a time, across all tabs of the origin, for as long as that call takes, including a refresh's chain reads. A browser without the Web Locks API is refused rather than served unlocked. A lock request the browser rejects before granting it (the specification allows a SecurityError) answers the call as a refusal naming the reason, rather than leaving it pending; a rejection that arrives once the holder is running (another context taking the lock withsteal) changes nothing, because the work is under way and the only true answer is its own. Under Node, where the smoke suite runs, calls run unlocked as before.Store.Saveis a compare-and-set on it under the store lock: a copy that has gone stale is refused with a named error (code: "stale"on the wire) instead of overwriting a decision made since. A versioned record whose key is gone was deleted in the meantime and is not brought back. Import adds a record only if absent at the moment of writing, and delete judges the risk at the moment of removal.These are the right properties for software that signs Bitcoin transactions, and I'd have argued for them in a design review. But they are broader than the finding, and the serialisation has a cost you should know about: a long refresh in one tab holds an audit in another until its node answers.
The finding
F05 (Medium / P2) from the Codex Security audit of
be879e1: A session peer can replace the script of an already-funded swap. The session handler re-audits every contract it receives, and the audit checked the new contract's terms but never asked whether anything was already staked on the old one. A second valid contract was stored beside the original funding outpoint, and every later spend was built for a script the output does not pay, refused by the network while the counterparty kept the refund branch.The fix
FundingBoundis the single predicate (script present and this contract's, judged against the contract on the record now), and it gates the card's Zenon actions and redeem, the pre-signed refund, the broadcast of even a pre-signed refund, andplanCreate. A transaction the backend serves must hash to the id asked for. Refresh retries the binding on every poll for either leg.Tests
Go covers the freeze (identical resend idempotent with zero writes, different contract refused with the record untouched, every kind of stake, re-audit before a stake still replacing, the pubkey-hash mirror), the binding (from a real transaction on adoption, a mismatching transaction never recorded and refused by
planCreate, an unread funding refused, binding filled from the chain before signing, unreadable chain refusing, substituted transaction refused, a script recorded for one contract not binding another), the store (stale copy refused, a deterministic audit-versus-refresh race through a storage hook, add-if-absent over a corrupt record, delete-if-safe, a failed write not advancing the version), outcomes (surviving a concurrent archive with exactly one broadcast, refused over a changed txid, vout, or contract, not resurrecting a deletion), and recovery (unbound refund built only with consent and warned, unbound redeem refused with or without consent, mismatched binding refused). The smoke suite covers the idempotent audit and the recovery consent offline, and stands in a lock manager for each shape the bridge must survive: refused before grant, granting in order and serialising two calls, rejecting after the holder released, rejecting while it still runs, throwing synchronously, and any rejection left unhandled.go test ./...,go veton both targets, typecheck, lint, and the dev build pass.Not covered by automation, and worth a manual check on your side: two real tabs on one swap under Web Locks, and the card's unbound-funding badge and hidden controls.
Review follow-up
edgepillar's review of
bb4f9b2found two things, both fixed in the last four commits:locks.request, so a request rejected before grant hung the call. Handled as described above, with a once around the answer so no path can send twice, and a synchronous throw releasing both callbacks. Regression in the smoke suite. Codex's review of that fix found the rejection handler was itself released only when it fired, leaking one callback per successful call; the second commit gives the request a handler for each outcome and releases both whichever fires, with a smoke check that invokes them after the call and counts Go's "call to released function" log. Its next round found the synchronous-throw path did not ask whether the lock had been granted, so a manager that grants and then throws answered the call with the exception while the work ran on; the third commit gates that path the same way. Three rounds on Daybreak at High; the third approved with one nit, that the smoke suite prove the holder callback's release rather than take it from the source, which the fourth commit takes.EngineErrorused a constructor parameter property, which Node's strip-only type stripping refuses; on this branch alone nothing under Node reachedwasm.ts, but Deduplicate relay events after verification #6's relay test loadsuseSession.ts, which now imports from it. The field is declared and assigned instead. Verified by merging Deduplicate relay events after verification #6 on top and running itsrelay:deliveryon Node 24: fails before, 12/12 after.Review trail
Five Codex Security reviews on Daybreak at High. Rounds one through four each returned REQUEST CHANGES, each pushing one layer further into the concurrency model described above, and each found something real, including a bug of mine in round four where a mismatching script was recorded as bound. The fifth returned APPROVE WITH NITS with verify-fix "fixed"; its log-wording nit is in the last commit and its request for real-browser two-tab tests is noted above.
Rebased onto
092026-audit, where #1 through #4 are merged, and targeted there. The last commit is the reconciliation the overlap needed: the binding check runs before F02's commitment check inplanCreate, and the earlier branches' fixtures carry the binding or serve real transactions so their adopted outputs bind.🤖 Generated with Claude Code
https://claude.ai/code/session_01UYCAfS9BXS9N9eCe39BRCg