Skip to content

M-H11: reject issuance of receiver state during escrow ops#674

Merged
philanton merged 1 commit into
fix/audit-findingsfrom
fix/clearnode-h-10
Apr 10, 2026
Merged

M-H11: reject issuance of receiver state during escrow ops#674
philanton merged 1 commit into
fix/audit-findingsfrom
fix/clearnode-h-10

Conversation

@philanton
Copy link
Copy Markdown
Contributor

@philanton philanton commented Apr 10, 2026

Description

When a sender submits a TransferSend, the node automatically enters the receiver-side path and calls issueTransferReceiverState. That function creates and stores a new TransferReceive state for the receiver’s channel, even if the receiver’s latest signed state is already in an escrow phase such as MutualLock or EscrowLock. As a result, an inbound transfer still advances the receiver’s channel state while the receiver is in the middle of a suspended escrow flow.
That breaks the normal completion path when the receiver later tries to complete the escrow flow.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Added validation to reject release or transfer operations when the receiver has an active escrow lock, with appropriate error messaging.
  • Tests

    • Added test cases to validate error handling for escrow lock scenarios in both app session and channel operations.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 01a18000-c3e3-4a69-bd04-b324f4872400

📥 Commits

Reviewing files that changed from the base of the PR and between da44862 and 5e7aa84.

📒 Files selected for processing (4)
  • clearnode/api/app_session_v1/handler.go
  • clearnode/api/app_session_v1/submit_app_state_test.go
  • clearnode/api/channel_v1/handler.go
  • clearnode/api/channel_v1/submit_state_test.go

📝 Walkthrough

Walkthrough

The changes introduce early validation in release-receiver-state issuance logic across two API handlers. When a last signed state has a non-nil EscrowChannelID, the functions now return an error immediately instead of conditionally skipping signing based on transition types. Two test cases validate this rejection behavior.

Changes

Cohort / File(s) Summary
Handler Logic Updates
clearnode/api/app_session_v1/handler.go, clearnode/api/channel_v1/handler.go
Replaced flag-based signing decision with early-exit validation. When lastSignedState has non-nil EscrowChannelID, handlers now return RPC error immediately instead of continuing. Removed conditional signing suppression based on lock transition types.
Test Coverage
clearnode/api/app_session_v1/submit_app_state_test.go, clearnode/api/channel_v1/submit_state_test.go
Added two new test cases validating rejection behavior: TestSubmitAppState_WithdrawIntent_ReceiverWithEscrowLock_Rejected and TestSubmitState_TransferSend_ReceiverWithEscrowLock_Rejected. Each verifies error response when receiver's last signed state contains active escrow lock.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • dimast-x
  • ihsraham

Poem

🐰 A lock so strong, escrow's guard,
Now caught before the signing card!
Early exit, swift and clean,
The finest validation ever seen.
hop hop

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the main change: rejecting receiver state issuance during escrow operations, which matches the core logic alteration across both handler files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/clearnode-h-10

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.

@nksazonov
Copy link
Copy Markdown
Contributor

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 10, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@philanton philanton merged commit 88c8a98 into fix/audit-findings Apr 10, 2026
3 checks passed
@philanton philanton deleted the fix/clearnode-h-10 branch April 10, 2026 12:32
nksazonov added a commit that referenced this pull request Apr 16, 2026
H-H01: fix(contracts): disallow challenge with CLOSE or FIN_MIG intents (#631)

H-M01: fix(contracts/ChannelHub): remove transferred amount check (#636)

fix(contracts/ChannelHub): allow unblocking escrow ops after migration (#637)

M-C01: feat(contracts): add token check between states (#639)

H-L03: fix(contracts/ChannelHub): skip disputed escrow during purge (#640)

M-H04: feat(contracts/ChannelHub): purge during escrow challenge finalization, count both skip and purge (#641)

M-M02: fix(contracts): require zero allocs on close (#643)

M-C02: fix(rpc/core): apply finalize escrow deposit correctly (#644)

H-L02: fix(contracts/ChannelHub): revert on withdrawFromVault failure (#651)

M-H03: fix(clearnode): log correct fields on failure (#647)

M-C03: fix(pkg/core): disallow negative amount transitions (#645)

H-L01: fix(contracts/ChannelHub): add CH address to prevent val addition replay (#650)

M-H06(clearnode): restrict max channel challenge duration (#654)

M-M03(clearnode): revert EscrowLock on insufficient home user balance (#655)

M-M04(clearnode): support default signer even if it is not approved (#656)

M-M05(clearnode): require correct intent on FinalizeEscrowWithdrawal (#657)

M-M09: reject asset decimals exceeding its token's decimals (#659)

M-L01: return correct errors during state advancement validation (#660)

M-L03: limit number of related ids per session key state (#662)

M-I02: normalize input hex addresses (#663)

M-H01: feat(contracts/ChannelHub): restrict to one node (#649)

M-H07: fix(contracts/ChannelHub): emit stored, not arbitrary candidate state (#664)

M-I01: feat(contracts/ChannelHub): remove updateLastState flag (#665)

H-L06: fix(contracts/ChannelHub): remove payable from methods, clarify in create (#666)

H-L09: feat(contracts/ChannelEngine): add non-home migration version check (#669)

YNU-839: fix blockchain listener lifecycle (#658)

M-H09: use channel signer for all channel state node sigs (#667)

H-L07: fix(contracts/ChannelHub): restrict createChannel to non-existing channels (#668)

H-I02: docs(contracts): add a note that rebasing tokens are not supported (#670)

H-I03: fix(contracts/ChannelHub): use CIE pattern in depositToHub (#671)

M-H11: revert empty signatures on quorum verification (#672)

M-H11: reject issuance of receiver state during escrow ops (#674)

H-I01: docs(contracts): note that fee-on-transfer tokens are not supported (#675)

M-L04: docs: mention liquidity monitoring (#677)

fix(contracts/ChannelHub): fix initialize escrow deposit dos (#679)

M-H08: enforce strict transition ordering after MutualLock and EscrowLock (#680)

fix: run forge fmt (#681)

M-L05: docs(contracts): document native token deposits (#685)

fix(clearnode): resolve a set of audit findings (#686)

M-H13: feat(contract): add validateChallengeSignature, revert in SK validator (#688)
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