M2-H01: fix(contracts/ChannelHub): add finalize escrow home chain intent check#704
Conversation
nksazonov
requested review from
dimast-x,
ihsraham and
philanton
as code owners
📝 WalkthroughWalkthroughHome-chain escrow deposit and withdrawal processing consolidates into a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol (1)
162-168: Test is fine as a single-case regression.Exercises the
require(intent == DEPOSIT || WITHDRAW || OPERATE, IncorrectStateIntent())guard at the top ofcreateChannel. If you want stronger coverage of the negative space (e.g.,CLOSE,INITIATE_ESCROW_DEPOSIT,FINALIZE_ESCROW_WITHDRAWAL,INITIATE_MIGRATION,FINALIZE_MIGRATION), a tiny table-driven helper or per-intent tests would be cheap to add — but not required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol` around lines 162 - 168, Test test_revert_ifDisallowedIntent correctly asserts createChannel reverts for StateIntent.CLOSE, but to improve negative-space coverage add additional cases: create a table-driven test or separate tests that iterate through other disallowed StateIntent values (INITIATE_ESCROW_DEPOSIT, FINALIZE_ESCROW_WITHDRAWAL, INITIATE_MIGRATION, FINALIZE_MIGRATION) and assert vm.expectRevert(ChannelHub.IncorrectStateIntent.selector) when calling ChannelHub.createChannel with each; keep the existing test as-is if you prefer single-case regression.contracts/src/ChannelHub.sol (1)
736-747: Fix correctly restores the missing intent + signature validation on the home-chain finalize path.The added
candidate.intent == StateIntent.FINALIZE_ESCROW_DEPOSITcheck and explicit_validateSignaturescall at the entry point close the described hole: previously a co-signedINITIATE_ESCROW_DEPOSITcould be submitted throughfinalizeEscrowDepositon the home chain and applied to the channel, advancing state and pulling node liquidity into locked funds. With the intent constraint the candidate must be aFINALIZE_ESCROW_DEPOSITtransition (whichChannelEngine.validateTransitionfurther validates againstprevState), and with sig validation preceding_processHomeChainEscrowthe helper no longer needs to re-validate.Minor optional refactor:
_channels[channelId].definitionis read asstoragehere and then re-read asmemoryinside_processHomeChainEscrow(line 1057). You could pass the already-loadedChannelDefinition memory(orstorage pointer) down to avoid the extra SLOAD of user/node/approvedSignatureValidators/etc. Same applies to the withdrawal symmetric path below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/ChannelHub.sol` around lines 736 - 747, finalizeEscrowDeposit lacked intent and signature checks on the home-chain path; enforce candidate.intent == StateIntent.FINALIZE_ESCROW_DEPOSIT and call _validateSignatures(channelId, candidate, metaDef.user, metaDef.approvedSignatureValidators) before invoking _processHomeChainEscrow to ensure only properly-intended, co-signed transitions are applied; optionally, to save an SLOAD, pass the already-read ChannelDefinition (metaDef) into _processHomeChainEscrow instead of re-reading _channels[channelId].definition.contracts/test/ChannelHub_units/ChannelHub_initiateEscrowWithdrawal.t.sol (1)
12-19: Covers the first-line intent guard — LGTM.The
intentcheck is the very firstrequireininitiateEscrowWithdrawal, so this test hits it independent of definition/signature validity. Same suggestion as oncreateChannel: consider additional wrong-intent cases (e.g.,INITIATE_ESCROW_DEPOSIT,FINALIZE_*,OPERATE) if you want full negative-space coverage, but not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/ChannelHub_units/ChannelHub_initiateEscrowWithdrawal.t.sol` around lines 12 - 19, Test only covers one wrong intent; add additional negative-case tests that set State.intent to other invalid values (e.g., StateIntent.INITIATE_ESCROW_DEPOSIT, StateIntent.FINALIZE_* variants, StateIntent.OPERATE) and assert vm.expectRevert(ChannelHub.IncorrectStateIntent.selector) before calling cHub.initiateEscrowWithdrawal(def, state). Add separate test functions (e.g., test_revert_ifWrongIntent_INITIATE_ESCROW_DEPOSIT, test_revert_ifWrongIntent_FINALIZE, test_revert_ifWrongIntent_OPERATE) in ChannelHub_initiateEscrowWithdrawal.t.sol to exercise those intent guards and keep the existing test as-is.contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol (2)
59-73: Consider a regression test that uses the actual exploit intent (INITIATE_ESCROW_DEPOSIT).Per the PR description, the specific bug being fixed is that a co-signed
INITIATE_ESCROW_DEPOSITstate could be submitted viafinalizeEscrowDepositon the home chain, reaching_processHomeChainEscrowFinalizeand moving node funds. The two tests here only useStateIntent.DEPOSIT, which would likely have reverted even before the fix (e.g., downstream validation in the engine).To directly lock in the fix, add a regression test that builds a valid home-chain
INITIATE_ESCROW_DEPOSITstate (likeescrowStateinChannelHub_initiateEscrowDeposit.t.sol), co-signed, and assertsfinalizeEscrowDeposit(channelId, ..., state)reverts withIncorrectStateIntent— proving the home-chain guard rejects the previously-exploitable payload.Also, since the entry-point intent guard runs before any channel lookup, the existing
setUpscaffolding (channel creation, signed init state) is unused by the two new tests. Either drop the scaffolding for these tests or, preferably, keep it and add the regression test above which actually needs it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol` around lines 59 - 73, Update the tests so one explicitly reproduces the exploited input: in test_revert_homeChain_ifWrongIntent (or add a new regression test) build a valid co-signed home-chain State with intent StateIntent.INITIATE_ESCROW_DEPOSIT (similar to escrowState in ChannelHub_initiateEscrowDeposit.t.sol), then call cHub.finalizeEscrowDeposit(channelId, bytes32(0), state) and expect a revert with ChannelHub.IncorrectStateIntent.selector; keep the existing setUp scaffolding (channel creation and signed init state) so the test uses realistic data rather than the current DEPOSIT-only inputs that wouldn’t have exercised the regression.
67-73: Minor:test_revert_nonHomeChain_ifWrongIntentis functionally equivalent to the home-chain test.Since the intent check is an entry-point require that runs before
_isChannelHomeChainis consulted, passingbytes32(0)vschannelIdmakes no difference — both hit the samerequirestatement. The "nonHomeChain" vs "homeChain" naming suggests differentiated code paths that aren't actually exercised. Consider either:
- merging into a single
test_revert_ifWrongIntent, or- replacing this test with one that exercises the non-home-chain path with a valid intent but some other failure mode (e.g., missing escrow record), so the "nonHomeChain" label is meaningful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol` around lines 67 - 73, The test test_revert_nonHomeChain_ifWrongIntent is redundant because finalizeEscrowDeposit checks intent before checking home-chain, so passing bytes32(0) vs channelId yields the same require hit; either merge this with test_revert_ifWrongIntent (remove the duplicate and keep one test invoking finalizeEscrowDeposit with StateIntent.DEPOSIT and expecting ChannelHub.IncorrectStateIntent) or replace this test to actually exercise the non-home-chain path: call finalizeEscrowDeposit with a valid intent (not StateIntent.DEPOSIT), a non-home channelId (use channelId), and set up ledger state so the intent passes but the non-home-chain branch runs and then assert the expected failure (e.g., missing escrow record) using cHub.finalizeEscrowDeposit and vm.expectRevert for the specific error you expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/src/ChannelHub.sol`:
- Around line 736-747: finalizeEscrowDeposit lacked intent and signature checks
on the home-chain path; enforce candidate.intent ==
StateIntent.FINALIZE_ESCROW_DEPOSIT and call _validateSignatures(channelId,
candidate, metaDef.user, metaDef.approvedSignatureValidators) before invoking
_processHomeChainEscrow to ensure only properly-intended, co-signed transitions
are applied; optionally, to save an SLOAD, pass the already-read
ChannelDefinition (metaDef) into _processHomeChainEscrow instead of re-reading
_channels[channelId].definition.
In `@contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol`:
- Around line 162-168: Test test_revert_ifDisallowedIntent correctly asserts
createChannel reverts for StateIntent.CLOSE, but to improve negative-space
coverage add additional cases: create a table-driven test or separate tests that
iterate through other disallowed StateIntent values (INITIATE_ESCROW_DEPOSIT,
FINALIZE_ESCROW_WITHDRAWAL, INITIATE_MIGRATION, FINALIZE_MIGRATION) and assert
vm.expectRevert(ChannelHub.IncorrectStateIntent.selector) when calling
ChannelHub.createChannel with each; keep the existing test as-is if you prefer
single-case regression.
In `@contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol`:
- Around line 59-73: Update the tests so one explicitly reproduces the exploited
input: in test_revert_homeChain_ifWrongIntent (or add a new regression test)
build a valid co-signed home-chain State with intent
StateIntent.INITIATE_ESCROW_DEPOSIT (similar to escrowState in
ChannelHub_initiateEscrowDeposit.t.sol), then call
cHub.finalizeEscrowDeposit(channelId, bytes32(0), state) and expect a revert
with ChannelHub.IncorrectStateIntent.selector; keep the existing setUp
scaffolding (channel creation and signed init state) so the test uses realistic
data rather than the current DEPOSIT-only inputs that wouldn’t have exercised
the regression.
- Around line 67-73: The test test_revert_nonHomeChain_ifWrongIntent is
redundant because finalizeEscrowDeposit checks intent before checking
home-chain, so passing bytes32(0) vs channelId yields the same require hit;
either merge this with test_revert_ifWrongIntent (remove the duplicate and keep
one test invoking finalizeEscrowDeposit with StateIntent.DEPOSIT and expecting
ChannelHub.IncorrectStateIntent) or replace this test to actually exercise the
non-home-chain path: call finalizeEscrowDeposit with a valid intent (not
StateIntent.DEPOSIT), a non-home channelId (use channelId), and set up ledger
state so the intent passes but the non-home-chain branch runs and then assert
the expected failure (e.g., missing escrow record) using
cHub.finalizeEscrowDeposit and vm.expectRevert for the specific error you
expect.
In `@contracts/test/ChannelHub_units/ChannelHub_initiateEscrowWithdrawal.t.sol`:
- Around line 12-19: Test only covers one wrong intent; add additional
negative-case tests that set State.intent to other invalid values (e.g.,
StateIntent.INITIATE_ESCROW_DEPOSIT, StateIntent.FINALIZE_* variants,
StateIntent.OPERATE) and assert
vm.expectRevert(ChannelHub.IncorrectStateIntent.selector) before calling
cHub.initiateEscrowWithdrawal(def, state). Add separate test functions (e.g.,
test_revert_ifWrongIntent_INITIATE_ESCROW_DEPOSIT,
test_revert_ifWrongIntent_FINALIZE, test_revert_ifWrongIntent_OPERATE) in
ChannelHub_initiateEscrowWithdrawal.t.sol to exercise those intent guards and
keep the existing test as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 224538f2-ea76-49fa-afee-abec9d2f2117
📒 Files selected for processing (18)
contracts/src/ChannelHub.solcontracts/test/ChannelEngine/ChannelEngine_validateTransition.t.solcontracts/test/ChannelHub_challenge/ChannelHub_challengeSessionKeyValidator.t.solcontracts/test/ChannelHub_emitsNodeBalanceUpdated.t.solcontracts/test/ChannelHub_units/ChannelHub_challenge.t.solcontracts/test/ChannelHub_units/ChannelHub_checkpointChannel.t.solcontracts/test/ChannelHub_units/ChannelHub_closeChannel.t.solcontracts/test/ChannelHub_units/ChannelHub_createChannel.t.solcontracts/test/ChannelHub_units/ChannelHub_depositToChannel.t.solcontracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.solcontracts/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.solcontracts/test/ChannelHub_units/ChannelHub_finalizeMigration.t.solcontracts/test/ChannelHub_units/ChannelHub_initiateEscrowDeposit.t.solcontracts/test/ChannelHub_units/ChannelHub_initiateEscrowWithdrawal.t.solcontracts/test/ChannelHub_units/ChannelHub_initiateMigration.t.solcontracts/test/ChannelHub_units/ChannelHub_withdrawFromChannel.t.solcontracts/test/TestChannelHub.solcontracts/test/WadMath.t.sol
💤 Files with no reviewable changes (1)
- contracts/test/ChannelHub_challenge/ChannelHub_challengeSessionKeyValidator.t.sol
M2-I02: fix(clearnode/api): normalize participant addresses consistently across endpoints (#712) M2-L01: docs(protocol): clarify session-key authorization scope and metadata extensibility (#711) M2-M01: fix(clearnode): alert on home channel challenge instead of auto-checkpoint (#710) M2-H01: fix(contracts/ChannelHub): add finalize escrow home chain intent check (#704)
3 participants
Description
finalizeEscrowDepositandfinalizeEscrowWithdrawalroute home-chain calls before checking the candidate state intent. When escrow metadata is absent and the channel is home on the current chain, the functions call_processHomeChainEscrowFinalize, which validates signatures and forwards the candidate toChannelEngine.validateTransition.The intent check is only reached on the non-home path. The home path therefore accepts any valid next signed channel transition supported by ChannelEngine. This breaks the intended escrow deposit flow. The user receives a co-signed
INITIATE_ESCROW_DEPOSITstate so they can submit it on the non-home chain and lock their funds in escrow. The home-chain submission of that same state is intentionally restricted to the node.However, the user can submit that signed initiation state through
finalizeEscrowDepositon the home chain. Since home-chain escrow metadata is empty,_isEscrowDepositHomeChainreturns true and the candidate is processed as a generic channel transition. As a result, the home channel advances to theINITIATE_ESCROW_DEPOSITstate and the node’s matching liquidity is moved from the node vault into the channel’s locked funds, even if the non-home escrow was never created and the user never locked the corresponding funds.Summary by CodeRabbit
Bug Fixes
Tests