Skip to content

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

Merged
nksazonov merged 1 commit into
fix/audit-findingsfrom
fix/audit-h-l07
Apr 10, 2026
Merged

H-L07: fix(contracts/ChannelHub): restrict createChannel to non-existing channels#668
nksazonov merged 1 commit into
fix/audit-findingsfrom
fix/audit-h-l07

Conversation

@nksazonov
Copy link
Copy Markdown
Contributor

@nksazonov nksazonov commented Apr 9, 2026

Description

The createChannel(..) function unconditionally emits ChannelCreated(..) event at line 491 even when operating on existing channels. The function allows DEPOSIT, WITHDRAW, and OPERATE intents on channels with status OPERATING, DISPUTED, or MIGRATING_IN (not just VOID), as confirmed in the intent-specific validation functions. When called on an existing channel to deposit additional funds or perform operations, it still emits ChannelCreated(channelId, def.user, def.node, def, initState), misleading off-chain indexers and UIs. The channel definition is only stored if status == VOID (line 1064-1066), confirming that subsequent calls are updates, not creations. This behavior is inconsistent with the protocol's established pattern seen in EscrowDepositEngine.sol:165 and EscrowWithdrawalEngine.sol:161, which explicitly check require(ctx.status == EscrowStatus.VOID, EscrowAlreadyExists()) to prevent operations on existing escrows.

Impact

Off-chain indexers listening for ChannelCreated events will incorrectly count the same channel multiple times, leading to inflated channel creation metrics. UIs that display channels based on ChannelCreated events may show duplicate channel listings, confusing users. Analytics dashboards and monitoring systems will report incorrect data about channel creation activity. While on-chain state remains correct (the definition is only stored once when status == VOID), the misleading events create data integrity issues for off-chain systems.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced channel creation validation with early status verification to prevent invalid state transitions.
  • Tests

    • Expanded test coverage for channel creation scenarios, including success cases for multiple operation intents and revert conditions for invalid channel states.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 9, 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: c316bfc7-55df-40bc-b2c7-fcac63b7576d

📥 Commits

Reviewing files that changed from the base of the PR and between c2310fe and 950c516.

📒 Files selected for processing (2)
  • contracts/src/ChannelHub.sol
  • contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol

📝 Walkthrough

Walkthrough

The createChannel() function now performs an early check to ensure the channel's stored status is ChannelStatus.VOID before proceeding, reverting with IncorrectChannelStatus() if the channel already exists. Test coverage is expanded with fixtures, helper functions, and new test cases validating this behavior across different intent types.

Changes

Cohort / File(s) Summary
Channel Creation Validation
contracts/src/ChannelHub.sol
Added early stored status validation in createChannel() to enforce that the channel must not already exist (status must be VOID) before proceeding with creation.
Test Coverage Expansion
contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol
Added persistent test fixtures (channelId, initialDepositState) and helper function _createDefaultChannel(). Expanded test coverage with three success cases for DEPOSIT, WITHDRAW, and OPERATE intents, and three revert cases asserting IncorrectChannelStatus when attempting to recreate existing channels.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • dimast-x
  • philanton
  • ihsraham

Poem

🐰 A channel must be void before it's born,
No duplicates allowed, no status torn,
With DEPOSIT, WITHDRAW, or OPERATE's call,
The validation stands tall—first check of all! 🎯

🚥 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 clearly and specifically summarizes the main change: restricting the createChannel function to only work with non-existing channels, which is the core issue being fixed.
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-l07

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 Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 9, 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.

@nksazonov nksazonov merged commit 359d58a into fix/audit-findings Apr 10, 2026
3 checks passed
@nksazonov nksazonov deleted the fix/audit-h-l07 branch April 10, 2026 08:58
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