MF2-H01: harden receiver-state issuance and dispute resolution#759
Merged
Conversation
…challenge Receiver-credit issuance (transfer_receive, release) was advancing the home channel head with a node-signed state regardless of channel status. Combined with the onchain SessionKeyValidator's missing expiry/scope checks, this could let an attacker turn a dust credit into a checkpoint that resets a dispute timer. The receiver state is still persisted while a channel is Challenged so amounts can be reconciled later, but the node no longer co-signs it. The new DB helper GetHomeChannelStatus performs the lookup inside the same transaction as the rest of the state-issuance flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generalize UpdateStateUserSigIfMissing into UpdateStateSigsIfMissing so the event reactor can repair both user and node sides of a stored state row. When a Checkpointed event lifts a channel out of ChannelStatusChallenged, the head row may have been persisted unsigned during the dispute window (any receiver state issued under the no-sign-while-challenged rule). The handler now recomputes the node signature locally for that head state so future flows treat it as fully co-signed. Open->Open checkpoints stay on the previous user-sig-only backfill path. Higher-version unsigned receiver states accrued during the challenge are intentionally left in place; they are reconciled when the channel closes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ngeRescue When a home channel is closed onchain after being Challenged, any unsigned receiver-state credits accrued during the dispute window (the no-sign-while- challenged rows) would otherwise be orphaned. The HomeChannelClosed handler now sums those credits and emits a single ChallengeRescue state on the user's ledger via core.NewChallengeRescueState: HomeChannelID is nil, AccountID is the closed channel ID, version 1 of a fresh epoch, and the amount is the sum of credits being rescued. The rescue state mirrors the "credit a user with no open channel" shape — stored without a node signature, to be folded into a properly signed state when the user next opens a channel. Includes new transition and transaction types (201), DB helper SumUnsignedReceiverStateAmountsAfterVersion, and unit + DB + handler tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…allenge clearance The Checkpointed handler previously re-signed the row at event.StateVersion (the on-chain enforced version). After a challenge that accrued unsigned receiver states, the actual off-chain head sits above that version, and the now-cleared channel's "real" latest state would have stayed unsigned, wedging the user's state chain on the next RPC. Backfill the node signature on whichever row is the highest stored version for the channel via GetLastStateByChannelID. The user-signature backfill keeps targeting event.StateVersion, since that is what the on-chain event proves was co-signed by the user. Also update the protocol docs (enforcement.md, protocol-description.md) to match: only the head queued "receive" state is signed on clearance, with earlier queued entries left unsigned as history; and surface challenge_rescue in the api.yaml transition_type and transaction_type enums so schema consumers know the new value exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR implements a challenge-rescue flow that preserves accrued unsigned receiver credits when a disputed home channel closes. When a channel enters ChangesChallenge Rescue Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
nitronode/event_handlers/service.go (2)
22-30: 💤 Low valueDoc comment understates
NewEventHandlerServiceresponsibilities.The signer/packer are also used to construct the rescue path on
HomeChannelClosed, not just the checkpoint backfill. Worth a small expansion so future readers don't assume the deps are checkpoint-only.📝 Proposed doc tweak
// NewEventHandlerService creates a new EventHandlerService instance. -// nodeSigner and statePacker are used to backfill the node signature on the -// checkpointed head state when it is missing from the local record. +// nodeSigner and statePacker are used to backfill the node signature on the +// off-chain head state when a challenge clears, and (via the rescue path) +// to produce signed state material during HomeChannelClosed handling. func NewEventHandlerService(nodeSigner *core.ChannelDefaultSigner, statePacker core.StatePacker) *EventHandlerService {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nitronode/event_handlers/service.go` around lines 22 - 30, The doc comment for NewEventHandlerService currently says nodeSigner and statePacker are used to backfill the node signature on the checkpointed head state; update it to also state these dependencies are used to construct the rescue path on HomeChannelClosed. Locate the NewEventHandlerService constructor and the EventHandlerService type and expand the comment to mention both responsibilities (checkpoint backfill and rescue-path construction for HomeChannelClosed) and reference nodeSigner and statePacker by name so future readers aren’t misled.
152-174: 💤 Low valueHelper looks correct; verify it tolerates
head.Version == event.StateVersion.When a challenge clears without any during-challenge receiver state, the head equals the on-chain checkpointed row, which was just user-sig-backfilled on lines 126–130. In that case the row at
head.Versionmay already be node-signed (if the original RPC path signed it) or still missing a node sig (if it was originally a receiver-only state checkpointed on chain). The current logic correctly node-signs the missing case and is a no-op whenhead.NodeSig != nil, so this is well-behaved. The doc comment on lines 132–136 ("the off-chain head may sit above event.StateVersion") reads as ifhead.Version > event.StateVersionis the only invocation case — consider rewording slightly so the head-equals-checkpoint case is documented as the "no-op" path explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nitronode/event_handlers/service.go` around lines 152 - 174, Update the function comment for backfillOffChainHeadNodeSig to clarify that the off-chain head can be equal to event.StateVersion (not only greater), and explicitly document that when head.Version equals the checkpointed on-chain state the function is a no-op if head.NodeSig is already present; mention that the function will still sign the row if the checkpointed head is missing the node signature. Reference the backfillOffChainHeadNodeSig function and its behavior around head.Version and head.NodeSig when rewording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nitronode/api/channel_v1/submit_state_test.go`:
- Around line 240-261: The test setup currently ignores errors returned from
core.NewChannelDefaultSigner for nodeSigner and userWalletSigner; update these
calls to capture the error values and fail the test on error (e.g., check err
and call t.Fatalf/t.Fatal or require.NoError) so setup failures are not
swallowed. Locate the two NewChannelDefaultSigner calls (assigning nodeSigner
and userWalletSigner) and replace the blank-identifier ignores with proper error
variables and assertions that abort the test if an error is returned.
In `@nitronode/event_handlers/service_test.go`:
- Around line 1433-1475: The test comment for
TestHandleHomeChannelClosed_OpenChannel_NoRescue is inaccurate: the code sets
channel.Status = core.ChannelStatusOpen and asserts that
SumUnsignedReceiverStateAmountsAfterVersion is not called, so this is an
Open→Closed happy-path test, not a "Finalize on Challenged" race; either update
the function comment to describe the Open→Closed scenario (referencing
TestHandleHomeChannelClosed_OpenChannel_NoRescue, channel.Status, and the mocked
calls: GetChannelByID, UpdateChannel, RefreshUserEnforcedBalance,
UpdateStateSigsIfMissing and the negative assertions on
SumUnsignedReceiverStateAmountsAfterVersion/StoreUserState/RecordTransaction),
or modify the test to exercise the Challenged→Finalize race by setting
channel.Status = core.ChannelStatusChallenged, stubbing
mockStore.SumUnsignedReceiverStateAmountsAfterVersion(channelID, closureVersion)
to return 0, and then keeping the same assertions that StoreUserState and
RecordTransaction are not called; pick one and make the corresponding change so
comment, setup, and assertions align.
In `@nitronode/event_handlers/service.go`:
- Around line 295-318: Clarify and harden issueChallengeRescueIfNeeded:
explicitly document/verify SumUnsignedReceiverStateAmountsAfterVersion's ">"
semantics so we don't silently exclude unsigned receiver states at version ==
closureVersion (add a comment and/or an explicit DB check that no unsigned
receiver-state rows exist with version == closureVersion if the protocol forbids
them), and add an assertion or explicit error check after retrieving prev with
GetLastStateByChannelID(channel.ChannelID, false) that prev.Version >
closureVersion (or return a clear error/log if not) before using prev as the
predecessor for core.NewChallengeRescueState to surface DB/invariant violations
early.
---
Nitpick comments:
In `@nitronode/event_handlers/service.go`:
- Around line 22-30: The doc comment for NewEventHandlerService currently says
nodeSigner and statePacker are used to backfill the node signature on the
checkpointed head state; update it to also state these dependencies are used to
construct the rescue path on HomeChannelClosed. Locate the
NewEventHandlerService constructor and the EventHandlerService type and expand
the comment to mention both responsibilities (checkpoint backfill and
rescue-path construction for HomeChannelClosed) and reference nodeSigner and
statePacker by name so future readers aren’t misled.
- Around line 152-174: Update the function comment for
backfillOffChainHeadNodeSig to clarify that the off-chain head can be equal to
event.StateVersion (not only greater), and explicitly document that when
head.Version equals the checkpointed on-chain state the function is a no-op if
head.NodeSig is already present; mention that the function will still sign the
row if the checkpointed head is missing the node signature. Reference the
backfillOffChainHeadNodeSig function and its behavior around head.Version and
head.NodeSig when rewording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f3e5a54-b3a6-42c6-85ab-3f16330c5f7d
📒 Files selected for processing (26)
contracts/SECURITY.mddocs/api.yamldocs/protocol/enforcement.mdnitronode/api/app_session_v1/handler.gonitronode/api/app_session_v1/interface.gonitronode/api/app_session_v1/submit_app_state_test.gonitronode/api/app_session_v1/testing.gonitronode/api/channel_v1/handler.gonitronode/api/channel_v1/interface.gonitronode/api/channel_v1/submit_state_test.gonitronode/api/channel_v1/testing.gonitronode/event_handlers/service.gonitronode/event_handlers/service_test.gonitronode/event_handlers/testing.gonitronode/main.gonitronode/store/database/channel.gonitronode/store/database/channel_test.gonitronode/store/database/db_store_test.gonitronode/store/database/interface.gonitronode/store/database/state.gopkg/blockchain/evm/channel_hub_reactor.gopkg/blockchain/evm/channel_hub_reactor_test.gopkg/core/interface.gopkg/core/types.gopkg/core/types_test.goprotocol-description.md
Previously the rescue state was skipped when no unsigned receiver credits had accrued during the dispute. That left the user's latest stored state pointing at the closed channel with a non-finalize transition, which: - caused subsequent receiver-state issuance to inherit the closed HomeChannelID via NextState() (IsFinal() returns false), node-signing credits onto a closed channel that the user can no longer enforce; and - blocked channels.v1.request_creation with "channel is already initialized", forcing the user off the wallet to recover usage of the asset. Emit the rescue state unconditionally on Challenged -> Closed. When no credits were queued, the rescue state carries amount = 0 — its purpose then is purely to advance the user's state chain to a fresh epoch with HomeChannelID nil so the closed-channel binding does not leak into future operations. core.NewChallengeRescueState now accepts amount >= 0 (still rejects negative). The handler helper is renamed issueChallengeRescueIfNeeded -> issueChallengeRescue to match the unconditional behavior. Doc rewording in enforcement.md, protocol-description.md, and contracts/SECURITY.md follows suit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- nitronode/api/channel_v1/submit_state_test.go: capture errors from core.NewChannelDefaultSigner instead of discarding with `_`, matching the repo Go rule on error handling. - nitronode/event_handlers/service_test.go: rewrite the doc comment above TestHandleHomeChannelClosed_OpenChannel_NoRescue so it describes the actual Open → Closed scenario (channel was never Challenged) instead of the stale Finalize-while-Challenged wording. - nitronode/event_handlers/service.go: document at the issueChallengeRescue call site why SumUnsignedReceiverStateAmountsAfterVersion is strict `>` on closureVersion, and tighten the prev-state-missing error message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nksazonov
reviewed
May 14, 2026
Per review: the repo has no centralized security-findings index, so referencing "MF2-H01" is opaque to future readers. Replace with a short description of the dust-credit checkpoint-reset / dispute-timer attack the guard is preventing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
nksazonov
reviewed
May 15, 2026
nksazonov
reviewed
May 15, 2026
nksazonov
reviewed
May 15, 2026
nksazonov
reviewed
May 15, 2026
Contributor
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Nice work! The three security remediations described in the added docs are all correctly implemented, well-tested, and match the spec. All findings below are informational.
…etup Address remaining nksazonov review nits on PR 759: - `issueChallengeRescue`: clarify negative-sum error mentions receive states - `NewTransactionFromTransition`: extend nil-sender error to call out challenge_rescue - `TestHandleHomeChannelCheckpointed_Success`: swap bare struct literal for `newTestEventHandlerService(t)` so future mock returns of an unsigned head don't crash on a nil statePacker / nodeSigner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d sits above closure Two defensive hardenings on the squash that runs when a Challenged home channel is closed onchain: 1. SumUnsignedReceiverStateAmountsAfterVersion now requires an epoch arg and filters rows by it. Channel IDs are unique per nonce so cross-epoch rows shouldn't share a home_channel_id today, but pinning the epoch keeps the invariant explicit and would surface any future DB inconsistency that reused an ID across epochs. The caller in issueChallengeRescue passes prev.Epoch (the closed channel's last off-chain state), so the sum can never reach above the closed channel's natural lifetime. 2. The caller now asserts prev.Version > closureVersion whenever the sum is non-zero. Contributing rows live strictly above closureVersion by the query's WHERE clause, so the head must too; a violation means the state chain disagrees with itself and is worth surfacing before issuing a rescue built on a stale predecessor. Reorders the prev fetch above the sum call so its epoch can flow in cleanly. Updates all three SumUnsigned... interfaces (store, core handler, evm reactor), both mocks, and the DB-level test (adds a cross-epoch exclusion case). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, drop GetHomeChannelStatus issueTransferReceiverState (channel_v1) and issueReleaseReceiverState (app_session_v1) previously gated node-signing on a denylist (skip when status == Challenged, sign otherwise). That left Closing- and Closed-status channels signed: a Closing channel has a co-signed Finalize in flight and a Closed channel will never settle, so attaching a node sig to a fresh receiver state on either is pointless and on Closing can produce a row that races the cooperative close. Switch to an allowlist anchored to the receiver's *active* home channel: CheckActiveChannel already returns a non-nil status only for Open or Void channels and nil for Challenged / Closing / Closed. Sign only when the returned status is Open; otherwise persist the receiver row unsigned. The Challenged path still falls into the unsigned branch (the squash at close picks it up), Closing / Closed avoid producing a doomed node-signed credit, and the new check needs no additional DB method. That also lets us delete GetHomeChannelStatus end-to-end — it was added earlier in this PR purely to power the denylist, and CheckActiveChannel covers the new check. Also tighten backfillOffChainHeadNodeSig: after a challenge clears it now skips (with a debug log) when the off-chain head is not a receiver-credit transition, matching the spec wording. User ops are rejected upstream while the channel is Challenged so this should never fire, but a defensive guard avoids node-signing an unexpected transition kind if that invariant ever breaks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…serialize with receiver issuance issueTransferReceiverState (channel_v1) and issueReleaseReceiverState (app_session_v1) acquire LockUserState(receiver, asset) at the start of their RPC transactions, before reading channel status. HandleHomeChannelClosed previously didn't take that lock, leaving a window where an RPC receiver- issuance reading "channel is Challenged" could commit an unsigned receiver row after the event handler had already summed and issued the ChallengeRescue: that row would be orphaned with home_channel_id = closed channel, version above the closure version, and would never be redeemed. Acquire the same lock at the top of HandleHomeChannelClosed (after the channel-type guards, before UpdateChannel / sum / rescue). The two paths now serialize on the user_balances row: - If the RPC tx wins: it commits its unsigned receiver row first; the event tx then sees that row in the rescue sum. - If the event tx wins: it sets channel status to Closed and issues the rescue (which advances the user state head to HomeChannelID = nil); the RPC tx unblocks, picks up the post-rescue head via GetLastUserState, and produces a new state with HomeChannelID = nil — the "credit to a user with no open home channel" shape, no orphan possible. Add LockUserState to core.ChannelHubEventHandlerStore and evm.ChannelHubReactorStore (DBStore already implements it), wire the mocks, and add the expected call to all four HandleHomeChannelClosed_* tests. Concurrent integration coverage for both lock orders (Postgres FOR UPDATE serialization) is deferred to a follow-up — the lock primitive itself is already proven by LockUserState's own DBStore tests, and this commit only adds the call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… closure Previous scope (MF2-H01 narrow): sum unsigned (TransferReceive, Release) amounts strictly above closureVersion. Covered only the dust-during- challenge attack; left two soundness gaps surfaced during review: 1. Signed receives above closure stranded. When the user challenges with an older state (e.g. to dispute a poisoned head), legitimate receives that landed pre-challenge sit above closureVersion in the DB but were excluded by `node_sig IS NULL` because the channel was Open at issuance, so the row is node-signed. User walked away undercredited. 2. Sends above closure never deducted. The recipient is credited offchain by the Node when the send is acknowledged; rescuing the user for receives without docking sends double-pays the hub (recipient gets the offchain credit AND the user keeps the higher onchain balance from rolling back). Replace the predicate with a net-change query: receives (TransferReceive, Release) above closure minus sends (TransferSend, Commit) above closure Drop `node_sig IS NULL` — signed pre-challenge transitions are real value the state-advancer already validated at submit time. HomeDeposit / HomeWithdrawal / escrow / migrate / finalize / acknowledgement stay excluded: they require onchain backing the chain didn't enforce at this closure or settle on a different ledger. Commit is included as a debit because the user committed funds to an app session whose release won't land if the channel closed stale, mirroring the TransferSend case. Caller (issueChallengeRescue) clamps the net at zero. Negative net is only reachable when the user closes at a version where her own channel balance was higher than the off-chain head — adversarial rollback of her own sends/commits. Onchain has already paid above the head value; rescue must not dock further. Rename SumUnsignedReceiverStateAmountsAfterVersion -> SumNetTransitionAmountAfterVersion across: - nitronode/store/database (impl, interface, sqlite test) - pkg/core (ChannelHubEventHandlerStore) - pkg/blockchain/evm (ChannelHubReactorStore) - mocks in nitronode/event_handlers/testing.go and pkg/blockchain/evm/channel_hub_reactor_test.go New DB-level test cases pin every scenario from the design discussion: pure dust during challenge, signed pre-challenge receives stranded, HomeDeposit poison exclusion, adversarial rollback (negative net), Commit deduction, excluded transition kinds, strict-> closure boundary, and the pre-existing cross-epoch exclusion. New event-handler test pins the negative-net clamp. Docs (docs/protocol/enforcement.md, protocol-description.md, contracts/SECURITY.md) updated to describe the broader scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nksazonov
approved these changes
May 15, 2026
ihsraham
requested changes
May 18, 2026
Collaborator
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
I would hold approval for now. The receiver-state signing rule looks right in the steady state, but challenge enter/clear is not serialized with the receiver issuance path. That leaves a race where a receive/release RPC can make a node-signed state after the challenge is observed onchain but before the local DB status update is visible, which is enough to recreate the H-01 reset precondition.
| } | ||
|
|
||
| if err := tx.UpdateStateUserSigIfMissing(event.ChannelID, event.StateVersion, event.UserSig); err != nil { | ||
| if err := tx.UpdateStateSigsIfMissing(event.ChannelID, event.StateVersion, event.UserSig, ""); err != nil { |
Collaborator
There was a problem hiding this comment.
[P1/blocking] This handler needs to take the same LockUserState(channel.UserWallet, channel.Asset) lock before mutating the channel to Challenged.
The receiver issuance paths already lock that row, then call CheckActiveChannel and may attach NodeSig when they still see Open. Without the same lock here, an in-flight transfer receive or app-session release can read Open, sign the receiver state, and commit after this challenge event, leaving a node-signed higher-version receiver state for a disputed channel.
I would make challenge entry use the same serialization pattern as HandleHomeChannelClosed: load the channel, lock the user/asset row, then update status/backfill signatures while still inside the transaction.
Contributor
Author
There was a problem hiding this comment.
Yep, good catch — same race class as the one 24cdaff1 closed for HandleHomeChannelClosed. Fixed in 8d914687: LockUserState(channel.UserWallet, channel.Asset) acquired right after the nil/type guards, before any mutation. RPC paths block until the challenge event commits and then re-check Status via CheckActiveChannel.
Added TestHandleHomeChannelChallenged_AcquiresUserLockBeforeMutation to pin the ordering via a captured-flag guard on UpdateChannel)
| // channel's actual latest state. Backfill the node signature on that head so future | ||
| // flows treat it as fully co-signed. On normal Open→Open checkpoints the head row | ||
| // is already node-signed via the RPC path and this is a no-op. | ||
| if wasChallenged { |
Collaborator
There was a problem hiding this comment.
[P2] Challenge clearance should also take the user-state lock before flipping back to Open and calling backfillOffChainHeadNodeSig.
Otherwise a receiver RPC can read Challenged, choose to store an unsigned receiver state, then this handler can backfill the old head and commit before that RPC stores a newer unsigned head. The channel is now open, but the latest local head remains unsigned.
One way to close this is to lock (channel.UserWallet, channel.Asset) before UpdateChannel and the backfill. Then the RPC either commits before the backfill and is included, or waits and sees Open and signs normally.
Contributor
Author
There was a problem hiding this comment.
Yep, same fix applied to HandleHomeChannelCheckpointed in 8d914687. LockUserState goes in after the nil/type guards and before UpdateChannel + backfillOffChainHeadNodeSig, so an RPC that read Status=Challenged either commits its unsigned row before us (and the backfill picks it as the head) or blocks until we set Status=Open and then sees that via its own re-check.
Took the lock unconditionally rather than only on wasChallenged — the Open→Open path still mutates StateVersion and calls UpdateStateSigsIfMissing, and uniform pattern across the three handlers is easier to reason about. Added TestHandleHomeChannelCheckpointed_AcquiresUserLockBeforeMutation to pin the ordering)
…nelCheckpointed Same race class as MF2-H01 fixed in HandleHomeChannelClosed: receiver-issuance RPC paths (issueTransferReceiverState / issueReleaseReceiverState) lock the user/asset row and re-check channel status, but the challenge/checkpoint event handlers mutated status without taking that lock. An in-flight RPC could sign a receiver state for a status the channel no longer holds: 1. HandleHomeChannelChallenged: RPC reads Status=Open via CheckActiveChannel, node-signs a receiver state, commits after the handler flips status to Challenged. Result: node-signed higher-version receiver row on a disputed channel. 2. HandleHomeChannelCheckpointed: RPC reads Status=Challenged, decides to store an unsigned receiver row, commits after the handler flips back to Open and backfills the prior head via backfillOffChainHeadNodeSig. Result: Open channel with an unsigned latest head that the backfill never reached. Serialize both handlers on LockUserState matching HandleHomeChannelClosed, acquired after the nil/type guards and before any status mutation or backfill. Lock is released when the surrounding event-processing transaction commits. Existing handler tests gain the lock expectation. Two new _AcquiresUserLockBeforeMutation tests pin the ordering via a captured-flag guard on UpdateChannel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ihsraham
approved these changes
May 18, 2026
Collaborator
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head, the blocking race is addressed and focused checks pass.
nksazonov
added a commit
that referenced
this pull request
May 19, 2026
- MF2-I02: fix: add SESSION_KEY_AUTH_TYPEHASH to session key authorization payload (#767) - MF2-M02: accept pre-finalize escrow version in ongoing check (#765) - MF2-L03: docs: document home chain migration as not yet active off-chain (#769) - MF2-I03: reject reused home channel ID explicitly (#766) - MF2-H03: guard challenge_rescue against post-Finalize close (#768) - MF2-C02: bound ledger values to Solidity uint256/int256 ranges (#764) - MF2-C01: preserve post-Finalize closing marker across challenge cycle (#762) - MF2-M01: fix(sdk): use keccak256(utf8) for session key application ID hashing (#761) - MF2-H01: harden receiver-state issuance and dispute resolution (#759) - MF2-L01: gate transfer_send during channel creation (#763)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two security findings:
ChannelStatusChallengedhome channel, enabling dispute reset via checkpoint.request_creationrejected fresh channels withchannel is already initialized, follow-up RPCs failed withuser has no active channel, and incoming receives could still node-sign credits onto the closed channel. User had to migrate wallets to recover use of the asset.Five remediations, each adding tests:
Restrict node co-signing of receiver states to
Openhome channels.issueTransferReceiverState(channel_v1) andissueReleaseReceiverState(app_session_v1) now allowlist viaCheckActiveChannel: node-signs only when the receiver's active home channel isOpen. Receiver states forChallenged,Closing, andClosedchannels are persisted unsigned (still tracked for reconciliation, never returned for onchain submission). Closes the MF2-H01 dust-checkpoint vector and also closes the MF2-H02 vector of incoming sends getting node-signed onto an already-closed home channel.Backfill node sig on the off-chain head when a challenge clears.
HandleHomeChannelCheckpointednow recomputes the node signature for the channel's highest stored version (GetLastStateByChannelID) rather than the on-chain enforced version, so any during-challenge receiver state that became the new latest row is fully co-signed.UpdateStateUserSigIfMissingis generalized toUpdateStateSigsIfMissing(channelID, version, userSig, nodeSig). Open→Open checkpoints stay on the prior user-sig-only path. Defensive guard skips backfill when the head transition is not a receiver-credit type.Squash challenge-window net channel change into
ChallengeRescueonChallenged → Closed; emit unconditionally to clear the closed-channel head (MF2-H02). New DB querySumNetTransitionAmountAfterVersionreturnsreceives − sendsabove the closure version, where receives = (TransferReceive,Release) and sends = (TransferSend,Commit), across both signed and unsigned rows. Caller clamps net at zero (negative net is reachable only when the user closes at a version where her own channel balance exceeded the off-chain head — adversarial rollback of her own sends/commits; on-chain has already paid above head, rescue must not dock further).HomeChannelClosedemits a singleChallengeRescue(transition + transaction type201) on the user's ledger — fresh epoch, version 1,HomeChannelIDnil,AccountID= closed channel ID — built viacore.NewChallengeRescueState. Emission is unconditional onChallenged → Closed(zero-amount allowed): this is the MF2-H02 fix. Without it, the receiver's latest stored state retains the closedHomeChannelID,IsFinal()returns false on the non-finalize head,NextState()inherits the closed channel ID,channels.v1.request_creationfails withchannel is already initialized, andsubmit_statecalls fail withuser has no active channel. The rescue advances the user's head toHomeChannelID = nilsorequest_creationproceeds and subsequent receiver issuance produces the "credit a user with no open channel" shape.SumNetTransitionAmountAfterVersionis pinned to the previous head's epoch; the caller assertsprev.Version > closureVersionwhenever the sum is non-zero. (CooperativeOpen → Closing → Closedcloses via Finalize, soIsFinal()is true and no rescue is needed; that path is unaffected.)Serialize event-handler status mutations with receiver-issuance RPCs via
LockUserState.HandleHomeChannelClosed,HandleHomeChannelChallenged, andHandleHomeChannelCheckpointednow acquireLockUserState(user, asset)after type guards and before status mutation / sum / rescue / backfill. Closes a race window where an in-flight RPC readingStatus = Open(orChallenged) viaCheckActiveChannelcould commit a node-signed or unsigned receiver row after the handler had already flipped status, producing orphaned rows (Openchannel with unsigned latest head not reached by backfill; node-signed receiver on a disputed channel; receiver row above closure version that the rescue sum already finalized).LockUserStateadded tocore.ChannelHubEventHandlerStoreandevm.ChannelHubReactorStore(DBStore already implemented it).Document the accepted session-key off-chain scope-enforcement gap.
SessionKeyValidatoron-chain validates cryptographic signatures only; off-chainValidateChannelSessionKeyForAsset(expiration, asset scope) runs only on the node's acknowledgement path (channels.v1.submit_statewithTransitionTypeAcknowledgement). A holder of a session key ever authorized on the channel — including expired, revoked, or retired keys — can fetch the node-signed receive state, sign manually, and submit directly on-chain. Outcome is bounded by states the user already signed and the receive state's strictly-additive allocation effect on the user, so the gap is griefing-only on the user's own sends, not theft; and the direct-submission path is the only recovery when the node is unavailable. Documented incontracts/SECURITY.mdanddocs/protocol/security-and-limitations.md.Protocol docs (
docs/protocol/enforcement.md,protocol-description.md) updated to describe the broader scope.challenge_rescueadded totransition_typeandtransaction_typeenums indocs/api.yaml.contracts/SECURITY.mdupdated for the net-change rescue scope and the accepted limitation.Test plan
go test ./...— full suite greenOpenhome channel (Challenged / Closing / Closed) produces unsigned rows in bothchannel_v1(transfer) andapp_session_v1(release) flows;Openproduces node-signedHandleHomeChannelCheckpointedre-signs the off-chain head (highest stored version) when challenge clears; no-op when head is already signed; skips when head transition is not a receiver-credit type; no head-sig call on Open→OpenHomeChannelClosedwhileChallengedemits oneChallengeRescuewithmax(0, receives − sends)above closure; zero-amount rescue still emitted to advance epoch (MF2-H02 regression); deterministic txID =GetReceiverTransactionID(closedChannelID, newStateID); no rescue on Open→Closed (cooperative finalize)SumNetTransitionAmountAfterVersion— dust receives during challenge; signed pre-challenge receives stranded above closure included; HomeDeposit / finalize / acknowledgement / escrow / migrate excluded; adversarial rollback (negative net) returned; Commit deduction; strict>closure boundary; cross-epoch exclusionHandleHomeChannelClosed,HandleHomeChannelChallenged,HandleHomeChannelCheckpointedacquireLockUserStatebefore status mutation / backfill — captured-flag ordering tests onUpdateChannelNewChallengeRescueStateaccepts amount ≥ 0 (rejects negative), rejectsprev.HomeChannelID == nil; produces version=1, empty token/blockchain, balance creditedPostgres
FOR UPDATEintegration coverage for both lock orders (RPC wins vs event wins) deferred to a follow-up; the lock primitive is proven byDBStore.LockUserState's own tests, and the call sites are pinned by the ordering tests above.🤖 Generated with Claude Code