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

[nksazonov]

Description

The functions withdrawFromChannel(...), checkpointChannel(...), and createChannel(...) are marked payable but fail to validate msg.value for operations that should never accept ETH.

Specifically:

1. withdrawFromChannel(...) only accepts WITHDRAW intent, which always has userFundsDelta < 0 (funds flowing OUT to user), yet is marked payable;
2. checkpointChannel(...) only accepts OPERATE intent, which always has userFundsDelta = 0 (no user fund movement), yet is marked payable;
3. createChannel(...) accepts multiple intents (DEPOSIT, WITHDRAW, OPERATE) but only validates msg.value inside _pullFunds(...), which is only called when userFundsDelta > 0. For WITHDRAW (userFundsDelta < 0) and OPERATE (userFundsDelta = 0) intents, msg.value is never validated.

Any ETH sent with these non-deposit operations is added to the contract balance, but not tracked in any accounting variable (_nodeBalances, lockedFunds, or _reclaims), making it permanently stuck with no recovery mechanism.

Impact

This is a fail-unsafe design where the protocol silently accepts invalid input instead of rejecting it. The payable modifier on withdrawFromChannel(..) and checkpointChannel(..) creates a misleading interface that suggests these functions can accept ETH when they logically never should. Users who send ETH with these operations (whether through direct calls, contract integration bugs, SDK errors, or UI mistakes) will permanently lose those funds. While this requires user error to trigger and provides no direct benefit to attackers, it represents a protocol design flaw where the declared interface (payable) doesn't match the actual behavior (never processes ETH).

Summary by CodeRabbit

• Bug Fixes

  • Tightened ETH handling for channel operations to prevent unintended native asset transfers. Channel withdrawals, checkpoints, and closures now reject ETH. Channel creation stricter validates ETH acceptance based on operation intent.

• Tests

  • Added comprehensive test coverage for ETH rejection behavior across channel operations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 205859ee-57f1-4db5-a9a0-7b809904c70c

📥 Commits

Reviewing files that changed from the base of the PR and between 38d2ad1 and be1d89d.

📒 Files selected for processing (5)

• contracts/src/ChannelHub.sol
• contracts/test/ChannelHub_units/ChannelHub_checkpointChannel.t.sol
• contracts/test/ChannelHub_units/ChannelHub_closeChannel.t.sol
• contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol
• contracts/test/ChannelHub_units/ChannelHub_withdrawFromChannel.t.sol

---

📝 Walkthrough

Walkthrough

This PR strengthens ETH value handling in the ChannelHub contract by enforcing stricter validation rules. It adds a check in createChannel() rejecting ETH when intent is not DEPOSIT, and removes payable modifiers from withdrawFromChannel, checkpointChannel, and closeChannel to prevent accidental ETH transfers. Corresponding unit tests validate the new restrictions.

Changes

Cohort / File(s)	Summary
ChannelHub ETH Validation contracts/src/ChannelHub.sol	Added msg.value == 0 enforcement in createChannel() for non-DEPOSIT intents, reverting with IncorrectValue(). Removed payable modifier from withdrawFromChannel, checkpointChannel, and closeChannel.
Unit Tests for ETH Rejection contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol, ChannelHub_withdrawFromChannel.t.sol, ChannelHub_checkpointChannel.t.sol, ChannelHub_closeChannel.t.sol	Added four new test contracts with test_revert_ifETHSent() test functions verifying ETH is rejected when sent to channel operations, each using low-level calls with value: 1 and asserting failure with empty revert data.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• YNU-797: feat(contracts/ChannelHub): add NodeBalanceUpdated event #621: Modifies the same ChannelHub functions (createChannel, withdrawFromChannel, checkpointChannel, closeChannel), indicating related ETH handling changes.
• YNU-714: feat(contracts-new): add non-initial channel creation #510: Updates channel creation intent handling in ChannelHub's createChannel function, overlapping with the intent validation logic introduced here.

Suggested reviewers

• ihsraham
• philanton
• dimast-x

Poem

🐰 A rabbit hops through contracts with care,
No stray ETH floating in the air!
Payable stripped, validation tight,
Channel operations now done right! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly reflects the main change: removing payable modifiers from three ChannelHub methods and clarifying ETH handling in createChannel, which matches the core objective of preventing untracked ETH acceptance.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/audit-h-l06

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ 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.