YNU-781: fix(contracts): signature validation registration#544
YNU-781: fix(contracts): signature validation registration#544nksazonov merged 11 commits intofeat/channel-hub-versionfrom
Conversation
|
@coderabbitai review |
Summary of ChangesHello @nksazonov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the signature validation mechanism within the contracts. The previous static channel-specific validator approach has been replaced with a dynamic, per-node validator registry. This change provides nodes with more control over supported signature schemes, enhances cross-chain compatibility by decoupling validator addresses from channel IDs, and streamlines the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
✅ Actions performedReview triggered.
|
|
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 You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors signature validation from a per-channel model to a per-node registry system with 1-byte validator identifiers (0x00 = default, 0x01-0xFF = node-registered). Introduces on-chain validator registration with cross-chain replay protection, removes validator fields from escrow structs, and updates all supporting contracts, validators, and test infrastructure accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Node
participant Hub as ChannelHub
participant Registry as Validator Registry
participant Utils as EcdsaSignatureUtils
Node->>Hub: registerNodeValidator(node, validatorId, validator, signature)
activate Hub
Hub->>Utils: validateEcdsaSigner(message, signature, node)
activate Utils
Utils-->>Hub: bool (valid)
deactivate Utils
alt Signature Valid
Hub->>Registry: Store validator mapping
Hub->>Hub: Emit ValidatorRegistered
Note over Hub: (node, validatorId) -> validator
else Signature Invalid
Hub-->>Node: Revert InvalidSignature
end
deactivate Hub
sequenceDiagram
participant User as User/Relayer
participant Hub as ChannelHub
participant Validator as Validator (via Registry)
participant Utils as EcdsaSignatureUtils
User->>Hub: submit signature (first byte = validatorId)
activate Hub
Hub->>Hub: _selectValidatorAndFormatSig(signature, node)
Note over Hub: Extract validatorId from byte 0<br/>Lookup validator from registry<br/>Strip first byte from signature
activate Validator
Hub->>Validator: validateSignature(channelId, signingData, sig, participant)
deactivate Validator
activate Utils
Validator->>Utils: validateEcdsaSigner(message, signature, participant)
Utils-->>Validator: bool (valid)
deactivate Utils
alt Signature Valid
Hub->>Hub: Process transaction
else Invalid
Hub-->>User: Revert InvalidSignature
end
deactivate Hub
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the signature validation mechanism, moving from a channel-specific validator to a more flexible node-based validator registry, which aims to enhance cross-chain compatibility and modularity. However, this introduces a critical vulnerability: by allowing a node to control the validator, it can unilaterally bypass user signatures, effectively gaining full control over user funds within any channel. It is strongly recommended that this change be reverted, and the validator remain part of the immutable channel definition or require explicit approval from all participants. Additionally, consider reducing code duplication in ChannelHub.sol, and note that the reduction in foundry.toml's optimizer_runs may slightly increase gas costs.
contracts/src/ChannelHub.sol
Outdated
| _selectValidatorAndFormatSig(initState.userSig, def.node); | ||
| _validateSignature(channelId, initState, userSigData, def.user, userValidator); | ||
| (ISignatureValidator nodeValidator, bytes calldata nodeSigData) = | ||
| _selectValidatorAndFormatSig(initState.nodeSig, def.node); |
There was a problem hiding this comment.
So, this way node also supports now all registered validators?
There was a problem hiding this comment.
As we discussed, we cannot disable previously registered ones on the contract. But the node can simply stop signing new states signed with the validator it no longer supports
* docs(contracts): update signature validator approach * refactor(contracts): reorg BaseValidator into lib * docs(contracts): add chainId to val registration msg * feat(contracts): add sig val registration * test(contracts): sig val registration * build(contracts): reduce number of optimizer runs for Custody * docs(contracts): add per-channel validator bitmask * feat(contracts): add per-channel sig val bitmask * refactor(contracts): remove verifyChallengerSig * fix(contracts/Types): rename default val to sig val * chore(contracts): rename states to ledgers, run forge fmt
3 participants
Summary by CodeRabbit
New Features
Documentation
Performance