Conversation
- Replace unconstrained usernameHash input with username[32] array - Instantiate UsernameHash() component internally for proper hash computation - Update test suite to work with new constrained input format - Prevent arbitrary field element injection into Merkle tree registry Fixes Alien-Protocol#174 - Security vulnerability Finding F-02
…le to wasm32v1-none on every PR
📝 WalkthroughWalkthroughThe PR addresses security finding F-02 by replacing the unconstrained private Changes
Sequence Diagram(s)sequenceDiagram
participant Prover
participant MerkleUpdateProof
participant UsernameHash
participant MerkleTree
Prover->>MerkleUpdateProof: Provide username[32]
MerkleUpdateProof->>UsernameHash: Wire username[32]
UsernameHash->>UsernameHash: Hash username in 8 chunks
UsernameHash->>UsernameHash: Hash 2 intermediate hashes
UsernameHash->>MerkleUpdateProof: Return username_hash
MerkleUpdateProof->>MerkleTree: Use computed hash as leaf
MerkleTree->>MerkleTree: Verify Merkle path (oldRoot → newRoot)
MerkleUpdateProof->>Prover: Return newRoot
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@sheyman546 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zk/circuits/merkle/merkle_update_proof.circom (1)
72-72:⚠️ Potential issue | 🔴 CriticalMultiple
component maindeclarations prevent compilation.The circom compiler found 7 separate
component maindeclarations across the project:
zk/circuits/username_merkle.circom:54zk/circuits/merkle_update.circom:99zk/circuits/username_hash.circom:48zk/circuits/merkle/merkle_non_inclusion.circom:70zk/circuits/merkle/username_leaf_main.circom:18zk/circuits/merkle/merkle_update_proof.circom:72zk/circuits/merkle/merkle_inclusion.circom:37Circom requires exactly one main component per compilation. Either consolidate all circuits into a single main component, or configure the build system to compile each circuit file independently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/circuits/merkle/merkle_update_proof.circom` at line 72, There are multiple `component main` declarations (e.g., MerkleUpdateProof, MerkleInclusion, MerkleNonInclusion, UsernameLeafMain, UsernameHash, UsernameMerkle) which breaks circom compilation; fix by keeping exactly one top-level `component main` and either (a) consolidate the other circuits into that single main by instantiating MerkleUpdateProof, MerkleInclusion, MerkleNonInclusion, UsernameLeafMain, UsernameHash, UsernameMerkle inside one main component and expose the required public signals, or (b) remove/rename the extra `component main` declarations (e.g., rename them to component merkle_update_proof_main, merkle_inclusion_main, etc.) and update your build configuration to compile each circuit file independently so each file has at most one main; ensure the retained main uses the correct public signals (oldRoot/newRoot) for MerkleUpdateProof if you choose consolidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SECURITY_FIX_SUMMARY.md`:
- Line 43: Update the summary line to clarify the private input change: state
that public inputs/outputs (oldRoot, newRoot, out_newRoot) are unchanged but the
private input interface was updated from usernameHash (single field element) to
username[32] (32-element array), and instruct callers/off-chain witness
generators to provide username[32] instead of usernameHash; mention both symbols
usernameHash and username[32] to identify the change.
---
Outside diff comments:
In `@zk/circuits/merkle/merkle_update_proof.circom`:
- Line 72: There are multiple `component main` declarations (e.g.,
MerkleUpdateProof, MerkleInclusion, MerkleNonInclusion, UsernameLeafMain,
UsernameHash, UsernameMerkle) which breaks circom compilation; fix by keeping
exactly one top-level `component main` and either (a) consolidate the other
circuits into that single main by instantiating MerkleUpdateProof,
MerkleInclusion, MerkleNonInclusion, UsernameLeafMain, UsernameHash,
UsernameMerkle inside one main component and expose the required public signals,
or (b) remove/rename the extra `component main` declarations (e.g., rename them
to component merkle_update_proof_main, merkle_inclusion_main, etc.) and update
your build configuration to compile each circuit file independently so each file
has at most one main; ensure the retained main uses the correct public signals
(oldRoot/newRoot) for MerkleUpdateProof if you choose consolidation.
🪄 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: 296ae1f0-e396-4078-8525-68a1df04fec6
📒 Files selected for processing (4)
.github/workflows/checks.ymlSECURITY_FIX_SUMMARY.mdzk/circuits/merkle/merkle_update_proof.circomzk/tests/test_update_proof.js
| - **Constrained Input**: Username must be provided as 32-character array | ||
| - **Canonical Hashing**: Hash is computed internally using `UsernameHash()` | ||
| - **Prevents Arbitrary Data**: Provers cannot inject arbitrary field elements | ||
| - **Maintains Compatibility**: Same public interface and security guarantees |
There was a problem hiding this comment.
Clarify that the private input interface changed.
The statement "Same public interface and security guarantees" is partially misleading. While the public inputs/outputs (oldRoot, newRoot, out_newRoot) remain unchanged, the private input changed from usernameHash (single field element) to username[32] (32-element array). This is a breaking change for any off-chain code that generates witnesses for this circuit.
Consider revising to: "Same public inputs/outputs; private input interface updated (callers must now provide username[32] instead of usernameHash)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY_FIX_SUMMARY.md` at line 43, Update the summary line to clarify the
private input change: state that public inputs/outputs (oldRoot, newRoot,
out_newRoot) are unchanged but the private input interface was updated from
usernameHash (single field element) to username[32] (32-element array), and
instruct callers/off-chain witness generators to provide username[32] instead of
usernameHash; mention both symbols usernameHash and username[32] to identify the
change.
ryzen-xp
left a comment
ryzen-xp
left a comment
There was a problem hiding this comment.
Fix this code and ci in 5 hours , if not then I will unassigned
| run: | | ||
| source $HOME/.cargo/env | ||
| cargo check --verbose No newline at end of file | ||
| cargo check --verbose |
There was a problem hiding this comment.
don't change any thing in this file
2 participants
Key Sections:
Summary - Clear overview of the WASM build enhancement
Problem Statement - Why this change is needed
Technical Details - Implementation specifics
Benefits - Early detection, security, and reliability improvements
Impact Assessment - Performance and development experience effects
Migration Notes - Guidance for developers and DevOps
Highlights:
Zero breaking changes - only adds validation
~30-60 seconds CI overhead - minimal performance impact
Early detection - catches WASM issues before merge
Production safety - ensures all merged code can actually deploy
closes #207
Summary by CodeRabbit
Bug Fixes
Tests
Chores