Skip to content

MF2-H03: guard challenge_rescue against post-Finalize close#768

Merged
philanton merged 2 commits into
fix/audit-findings-finalx2from
fix/nitronode-mf2-h03
May 19, 2026
Merged

MF2-H03: guard challenge_rescue against post-Finalize close#768
philanton merged 2 commits into
fix/audit-findings-finalx2from
fix/nitronode-mf2-h03

Conversation

@philanton
Copy link
Copy Markdown
Contributor

Summary

  • HandleHomeChannelClosed previously issued a challenge rescue for every channel that was Challenged before the on-chain close, including cooperative closes backed by a node-signed Finalize. In that path the rescue either silently overwrote a post-Finalize receiver credit at (epoch+1, version=0) or collided on deterministic GetStateID with an existing (epoch+1, version=1) credit — rolling back the close-event handler and leaving the channel stuck Challenged, which in turn blocked request_creation for the same (wallet, asset) pair.
  • Guard the rescue branch with HasSignedFinalize. Mirrors the existing post-Finalize check in HandleHomeChannelCheckpointed.
  • Align NewChallengeRescueState with the NextState() post-final convention: fresh-epoch state starts at version 0, not 1. The guard guarantees no other (epoch+1, version=0) row exists when the rescue actually runs.

Failure modes prevented

Post-Finalize receiver credits are produced by NextState() in a fresh epoch with HomeChannelID=nil. Without the guard:

  1. GetLastStateByChannelID only matches channel-attached rows → returns the Finalize state at the original epoch, missing the post-Finalize credits.
  2. SumNetTransitionAmountAfterVersion filters by home_channel_id → returns zero.
  3. NewChallengeRescueState builds a zero-amount rescue at (Finalize.Epoch+1, version=1).
  4. StoreUserState either overwrites the lone credit at (epoch+1, version=0) (silent balance loss) or collides with (epoch+1, version=1) and rolls back the entire close handler.

Test plan

  • go test ./pkg/core/...TestNewChallengeRescueState updated for version 0.
  • go test ./nitronode/event_handlers/... -run TestHandleHomeChannelClosed — all rescue tests pass.
  • New TestHandleHomeChannelClosed_PostFinalize_SkipsRescue verifies the rescue branch fully short-circuits when HasSignedFinalize is true.
  • New TestHandleHomeChannelClosed_PostFinalize_CollisionRegression documents the two failure modes the guard prevents.
  • New TestHandleHomeChannelClosed_PostFinalize_HasSignedFinalizeErr confirms error propagation from the HasSignedFinalize lookup.
  • Existing rescue tests (_Squash, _NoCredits, _NegativeNet_ClampsToZero) updated for the new version=0 invariant and the added HasSignedFinalize mock.
  • go vet ./nitronode/event_handlers/... ./pkg/core/... clean.

Follow-ups (separate tickets)

  • Post-Finalize unsigned receiver credits at (epoch+1, version=0..N) with HomeChannelID=nil remain unsigned forever (channel_v1/handler.go:130 skips node-sig when channel status is non-Open). Confirm request_creation folds them into the next signed ledger correctly; otherwise file a separate fix.
  • Operator reconciliation for production channels already wedged at Challenged due to this bug pre-fix: needs a detection query (Challenged channels where HasSignedFinalize=true) and a one-shot flip to Closed.

🤖 Generated with Claude Code

HandleHomeChannelClosed previously issued issueChallengeRescue() for every
channel that was Challenged before the on-chain close, including cooperative
closes backed by a node-signed Finalize. In that path:

- GetLastStateByChannelID returns the channel-attached Finalize state at the
  original epoch, since post-Finalize receiver credits are produced by
  NextState() in a fresh epoch with HomeChannelID=nil.
- SumNetTransitionAmountAfterVersion filters by home_channel_id and therefore
  ignores those credits, returning zero.
- NewChallengeRescueState then constructed a zero-amount rescue at
  (Finalize.Epoch+1, version=1) which either overwrote a lone receiver credit
  at (epoch+1, version=0) — silent balance loss — or collided on the
  deterministic GetStateID with an existing (epoch+1, version=1) credit,
  rolling back the close-event handler and leaving the channel stuck in
  Challenged (which in turn blocked request_creation for the same
  (wallet, asset) pair).

Guard the rescue branch with HasSignedFinalize: when a node-signed Finalize
exists for the channel, the cooperative-close path has already advanced the
user to a fresh epoch via NextState() and the rescue is destructive. Mirror
the existing post-Finalize check in HandleHomeChannelCheckpointed.

Also align NewChallengeRescueState with the NextState() post-final convention:
fresh-epoch state starts at version 0, not 1. With the guard in place, no
other (epoch+1, version=0) row exists when the rescue runs.

Tests:
- New TestHandleHomeChannelClosed_PostFinalize_SkipsRescue verifies the
  rescue branch fully short-circuits when HasSignedFinalize is true.
- New TestHandleHomeChannelClosed_PostFinalize_CollisionRegression
  documents the two failure modes (overwrite + ID collision) the guard
  prevents.
- New TestHandleHomeChannelClosed_PostFinalize_HasSignedFinalizeErr
  confirms HasSignedFinalize errors propagate.
- Existing rescue tests updated for the new version=0 invariant and the
  added HasSignedFinalize mock expectation.

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

coderabbitai Bot commented May 18, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1a5eaec2-558d-4593-9c71-e28cf7c715fb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/nitronode-mf2-h03

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread nitronode/event_handlers/service.go Outdated
Copy link
Copy Markdown
Contributor

@nksazonov nksazonov left a comment

Choose a reason for hiding this comment

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

Excellent work. The guard at service.go:375 correctly and completely remediates all three failure modes described in the audit finding — the balance-overwrite path, the StoreUserState ID-collision path, and the resulting Challenged-channel deadlock. The version-0 correction in NewChallengeRescueState aligns the rescue with the NextState() convention. All three new regression tests are well-scoped and the existing tests are correctly updated. The two comments below are informational — neither blocks merging.

Comment thread pkg/core/types.go
Comment thread nitronode/event_handlers/service_test.go
Demote post-Finalize rescue-skip log to Debug; document
NewChallengeRescueState's sole authorized callsite and the collision it
prevents; mark CollisionRegression as an intentional twin of SkipsRescue
so future cleanups do not dedupe the two.

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

@ihsraham ihsraham left a comment

Choose a reason for hiding this comment

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

Approving for H-03. Nice fix: the signed-Finalize guard closes the destructive rescue path, while the normal challenge-rescue flow stays covered. Thanks for the focused tests and follow-up cleanup.

@philanton philanton merged commit 2fcdf3d into fix/audit-findings-finalx2 May 19, 2026
7 checks passed
@philanton philanton deleted the fix/nitronode-mf2-h03 branch May 19, 2026 08:22
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)
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.

3 participants