M-H01: feat(contracts/ChannelHub): restrict to one node#649
M-H01: feat(contracts/ChannelHub): restrict to one node#649nksazonov wants to merge 8 commits intofix/audit-findingsfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughBinds each ChannelHub deployment to an immutable NODE, enforces exact node equality in channel definitions, timestamps node-registered validators with an activation delay, updates deployment script and tests, and adds security/design docs describing the bootstrap vulnerability and mitigation options. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChannelHub
participant Node
participant ValidatorRegistry
participant BlockTime as BlockTime
Client->>ChannelHub: createChannel(def where def.node == NODE, userSig, approvedSignatureValidators)
ChannelHub->>ValidatorRegistry: extract validatorId from userSig + approvedSignatureValidators
ValidatorRegistry->>ChannelHub: return (validatorAddr, registeredAt)
ChannelHub->>BlockTime: check block.timestamp >= registeredAt + VALIDATOR_ACTIVATION_DELAY
alt validator active
ChannelHub->>validatorAddr: validate(userSig, payload)
validatorAddr-->>ChannelHub: validation result
ChannelHub-->>Client: channel created / funds processed
else validator inactive
ChannelHub-->>Client: revert ValidatorNotActive(validator, id, activatesAt)
end
Note right of ChannelHub: if def.node != NODE -> revert IncorrectNode()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
nksazonov
force-pushed
the
fix/audit-m-h01
branch
from
a240db9 to
f6f76e1
Compare
|
@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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/initial-user-sig-validation.md (2)
454-465: Clarify session key support for Option H in comparison table.Line 456 (Option H row) shows "✓" for "Session key (createChannel)", while all validator-restricting options (A-G) show "✗". This is technically correct—Option H doesn't restrict validators, so session keys remain functional—but potentially misleading because they remain insecure (the bound node can still forge signatures using a malicious validator).
Readers might interpret "✓" as "secure session key support" rather than "session keys are not blocked (but vulnerability persists within trust boundary)".
📝 Suggested clarification
Consider adding a footnote or marking this cell differently:
-| **H** (per-node hub) | ✓ (cross-node only) | ✓ (cross-node only) | — | ✓ | ✓ | ✓ | No | **✗** | +| **H** (per-node hub) | ✓ (cross-node only) | ✓ (cross-node only) | — | ✓ (¹) | ✓ | ✓ | No | **✗** |And add below the table:
(¹) Session keys remain functional but insecure within the trust boundary—the bound node can still register a malicious validator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/initial-user-sig-validation.md` around lines 454 - 465, Update the Options Comparison Table so Option H's "Session key (createChannel)" cell is explicitly qualified (e.g., change the ✓ to "✓¹" or similar) and add the suggested footnote text below the table clarifying that session keys remain functional but insecure within the trust boundary (e.g., "(¹) Session keys remain functional but insecure within the trust boundary—the bound node can still register a malicious validator."). Reference the "Option H" row and the "Session key (createChannel)" column when making the change.
34-51: Constraint 5 conflicts with implemented Option H.Line 47 lists "Extensible without ChannelHub redeployment" as a constraint, but the implemented solution (Option H, lines 399-451) explicitly requires one deployment per node, creating operational fragmentation. This tension is acknowledged in Option H's "Cons" section (lines 447-450), so it appears the team accepted this trade-off for simplicity. However, readers might expect constraints to be hard requirements rather than preferences.
📝 Suggested clarification
Consider revising the constraints header to distinguish between hard requirements and desirable properties, or add a note that Constraint 5 is relaxed in the chosen solution:
## Constraints on the Fix -1. **No additional user interaction.** The existing interaction may change format, but an +1. **No additional user interaction (hard requirement).** The existing interaction may change format, but an extra signing step or extra on-chain transaction is not acceptable. ... -5. **Extensible without ChannelHub redeployment.** New wallet formats (ERC-4337, future +5. **Extensible without ChannelHub redeployment (desirable).** New wallet formats (ERC-4337, future schemes) must be addable without requiring users to migrate to a new deployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/initial-user-sig-validation.md` around lines 34 - 51, The constraints list currently presents "Extensible without ChannelHub redeployment" (Constraint 5) as a hard requirement but Option H explicitly requires per-node ChannelHub deployments, so update the document to clarify requirements: either split the constraints section into "Hard requirements" and "Desirable properties" or add a short explicit note referencing Option H and ChannelHub that Constraint 5 is relaxed for the chosen solution (Option H) and explain this tradeoff; ensure you mention Constraint 5, Option H, and ChannelHub so readers can locate and reconcile the tension.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/initial-user-sig-validation.md`:
- Around line 268-274: The Markdown table has a mismatched separator row (4
cells) vs. the header (3 columns) causing formatting break; update the separator
from "|---|---|---|---|" to "|---|---|---|" and ensure each subsequent data row
(e.g., the row starting "Who adds the malicious validator?") has exactly three
pipe-separated cells to match the header.
---
Nitpick comments:
In `@contracts/initial-user-sig-validation.md`:
- Around line 454-465: Update the Options Comparison Table so Option H's
"Session key (createChannel)" cell is explicitly qualified (e.g., change the ✓
to "✓¹" or similar) and add the suggested footnote text below the table
clarifying that session keys remain functional but insecure within the trust
boundary (e.g., "(¹) Session keys remain functional but insecure within the
trust boundary—the bound node can still register a malicious validator.").
Reference the "Option H" row and the "Session key (createChannel)" column when
making the change.
- Around line 34-51: The constraints list currently presents "Extensible without
ChannelHub redeployment" (Constraint 5) as a hard requirement but Option H
explicitly requires per-node ChannelHub deployments, so update the document to
clarify requirements: either split the constraints section into "Hard
requirements" and "Desirable properties" or add a short explicit note
referencing Option H and ChannelHub that Constraint 5 is relaxed for the chosen
solution (Option H) and explain this tradeoff; ensure you mention Constraint 5,
Option H, and ChannelHub so readers can locate and reconcile the tension.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d20dff94-645a-49c4-b029-93fee9ce1933
📒 Files selected for processing (9)
contracts/SECURITY.mdcontracts/initial-user-sig-validation.mdcontracts/script/DeployChannelHub.s.solcontracts/src/ChannelHub.solcontracts/test/ChannelHub_Base.t.solcontracts/test/ChannelHub_claimFunds.t.solcontracts/test/ChannelHub_pushFunds.t.solcontracts/test/TestChannelHub.solprotocol-description.md
nksazonov
force-pushed
the
fix/audit-m-h01
branch
from
6d0ed2b to
e09ce12
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/initial-user-sig-validation.md (1)
268-270:⚠️ Potential issue | 🟡 MinorThe Option F comparison table is still malformed.
The header has 3 columns, but the separator row still has 4 cells, so this block renders incorrectly.
🔧 Proposed fix
| | Original attack | Option F attack | -|---|---|---|---| +|---|---|---| | Who adds the malicious validator? | Any node (permissionless) | Only `bootstrapAdmin` (governance) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/initial-user-sig-validation.md` around lines 268 - 270, The markdown table header in the comparison block is mismatched: the header defines three columns but the separator row uses four cells; update the separator row to have exactly three pipes/cells to match the header (replace the current "|---|---|---|---|" with a three-cell divider) so the table with "Original attack" and "Option F attack" renders correctly; ensure the separator aligns with the header columns and the cells beneath (e.g., the row containing "Who adds the malicious validator?").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/SECURITY.md`:
- Around line 253-271: Update the earlier validator-selection claim so it
explicitly scopes the protection to already-created channels or previously
agreed channel definitions: change the sentence that says the
`approvedSignatureValidators` bitmask prevents malicious nodes from forging user
signatures to instead state that it prevents forging on existing channels (i.e.,
where `channelId` embeds the agreed `approvedSignatureValidators`), and add a
brief clause noting the bootstrap exception at
`createChannel`/`ChannelDefinition` where the bitmask arrives in calldata and
does not provide protection; also mention `closeChannel` as subject to the same
bootstrap risk.
In `@contracts/src/ChannelHub.sol`:
- Line 183: registerNodeValidator currently allows arbitrary node keys into
_nodeValidatorRegistry and initiateEscrowDeposit/initiateEscrowWithdrawal
validate against def.node/def.approvedSignatureValidators on non-home paths,
leaking foreign-node registries; fix by enforcing the single-node invariant: in
registerNodeValidator and any code paths that write to _nodeValidatorRegistry
ensure def.node == NODE (or call _requireValidDefinition(def)) before
persisting, and in initiateEscrowDeposit/initiateEscrowWithdrawal add a guard
that if !_isChannelHomeChain(channelId) then require def.node == NODE (or call
_requireValidDefinition) before using def.approvedSignatureValidators to
validate signatures or creating escrow records so foreign-node validator
registries cannot be materialized.
---
Duplicate comments:
In `@contracts/initial-user-sig-validation.md`:
- Around line 268-270: The markdown table header in the comparison block is
mismatched: the header defines three columns but the separator row uses four
cells; update the separator row to have exactly three pipes/cells to match the
header (replace the current "|---|---|---|---|" with a three-cell divider) so
the table with "Original attack" and "Option F attack" renders correctly; ensure
the separator aligns with the header columns and the cells beneath (e.g., the
row containing "Who adds the malicious validator?").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2363ed03-6dd9-436f-b089-dec4ebcc73d6
📒 Files selected for processing (11)
contracts/SECURITY.mdcontracts/foundry.tomlcontracts/initial-user-sig-validation.mdcontracts/script/DeployChannelHub.s.solcontracts/src/ChannelHub.solcontracts/test/ChannelHub_Base.t.solcontracts/test/ChannelHub_claimFunds.t.solcontracts/test/ChannelHub_pushFunds.t.solcontracts/test/ChannelHub_sigValidator.t.solcontracts/test/TestChannelHub.solprotocol-description.md
✅ Files skipped from review due to trivial changes (3)
- contracts/foundry.toml
- contracts/test/ChannelHub_claimFunds.t.sol
- protocol-description.md
🚧 Files skipped from review as they are similar to previous changes (4)
- contracts/test/ChannelHub_pushFunds.t.sol
- contracts/test/ChannelHub_Base.t.sol
- contracts/test/TestChannelHub.sol
- contracts/script/DeployChannelHub.s.sol
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/initial-user-sig-validation.md (1)
434-434: Tighten wording for concision.“has the ability to” can be simplified to “can” without changing meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/initial-user-sig-validation.md` at line 434, Replace the phrase "has the ability to" with the more concise "can" in the sentence under Option H (the line starting "Within Option H the bound node still has the ability to register a malicious validator and") so it reads "Within Option H the bound node still can register a malicious validator and" (adjust surrounding punctuation/grammar if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/initial-user-sig-validation.md`:
- Around line 418-424: The Option H table is misleadingly scored as fully `✓`
for “Node-independent” and “createChannel admin-proof” despite the text stating
it's only a threat-model shift; update the table cells for Option H to reflect
partial mitigation (e.g., change those cells to “Partial (cross-node only)” or
similar) so readers don't over-read it as a complete validator-level fix; ensure
you update any other occurrences of those same labels (the Option H row and the
related summary at or near the mention of ChannelDefinition and
approvedSignatureValidators) so the table and prose are consistent.
---
Nitpick comments:
In `@contracts/initial-user-sig-validation.md`:
- Line 434: Replace the phrase "has the ability to" with the more concise "can"
in the sentence under Option H (the line starting "Within Option H the bound
node still has the ability to register a malicious validator and") so it reads
"Within Option H the bound node still can register a malicious validator and"
(adjust surrounding punctuation/grammar if needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e599af2c-64a3-473f-966e-1e45644db700
📒 Files selected for processing (1)
contracts/initial-user-sig-validation.md
|
This PR should also remove core related to multi-node support. However, it is unclear whether it should be removed from events as well. @philanton , WDYT? |
There was a problem hiding this comment.
I'm not against validator activation delay, but it seems to be just a delay, so until there is a clear incident response mechanism, unregistering this validator, this design doesn't seem to be complete.
The only entity with the ability to register a validator is node signer, and if it is compromised, there is no way to stop the exploit right now.
For golang side, the only thing that I see required for update is ABI bindings, there is not really something related to multi-node setup right now.
nksazonov
force-pushed
the
fix/audit-m-h01
branch
from
6b5c139 to
aca69d4
Compare
3 participants
Description
createChannel()accepts aChannelDefinitionfrom calldata and passesdef.approvedSignatureValidatorsinto_validateSignatures()._validateSignatures()calls_extractValidator()which selects the validator based on the first byte ofuserSig/nodeSigand only checks that the chosenvalidatorIdis set inapprovedSignatureValidatorsbefore invoking it. A node can register a maliciousISignatureValidatorviaregisterNodeValidator()that always returnsVALIDATION_SUCCESS, then setapprovedSignatureValidatorsin calldata to include that validator. This letscreateChannel()succeed with an arbitraryuserSig, so the channel can be created without user consent. The same bypass applies tocloseChannel()by providing another fakeuserSig, enabling the node to close the channel and transfer locked funds to itself.Impact
Unauthorized channel creation and theft of user funds up to the user’s
ERC20allowance/balance. This violates the protocol requirement: “Every state update requires valid signatures from all required participants. No participant can unilaterally change the state.”Summary by CodeRabbit
Security
Documentation
Tests
Chores