chore: check if this is the first vote or mask#946
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as SDK (vote.ts)
participant Noir as Noir runtime
participant Circuit as Circuit (main.nr)
SDK->>SDK: prepare crispInputs (includes is_first_vote)
SDK->>Noir: execute circuit with crispInputs
Noir->>Circuit: main(is_first_vote, inputs...)
alt is_first_vote == true
Note right of Circuit `#DFF2E1`: first-vote path (no sum validation)
Circuit->>Circuit: return (ct0is, ct1is)
else is_first_vote == false
Note right of Circuit `#FFF3D6`: subsequent-vote path (validate sums)
Circuit->>Circuit: is_vote_zero = check_coefficient_zero(k1) -> bool
Circuit->>Circuit: assert(is_ct_add_valid) then return (sum_ct0is, sum_ct1is)
end
Circuit-->>Noir: returnValue
Noir-->>SDK: { returnValue, proof }
SDK->>SDK: return enhanced result to caller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-08-27T13:49:48.617ZApplied to files:
📚 Learning: 2025-09-22T15:08:29.814ZApplied to files:
📚 Learning: 2025-08-27T14:02:25.412ZApplied to files:
📚 Learning: 2025-10-10T12:56:40.538ZApplied to files:
🧬 Code graph analysis (1)examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
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 |
ctrlc03
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/CRISP/circuits/src/main.nr(4 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPInputValidator.sol(1 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(7 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/circuits/src/main.nr
🧬 Code graph analysis (4)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleProof(39-79)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
MESSAGE(11-11)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (6)
testAddress(32-32)merkleProof(33-33)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)VOTE(14-14)examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
generateMaskVote(215-256)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(164-202)generateProof(258-266)generateProofWithReturnValue(268-276)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)examples/CRISP/packages/crisp-sdk/tests/utils.ts (1)
compareCoefficientsArrays(14-35)
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)examples/CRISP/packages/crisp-sdk/src/types.ts (1)
CRISPCircuitInputs(135-173)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
[high] 208-208: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: build_e3_support_risc0
- GitHub Check: rust_unit
🔇 Additional comments (6)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
65-66: Consistent index source for proof bitsPulling the bitmask from
proof.indexkeeps the derived indices aligned with the proof data—nice cleanup.examples/CRISP/packages/crisp-contracts/contracts/CRISPInputValidator.sol (1)
92-96: Documenting the upcoming first-vote guardAppreciate the inline note capturing the intent to surface the first-vote flag to the verifier—it clarifies the roadmap while keeping logic untouched.
examples/CRISP/packages/crisp-sdk/src/types.ts (2)
171-173: Propagation hook for first-vote flagAdding
is_first_voteto the circuit inputs ensures the new Noir constraint has a clear entry point from the SDK surface.
218-220: SDK params stay in sync with circuit inputsExtending the encrypt/generate params with
slotAddressandisFirstVotekeeps the higher-level API aligned with the circuit’s expectations.examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
26-29: Centralizing the default masking messageElevating the vote message into shared constants avoids hard-coded duplicates elsewhere—good call.
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
31-33: Reusable slot address for test proof generationBreaking the slot address into its own export keeps the test fixtures tidy and easier to reuse.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/CRISP/circuits/src/utils.nr (2)
34-63: Critical security issue from past review has been resolved.The new implementation correctly checks each coefficient individually instead of using sum-based validation, which eliminates the Field arithmetic cancellation vulnerability. The tests now properly assert the return value as well.
Optional: Consider early return for efficiency.
While functionally correct, the current implementation continues checking all coefficients even after finding a non-zero value. For better performance, consider returning
falseimmediately upon finding any non-zero coefficient:pub fn check_coefficient_zero<let D: u32>(k1: Polynomial<D>) -> bool { let HALF_LARGEST_MINIMUM_DEGREE = 28; let HALF_D = D / 2; let START_INDEX_Y = HALF_D - HALF_LARGEST_MINIMUM_DEGREE; let START_INDEX_N = D - HALF_LARGEST_MINIMUM_DEGREE; - let mut res = true; // Loop through all coefficients in the space where we could have a vote // yes part for i in START_INDEX_Y..HALF_D { if k1.coefficients[i] != 0 { - res = false; + return false; } } // no part for i in START_INDEX_N..D { if k1.coefficients[i] != 0 { - res = false; + return false; } } - res + true }
39-43: Optional: Extract shared constants to reduce duplication.The constants
HALF_LARGEST_MINIMUM_DEGREE,HALF_D,START_INDEX_Y, andSTART_INDEX_Nare duplicated betweencheck_coefficient_valuesandcheck_coefficient_zero. Consider extracting them to module-level constants for maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/circuits/src/utils.nr(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-08-27T14:02:25.412Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T14:02:25.412Z
Learning: In CRISP circuits using Greco library, k1 represents a binary value encoded as a polynomial where only one specific coefficient (k1.coefficients[2048 - 1]) carries the actual binary information and needs the constraint `assert(0 == b * (qmt - b))`. Other coefficients in the polynomial are not significant and don't require the binary constraint.
Applied to files:
examples/CRISP/circuits/src/utils.nr
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test_net
- GitHub Check: build_e3_support_dev
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: build_sdk
🔇 Additional comments (2)
examples/CRISP/circuits/src/utils.nr (2)
93-105: LGTM! Test properly validates failure case.The test correctly asserts the return value and places a non-zero coefficient (1 at index 99) within the checked range, which should cause the function to return
false. This addresses the concern from the past review.
107-119: LGTM! Test properly validates success case.The test correctly asserts the return value for a polynomial with all zero coefficients, verifying the expected
truereturn. This addresses the concern from the past review.
cedoor
force-pushed
the
feat/check-if-first-time-voting
branch
from
c95a19c to
7ba9d83
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
236-255: Strip the hardcoded private key from production SDK code.This security issue was previously flagged. The hardcoded Hardhat key should not be embedded in SDK functions; instead, accept signature inputs as parameters so callers control the signing key.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/CRISP/circuits/src/main.nr(4 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPInputValidator.sol(1 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(7 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/CRISP/packages/crisp-sdk/tests/utils.ts
- examples/CRISP/packages/crisp-sdk/tests/constants.ts
- examples/CRISP/packages/crisp-contracts/contracts/CRISPInputValidator.sol
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-08-27T14:02:25.412Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T14:02:25.412Z
Learning: In CRISP circuits using Greco library, k1 represents a binary value encoded as a polynomial where only one specific coefficient (k1.coefficients[2048 - 1]) carries the actual binary information and needs the constraint `assert(0 == b * (qmt - b))`. Other coefficients in the polynomial are not significant and don't require the binary constraint.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.ts
🧬 Code graph analysis (3)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (5)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (5)
testAddress(32-32)MESSAGE(11-11)votingPowerLeaf(31-31)LEAVES(16-27)VOTE(14-14)examples/CRISP/packages/crisp-sdk/src/vote.ts (6)
generateMaskVote(215-256)encodeVote(64-90)encryptVoteAndGenerateCRISPInputs(164-202)generateProof(258-266)verifyProof(278-282)generateProofWithReturnValue(268-276)examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/utils.ts (2)
hashLeaf(18-20)generateMerkleProof(39-79)examples/CRISP/packages/crisp-sdk/tests/utils.ts (1)
compareCoefficientsArrays(14-35)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
MESSAGE(11-11)
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
examples/CRISP/packages/crisp-sdk/src/constants.ts (2)
DEFAULT_BFV_PARAMS(24-24)MESSAGE(29-29)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)examples/CRISP/packages/crisp-sdk/src/types.ts (1)
CRISPCircuitInputs(135-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_e3_support_risc0
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
🔇 Additional comments (6)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
26-29: Message literal centralization looks goodBundling the masking/signature message behind
MESSAGEkeeps the signing flow and tests in sync. 👍examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
65-67: Correct bit derivationUsing
proof.indexto compute the padded index bits keeps the witness consistent even when the caller no longer tracks the raw index separately. Looks good.examples/CRISP/packages/crisp-sdk/src/types.ts (1)
171-173: Type surface matches the circuit changesThe extra
slotAddress/isFirstVotefields and theis_first_votecircuit input keep the TS types aligned with the Noir entrypoint. 👍Also applies to: 218-220
examples/CRISP/circuits/src/utils.nr (1)
34-63: Zero-vote guard helper looks goodIterating the yes/no windows and flipping the flag on the first non-zero coefficient gives the precise “is it really zero?” predicate we needed, and the new tests cover both pass/fail paths. Nice addition.
Also applies to: 94-119
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
164-201: LGTM! Function properly extended for first-vote support.The additions of
slotAddress,isFirstVote, and merkle/signature parameters correctly populate all required circuit inputs.
248-249: Zero merkle proofs for mask votes are correctly handled by the circuit design.After examining the circuit source (
examples/CRISP/circuits/src/main.nr), I can confirm the merkle proof validation is not skipped—it's always performed viaget_merkle_root()at line 79-85. However, the zero proofs (combined with balance='0' and merkle_proof_length='1') intentionally compute a merkle root that won't match the provided merkle_root, causing the circuit to enter the mask vote code path (else block, lines 111-122).This is the intended design: normal votes must pass merkle validation; mask votes deliberately fail it to trigger the alternative branch. The circuit then verifies the vote is zero and validates the ciphertext addition. The
is_first_voteflag controls what gets returned. Tests confirm this works as expected with both isFirstVote states passing verification.
ctrlc03
force-pushed
the
feat/check-if-first-time-voting
branch
from
7ba9d83 to
fb8d8e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
236-240: Hardcoded private key remains a security risk.The function still embeds the Hardhat default private key and derives signatures internally. This is a security footgun — any production code consuming this helper ships a known secret. Please refactor to accept signature inputs (hashed_message, pub_key_x, pub_key_y, signature) from the caller, allowing tests to pass the Hardhat key externally while production code provides real credentials.
Extend the function signature:
export const generateMaskVote = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, slotAddress: string, isFirstVote: boolean, + signatureInputs: NoirSignatureInputs, ): Promise<CRISPCircuitInputs> => {Then replace lines 236-240 with:
- // hardhat default private key - const privateKey = '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' - const account = privateKeyToAccount(privateKey) - const signature = await account.signMessage({ message: MESSAGE }) - const { hashed_message, pub_key_x, pub_key_y, signature: extractedSignature } = await extractSignature(MESSAGE, signature) - return { ...crispInputs, - hashed_message: Array.from(hashed_message).map((b) => b.toString()), - public_key_x: Array.from(pub_key_x).map((b) => b.toString()), - public_key_y: Array.from(pub_key_y).map((b) => b.toString()), - signature: Array.from(extractedSignature).map((b) => b.toString()), + hashed_message: Array.from(signatureInputs.hashed_message).map((b) => b.toString()), + public_key_x: Array.from(signatureInputs.pub_key_x).map((b) => b.toString()), + public_key_y: Array.from(signatureInputs.pub_key_y).map((b) => b.toString()), + signature: Array.from(signatureInputs.signature).map((b) => b.toString()),examples/CRISP/circuits/src/main.nr (1)
132-139: CRITICAL: is_first_vote still not validated against ciphertext state.The circuit trusts the
is_first_voteflag without verifying it matches the actual ciphertext state. An attacker can setis_first_vote = trueon a populated slot (whereprev_ct*/sum_ct*are non-zero), pass a zero mask, and the circuit will return(ct0is, ct1is)— effectively wiping existing votes.The circuit must enforce that
is_first_votecan only be true whensum_ct* == ct*(indicating no prior votes). Please add coefficient-wise equality checks between sum and ct polynomials, assert the equality matches the flag, and only then branch.Apply this pattern:
+ let mut sums_match_ct = true; + for i in 0..1 { + for j in 0..2048 { + if sum_ct0is[i].coefficients[j] != ct0is[i].coefficients[j] { + sums_match_ct = false; + } + if sum_ct1is[i].coefficients[j] != ct1is[i].coefficients[j] { + sums_match_ct = false; + } + } + } + + assert(is_first_vote == sums_match_ct); + if is_first_vote { (ct0is, ct1is) } else { (sum_ct0is, sum_ct1is) }examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
251-267: CRITICAL: Test exercises the exploit path.This test passes a non-empty
encryptedVoteaspreviousCiphertextwhile settingisFirstVote: true, which is precisely the attack vector the circuit should prevent. The test expects the circuit to returnct0is/ct1is, validating the vote-wipeout exploit.Once the circuit enforces
is_first_vote == (sum_ct* == ct*), this test will fail. Please update it to use a genuine empty/zeropreviousCiphertext(e.g.,zkInputsGenerator.encryptVote(publicKey, new BigInt64Array([0n]))as used on line 32) whenisFirstVote: true.
🧹 Nitpick comments (1)
examples/CRISP/circuits/src/utils.nr (1)
35-63: LGTM! Sum-based vulnerability fixed.The coefficient-by-coefficient check correctly addresses the past review concern about Field arithmetic allowing non-zero coefficients to cancel out in a sum. Each coefficient is now validated individually, preventing the exploit.
As an optional micro-optimization, you could short-circuit by returning
falseimmediately when a non-zero coefficient is found, rather than continuing to check all coefficients. However, this is not critical.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/CRISP/circuits/src/main.nr(3 hunks)examples/CRISP/circuits/src/utils.nr(5 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPInputValidator.sol(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(7 hunks)examples/CRISP/packages/crisp-sdk/tests/utils.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/packages/crisp-contracts/contracts/CRISPInputValidator.sol
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.ts
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2024-10-03T23:02:41.732Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-03T23:02:41.732Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/utils.nrexamples/CRISP/circuits/src/main.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/utils.nr
📚 Learning: 2025-08-27T14:02:25.412Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T14:02:25.412Z
Learning: In CRISP circuits using Greco library, k1 represents a binary value encoded as a polynomial where only one specific coefficient (k1.coefficients[2048 - 1]) carries the actual binary information and needs the constraint `assert(0 == b * (qmt - b))`. Other coefficients in the polynomial are not significant and don't require the binary constraint.
Applied to files:
examples/CRISP/circuits/src/utils.nr
🧬 Code graph analysis (2)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
CRISPCircuitInputs(135-173)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/src/vote.ts (4)
generateMaskVote(215-256)generateProof(258-266)encodeVote(64-90)generateProofWithReturnValue(268-276)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)examples/CRISP/packages/crisp-sdk/tests/constants.ts (3)
merkleProof(33-33)testAddress(32-32)VOTE(14-14)examples/CRISP/packages/crisp-sdk/tests/utils.ts (1)
compareCoefficientsArrays(14-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rust_unit
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
🔇 Additional comments (14)
examples/CRISP/circuits/src/utils.nr (1)
93-119: Tests now properly exercise the helper.The assertions on the boolean return values correctly address the past review concern. Both the success and failure cases are now validated.
examples/CRISP/packages/crisp-sdk/tests/utils.ts (2)
7-12: LGTM! Normalization handles hex/decimal differences.The function correctly normalizes coefficient representations for comparison. Since the caller (
compareCoefficientsArrays) validates non-null values before invoking this, the implementation is safe for test utilities.
14-35: Well-structured coefficient comparison utility.The function performs thorough validation (length checks, null guards) and correctly normalizes coefficients before comparison. This is appropriate for test validation of circuit return values.
examples/CRISP/packages/crisp-sdk/src/types.ts (2)
171-172: Type definition correctly added for first-vote flag.The new field follows the existing naming convention (snake_case for circuit inputs) and uses the appropriate boolean type.
219-219: Parameter type correctly extended.The new
isFirstVotefield follows the camelCase convention for SDK parameters and properly propagates to circuit inputs.examples/CRISP/circuits/src/main.nr (1)
55-56: Public parameter added for first-vote detection.The parameter is correctly declared as public, allowing verification of the flag on-chain.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
144-144: Correct usage of isFirstVote parameter.The test properly sets
isFirstVote: falsefor a regular vote following previous ciphertext.
204-231: Test correctly exercises voter proof generation.The test properly generates and verifies a proof for a legitimate voter with
isFirstVote: false.
233-249: Test correctly exercises masking proof generation.The test properly generates and verifies a masking proof with
isFirstVote: false.
269-285: Test correctly validates non-first masking operation.This test properly exercises the non-first-vote path, expecting
sum_ct0is/sum_ct1isin the return value whenisFirstVote: false.examples/CRISP/packages/crisp-sdk/src/vote.ts (4)
161-162: Documentation correctly describes new parameter.The JSDoc properly documents the
isFirstVoteparameter.
164-201: Function correctly propagates isFirstVote.The implementation properly accepts
isFirstVotefrom the params and includes it in the returned CRISP inputs asis_first_vote.
212-213: Documentation correctly describes new parameter.The JSDoc properly documents the
isFirstVoteparameter for the mask vote generation.
268-276: LGTM! Proof generation now implemented.The function now properly generates the proof via
backend.generateProof(witness)and returns bothreturnValueandproof. This addresses the past review concern about returning placeholder proofs.
cedoor
force-pushed
the
feat/check-if-first-time-voting
branch
from
f8e2d75 to
cf977a2
Compare
2 participants
fix #944
merge #938 first
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation