fix: circuits audits#1703
Open
zahrajavar wants to merge 8 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IF-001 — unconstrained modular division (circuits/lib/src/math/modulo/U64.nr, c6b93ab)
ModU64::div_mod computed a modular inverse via an unconstrained hint but never verified it was correct — mul_mod only proves the given inputs multiply correctly, not that the hint is a true inverse. Fixed by asserting result * b == a (mod m) after the hint, matching the pattern already used in ModU128::div_mod. Added test_u64_div_mod_rejects_non_invertible_divisor, which forces the old code's exact failure mode (composite modulus, non-invertible divisor) and confirms the fix rejects it.
IF-002 — conditional equality check in C7 decoding (circuits/lib/src/core/threshold/decrypted_shares_aggregation.nr, 5a96fa5)
verify_decoding only asserted computed_message == message.coefficients[i] when the prover-declared coefficient was nonzero — a prover could declare any coefficient 0 and skip verification entirely, forging that part of the plaintext output. Made the check unconditional for all coefficients.
IF-003 — decryption proof not bound to real DKG output (3 commits)
SK/ESM cross-phase binding (06865b6, BfvDecryptionVerifier.sol, IDecryptionVerifier.sol): the wrapper checked VK hashes, committee hash, and plaintext hash, but never checked that the proof's party_ids/expected_sk/expected_esm outputs matched CiphernodeRegistry.getDkgAnchors(e3Id) — the address-signed record of who really holds which key share for this E3. Added _verifyDkgAnchors, mirroring the existing SlashingManager._resolveDkgPartyOperator pattern. Added ciphernodeRegistry as a constructor dependency; updated deploy scripts (deployAndSave/bfvDecryptionVerifier.ts, deployInterfold.ts) and both test files (BfvDecryptionVerifier.spec.ts, BfvVkBindingIntegration.spec.ts, the latter validated against a real folded proof fixture) accordingly. New tests cover forged SK/ESM (DkgAnchorMismatch) and unknown party IDs (DkgAnchorNotFound).
Party-ID range (2b80ac1, decryption_aggregator/main.nr): the circuit checked id < N_PARTIES (0-indexed range), but circuit-side party_ids are actually 1-indexed Shamir x-coordinates (x=0 is reserved for the secret itself and must never be a valid share). The old bound admitted the forbidden 0 and rejected the legitimate last party. Fixed via assert_valid_party_id (1 <= id <= N_PARTIES), with 4 regression tests.
Party-ID uniqueness (4e699ad, same file): nothing prevented the same party_id occupying multiple reconstruction slots, letting one real decryption share masquerade as multiple independent contributions and defeat the T+1 honest-party threshold. Added assert_strictly_increasing across the party_id loop; 3 new regression tests.
One remaining issue:
Ciphertext binding: decryption_aggregator still drops the ciphertext commitment C6/c6_fold compute internally — no on-chain proof ties the decrypted plaintext to a specific ciphertext. Blocked on a design decision (bridging the circuit's Poseidon commitment to the on-chain keccak256 e3.ciphertextOutput).