[PQ-Accounts] SHAKE hint and direct variants - #1730
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds Falcon-512 SHAKE account variants, pure Cairo hashing, packing, modular arithmetic, generic and optimized NTT implementations, account interfaces, validation logic, comprehensive tests, and release-mode CI coverage. ChangesFalcon-512 account support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Account
participant Verifier
participant SHAKE256
participant NTT
Account->>Verifier: submit transaction signature
Verifier->>SHAKE256: derive message point from message and salt
Verifier->>NTT: transform and verify Falcon polynomial values
NTT-->>Verifier: return norm and product checks
Verifier-->>Account: return validation status
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🧪 Cairo Contract Size Benchmark DiffBYTECODE SIZE (felts) (limit: 81,920 felts)No changes in felts. SIERRA CONTRACT CLASS SIZE (bytes) (limit: 4,089,446 bytes)No changes in bytes. This comment was generated automatically from benchmark diffs. |
![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
🧹 Nitpick comments (7)
packages/account/README.md (1)
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider listing the new Falcon interfaces in the sections below.
The new bullets describe the two Falcon accounts. The
Interfacessection still lists onlyISRC6andISRC9_V2. The tests useIFeltArrayPublicKeyandIFeltArrayDeployable, which are new public interfaces. Add them so readers can find the felt-array key and deploy entrypoints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/README.md` around lines 13 - 22, Update the README’s Interfaces section to list the new public interfaces IFeltArrayPublicKey and IFeltArrayDeployable alongside ISRC6 and ISRC9_V2, reflecting the felt-array key and deploy entrypoints used by the Falcon accounts.packages/account/src/tests/test_falcon_512.cairo (1)
375-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBare
#[should_panic]in four negative tests. These tests pass on any panic, including a panic from an unrelated cause such as calldata deserialization or a future assertion added earlier in the call path. The rest of both files already pins the panic reason with#[should_panic(expected: ...)], so these four sites are the inconsistent ones.
packages/account/src/tests/test_falcon_512.cairo#L375-L391: add the expected panic message totest_constructor_rejects_wrong_public_key_lengthandtest_constructor_rejects_noncanonical_public_key, so each test proves the length check and the canonicality check specifically.packages/account/src/tests/test_falcon_512_ntt.cairo#L143-L153: add the expected panic message totest_forward_roots_reject_unsupported_degreeandtest_inverse_roots_reject_unsupported_degree, using the real message emitted byget_even_roots_feltandget_scaled_inv_roots.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/src/tests/test_falcon_512.cairo` around lines 375 - 391, The four negative tests currently accept any panic; pin each to the specific expected panic message. In packages/account/src/tests/test_falcon_512.cairo:375-391, update test_constructor_rejects_wrong_public_key_length and test_constructor_rejects_noncanonical_public_key with the messages emitted by the public-key length and canonicality checks. In packages/account/src/tests/test_falcon_512_ntt.cairo:143-153, update test_forward_roots_reject_unsupported_degree and test_inverse_roots_reject_unsupported_degree with the actual messages from get_even_roots_felt and get_scaled_inv_roots.packages/account/src/falcon_512.cairo (1)
24-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the slice offsets from named constants and share the decoding.
The literals
29and31in the slices repeat the layout thatPUBLIC_KEY_FELTS,SIGNATURE_FELTSandDIRECT_SIGNATURE_FELTSalready encode. The twoverifyimplementations also duplicate the public key,s1and salt decoding. If the polynomial slot count changes, the length gate and the slices can desynchronize.♻️ Proposed refactor
+/// Felts per packed Z_q polynomial (28 full slots + 1 partial slot). +pub(crate) const POLY_FELTS: u32 = packing::PACKED_SLOTS; +/// Felts holding the 40-byte salt. +const SALT_FELTS: u32 = 2; + /// Number of felts in a packed Falcon-512 public key. -pub(crate) const PUBLIC_KEY_FELTS: u32 = 29; +pub(crate) const PUBLIC_KEY_FELTS: u32 = POLY_FELTS; /// Number of felts in a Falcon-512 signature carrying a product hint. -pub(crate) const SIGNATURE_FELTS: u32 = 60; +pub(crate) const SIGNATURE_FELTS: u32 = DIRECT_SIGNATURE_FELTS + POLY_FELTS; /// Number of felts in a hint-free Falcon-512 signature. -pub(crate) const DIRECT_SIGNATURE_FELTS: u32 = 31; +pub(crate) const DIRECT_SIGNATURE_FELTS: u32 = POLY_FELTS + SALT_FELTS;Then reuse one helper for the shared prefix:
/// Decodes `h_ntt`, `s1` and the message point shared by both signature layouts. fn decode_common( message_hash: felt252, public_key: Span<felt252>, signature: Span<felt252>, ) -> Option<(Array<u16>, Array<u16>, Array<u16>)> { let h_ntt = packing::unpack_512_u16(public_key)?; let s1 = packing::unpack_512_u16(signature.slice(0, POLY_FELTS))?; let message_point = hash_to_point_shake_512( message_hash, *signature.at(POLY_FELTS), *signature.at(POLY_FELTS + 1), )?; Some((h_ntt, s1, message_point)) }and slice the hint with
signature.slice(DIRECT_SIGNATURE_FELTS, POLY_FELTS).Also applies to: 36-52, 76-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/src/falcon_512.cairo` around lines 24 - 31, Derive all signature and public-key slice offsets from the existing layout constants, including the hint slice, instead of hardcoded 29 and 31 values. Add a shared decode_common helper for public-key, s1, and message-point decoding, then update both verify implementations to reuse it while preserving their distinct signature-layout handling.packages/account/src/falcon_512/hashing/shake256.cairo (1)
156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate
shake256behind#[cfg(test)]or add a non-test caller.
shake256has no non-test caller. Production code imports onlykeccak_f1600, and all calls are in the test module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/src/falcon_512/hashing/shake256.cairo` around lines 156 - 157, Update the `shake256` function visibility so it is compiled only for tests using the project’s test configuration attribute, unless a real production caller is added. Keep `keccak_f1600` and the existing test calls unchanged.packages/account/src/falcon_512/ntt/engine.cairo (1)
357-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset
bitswithboundin the last-level reduction.The branch resets
boundbut leavesbitsstale.bitsis not read after this point, so behavior is correct today. A future change that readsbitsafter the last level would use a wrong value. Keep the two tracked variables consistent.♻️ Proposed change
if bits + growth_bits > SAFE_BITS { cur = reduce_pass(cur.span(), q_nz); + bits = qbits; bound = q_felt; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/src/falcon_512/ntt/engine.cairo` around lines 357 - 360, Update the last-level reduction branch in the NTT engine to reset bits consistently when bound is reset after reduce_pass. Preserve the existing reduction flow and ensure both tracked values reflect the new bound for any subsequent use.packages/account/src/falcon_512/ntt/falcon512.cairo (1)
34-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or guard the degree precondition.
config_for_degreeaccepts anyn. Ifnis not a power of two in[4, 512],config_with_permcallsget_even_roots_feltwith an unsupported size and panics with "no root table for degree". The failure message does not name the caller argument. Add an explicit assertion so misuse fails with a clear reason.♻️ Proposed guard
pub fn config_for_degree(n: u32, levels: u32) -> NttConfig { + assert!(n >= 4 && n <= 512, "config_for_degree: unsupported degree"); let mut perm: Array<u16> = array![];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/src/falcon_512/ntt/falcon512.cairo` around lines 34 - 50, Add an explicit precondition at the start of config_for_degree validating that n is a power of two within the supported range [4, 512], and assert with a message that includes the supplied n when invalid. Keep the existing permutation construction and config_with_perm flow unchanged for valid degrees.packages/account/src/falcon_512/account.cairo (1)
155-178: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffRead the public key once per validation call.
validate_transactionandis_valid_signatureboth callread_public_key, which performs one storage read per stored felt. The Falcon-512 key is 29 felts, so each validation costs 29 storage reads plus the vector length read.The key is immutable after the constructor. Consider caching it in a single packed storage layout, or document the cost so integrators can budget for it. This is an observation about gas cost, not a correctness defect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account/src/falcon_512/account.cairo` around lines 155 - 178, Reduce repeated public-key storage reads across validate_transaction and is_valid_signature by caching the immutable Falcon-512 key in a single packed storage representation initialized by the constructor, then have read_public_key reuse that cached value. If the existing storage model prevents caching, document the 29-felt plus length-read cost for integrators instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/account/src/falcon_512/account.cairo`:
- Around line 141-152: Guard the public initializer method against repeated
calls by checking that self.public_key is empty before validating or appending
the key. Add and use a dedicated initialization error constant alongside the
existing Errors constants, ensuring a second call fails without mutating the
stored key.
In `@packages/account/src/tests/falcon_512_fixture.cairo`:
- Around line 8-11: Correct the documentation comment above msg() to describe
1222479111879746474823 as the encoded benchmark message “BENCH_MSG,” not its
hash; leave the function and returned value unchanged.
In `@packages/account/src/tests/test_falcon_512_ntt.cairo`:
- Around line 136-141: Update test_intt_reduces_before_last_level to pass the
nonzero input ntt([1, 2, 3, 4]) to intt, using input_bits = 111 and input_bound
= 12289 with the existing degree-4 configuration. Preserve the assertion that
the recovered values equal the original [1, 2, 3, 4] values.
---
Nitpick comments:
In `@packages/account/README.md`:
- Around line 13-22: Update the README’s Interfaces section to list the new
public interfaces IFeltArrayPublicKey and IFeltArrayDeployable alongside ISRC6
and ISRC9_V2, reflecting the felt-array key and deploy entrypoints used by the
Falcon accounts.
In `@packages/account/src/falcon_512.cairo`:
- Around line 24-31: Derive all signature and public-key slice offsets from the
existing layout constants, including the hint slice, instead of hardcoded 29 and
31 values. Add a shared decode_common helper for public-key, s1, and
message-point decoding, then update both verify implementations to reuse it
while preserving their distinct signature-layout handling.
In `@packages/account/src/falcon_512/account.cairo`:
- Around line 155-178: Reduce repeated public-key storage reads across
validate_transaction and is_valid_signature by caching the immutable Falcon-512
key in a single packed storage representation initialized by the constructor,
then have read_public_key reuse that cached value. If the existing storage model
prevents caching, document the 29-felt plus length-read cost for integrators
instead.
In `@packages/account/src/falcon_512/hashing/shake256.cairo`:
- Around line 156-157: Update the `shake256` function visibility so it is
compiled only for tests using the project’s test configuration attribute, unless
a real production caller is added. Keep `keccak_f1600` and the existing test
calls unchanged.
In `@packages/account/src/falcon_512/ntt/engine.cairo`:
- Around line 357-360: Update the last-level reduction branch in the NTT engine
to reset bits consistently when bound is reset after reduce_pass. Preserve the
existing reduction flow and ensure both tracked values reflect the new bound for
any subsequent use.
In `@packages/account/src/falcon_512/ntt/falcon512.cairo`:
- Around line 34-50: Add an explicit precondition at the start of
config_for_degree validating that n is a power of two within the supported range
[4, 512], and assert with a message that includes the supplied n when invalid.
Keep the existing permutation construction and config_with_perm flow unchanged
for valid degrees.
In `@packages/account/src/tests/test_falcon_512.cairo`:
- Around line 375-391: The four negative tests currently accept any panic; pin
each to the specific expected panic message. In
packages/account/src/tests/test_falcon_512.cairo:375-391, update
test_constructor_rejects_wrong_public_key_length and
test_constructor_rejects_noncanonical_public_key with the messages emitted by
the public-key length and canonicality checks. In
packages/account/src/tests/test_falcon_512_ntt.cairo:143-153, update
test_forward_roots_reject_unsupported_degree and
test_inverse_roots_reject_unsupported_degree with the actual messages from
get_even_roots_felt and get_scaled_inv_roots.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bca1632-f2eb-4d2d-9c6f-db954491b7bc
⛔ Files ignored due to path filters (1)
Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.github/workflows/test.ymlLICENSEScarb.tomlpackages/account/README.mdpackages/account/Scarb.tomlpackages/account/src/falcon_512.cairopackages/account/src/falcon_512/account.cairopackages/account/src/falcon_512/falcon.cairopackages/account/src/falcon_512/falcon_512_shake.cairopackages/account/src/falcon_512/falcon_512_shake_direct.cairopackages/account/src/falcon_512/hashing.cairopackages/account/src/falcon_512/hashing/hash_to_point.cairopackages/account/src/falcon_512/hashing/shake256.cairopackages/account/src/falcon_512/ntt.cairopackages/account/src/falcon_512/ntt/bitrev.cairopackages/account/src/falcon_512/ntt/engine.cairopackages/account/src/falcon_512/ntt/falcon512.cairopackages/account/src/falcon_512/ntt/falcon512_fast.cairopackages/account/src/falcon_512/ntt/roots_felt.cairopackages/account/src/falcon_512/ntt/roots_scaled.cairopackages/account/src/falcon_512/packing.cairopackages/account/src/falcon_512/zq.cairopackages/account/src/lib.cairopackages/account/src/tests.cairopackages/account/src/tests/falcon_512_fixture.cairopackages/account/src/tests/test_falcon_512.cairopackages/account/src/tests/test_falcon_512_ntt.cairopackages/interfaces/README.mdpackages/interfaces/src/account/accounts.cairo
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Review — falcon_512
|
immrsd
left a comment
immrsd
left a comment
There was a problem hiding this comment.
Left minor comments, apart from that LGTM
| while let Some(lane) = lanes.pop_front() { | ||
| push_lane_words(*lane, two16, b256, q32, ref coeffs); | ||
| } | ||
| if coeffs.len() == 512 { |
There was a problem hiding this comment.
512 is used many times throughout the file, might be a good idea to extract it to a constant (COEFFS_TARGET_LEN or maybe something like that)
| let (_, r0) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v0).unwrap(), q_nz); | ||
| let (_, r1) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v1).unwrap(), q_nz); | ||
| let (_, r2) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v2).unwrap(), q_nz); | ||
| let (_, r3) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v3).unwrap(), q_nz); | ||
| let (_, r4) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v4).unwrap(), q_nz); | ||
| let (_, r5) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v5).unwrap(), q_nz); | ||
| let (_, r6) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v6).unwrap(), q_nz); | ||
| let (_, r7) = DivRem::div_rem(TryInto::<felt252, u128>::try_into(v7).unwrap(), q_nz); |
There was a problem hiding this comment.
Does the compiler fail to derive types for TryInto here?
| @@ -0,0 +1,45 @@ | |||
| use openzeppelin_interfaces::src9::OutsideExecution; | |||
| /// Its public-key and signature formats are contract-specific encodings for the FALCON submission | ||
| /// verification relation, rather than FN-DSA (FIPS 206) encodings. | ||
| #[starknet::contract(account)] | ||
| pub mod Falcon512ShakeAccountUpgradeable { |
There was a problem hiding this comment.
Does the contract deliberately not use the with_component macros for OZ components? Do you think it might be helpful to reduce boilerplate code here?
| newPublicKey: Array<felt252>, | ||
| signature: Span<felt252>, | ||
| ) { | ||
| PublicKey::set_public_key(ref self, newPublicKey, signature); |
There was a problem hiding this comment.
Seem like it should be possible to use receiver syntax here
| PublicKey::set_public_key(ref self, newPublicKey, signature); | |
| self.set_public_key(newPublicKey, signature); |
| let mut index = 0; | ||
| while index != len { | ||
| public_key.append(self.Falcon512Account_public_key.at(index).read()); | ||
| index += 1; | ||
| } |
| while z != pad_len { | ||
| padded.append(0); | ||
| z += 1; | ||
| } |
There was a problem hiding this comment.
for z in 1..pad_len can be used here
| let mut state = [0; 25]; | ||
| let n_blocks = (l + pad_len) / RATE_BYTES; | ||
| let mut blk = 0; | ||
| while blk != n_blocks { |
| let mut out: Array<u8> = array![]; | ||
| let span = bytes.span(); | ||
| let mut i = 0; | ||
| while i != span.len() { |
| fn test_shake256_multiblock() { | ||
| let mut input: Array<u8> = array![]; | ||
| let mut i = 0; | ||
| while i != 200 { |
| fn test_unpack_rejects_noncanonical_slot_in_each_unrolled_position() { | ||
| let packed = pack_512(pseudorandom_coeffs().span()); | ||
| let mut bad_index = 0; | ||
| while bad_index != 7 { |
|
|
||
| fn max_values() -> Array<u16> { | ||
| let mut values = array![]; | ||
| while values.len() != 512 { |
There was a problem hiding this comment.
512 is used here as a magic value, consider extracting it to a constant
immrsd
left a comment
immrsd
left a comment
There was a problem hiding this comment.
Full review of the Falcon-512 PQ account addition — findings verified against the actual sources, with the crypto core independently re-derived and the gas/size numbers measured on this branch (scarb/cairo 2.18.0).
Measured baseline (release profile):
| CASM felts | class bytes | sierra gas / verify | |
|---|---|---|---|
Falcon512ShakeAccountUpgradeable |
57,120 (69.7% of 81,920 cap) | 2,159,881 | ~52.5M |
Falcon512ShakeDirectAccountUpgradeable |
57,048 | 2,039,421 | ~62.7M |
AccountUpgradeable (baseline) |
6,244 | 139,195 | — |
Cost split of one verification: SHAKE-256/hash_to_point ~36M (68%), generic intt ~15.5M (direct only), generated forward NTT ~1.6M, unpacking ~1.5M each.
Inline comments carry the individual findings (3 High, 8 Medium, 8 Low/Info). Two things that don't anchor to changed lines:
- SRC9 self-call cost (Low, pre-existing design):
SRC9Component::assert_valid_signaturedispatchesis_valid_signaturethroughcall_contract_syscallat the account's own address, re-entering the 57k-felt class and re-reading the whole 29-felt key (verified:CallContract: 2, StorageRead: 90vs 1/30 on the direct path). Account-agnostic by design and not introduced here, but far more visible with this key size — the presets could call the component impl directly. - Minor:
Errors::INVALID_PUBLIC_KEYreused for an unreachable stored-length invariant (account.cairo:397);SHIFTadded per NTT output instead of per butterfly pair (−512 CASM felts available, two-line generator change); duplicated cfg-gated module/forward_nttdeclarations inpresets/src/lib.cairoandfalcon.cairo;second_accept_ownership_hash()unused;DIRECT_SIGNATURE_FELTShardcoded in preset tests because the constant ispub(crate); theintt(…, 111, 12289, …)NTT test passes deliberately inconsistent bits/bound args with no explanatory comment.
Verified sound (checked, not assumed): norm bound 34034726 = reference l2bound[9] with correct inclusive comparison; centering arithmetic; hint soundness (the generator proves the 512 evaluation points are distinct roots of x^512+1, so the NTT is a bijection and mul_hint is uniquely pinned to s1·h); strict canonical-encoding bijection with [0,q)^512 (all Acc*/DivT constants verified); SHAKE-256 round constants, ρ/π/θ/χ/ι and pad10*1 layout against FIPS 202; REJECT_BOUND = 5q; all overflow bounds (norm accumulator < 2^36, NTT lazy-reduction schedule peaks at 2^122.3 with 6 bits of headroom — do not reuse the generator for n=1024 without re-running the bound check). The checked-in NTT tables regenerate byte-for-byte from the script; the fixtures were decoded in Python and the Falcon relation re-verified externally. Replay protection, deploy front-running (key in constructor calldata → address derivation), access control, and graceful rejection of malformed input all check out.
Overall: the cryptographic core is correct and carefully built, but the upgrade-bricking hazard needs closing, the generated-code and SHAKE efficiency work plus a CASM/gas CI guard are needed before these presets are safe to evolve at 70% of the bytecode cap — and the hint-vs-direct question deserves an explicit decision before both variants become public API.
| UpgradeableEvent: UpgradeableComponent::Event, | ||
| } | ||
|
|
||
| #[constructor] |
There was a problem hiding this comment.
Severity 4 — High: upgrading into (or out of) this preset permanently bricks the account.
The component stores its key under Falcon512Account_public_key, distinct from Account_public_key / EthAccount_public_key, and initializer is constructor-only with no post-upgrade path. A user upgrading AccountUpgradeable → this preset (which the README's "can adopt verifier changes at the same account address" wording invites) lands with an empty key: every __validate__ and is_valid_signature fails, and set_public_key needs a self-call that itself needs a passing validation — funds are irrecoverably frozen. The reverse direction fails identically. Only the cross-variant Falcon↔Falcon upgrade is safe, and that's the only case the tests cover (test_falcon_512_accounts.cairo:196-208). Same applies to falcon_512_shake_direct_account.cairo.
Suggested fix: document loudly that these presets are upgrade-compatible only with each other, and/or assert a non-empty Falcon512Account_public_key inside upgrade().
| type RemT = Falcon512Zq; | ||
| } | ||
|
|
||
| #[inline(always)] |
There was a problem hiding this comment.
Severity 4 — High: felt252_as_u128 silently truncates, and its Wide arm is ~20% dead bytecode.
The Wide((_, low)) => low arm discards the high 128 bits. It's provably unreachable for canonical inputs today (verified by re-running the generator's interval analysis), but a future caller passing a coefficient ≥ q would get a silently wrong (attacker-influenceable) NTT output instead of a panic — in the hint path that's the ingredient for an unsignalled forgery. Separately, the never-executed arm costs ~11,776 dead CASM felts across the 512 emission sites (~20.6% of the class), and the conversion overall is ~16,896 felts (~29.6%) plus ~3,070 steps per NTT call.
Suggested fix: have generate_ntt.py emit the op graph over BoundedInt with bounded_int_add/sub/mul (these lower to the same felt252_add/sub/mul CASM at zero extra cost) feeding straight into bounded_int_div_rem — removes ~16.9k CASM felts and ~3.1k steps per call while turning precondition violations into reverts. The helper traits would need re-exporting from openzeppelin_corelib_imports.
| /// bits that wrap to the bottom (`hi`); they occupy disjoint positions, so `lo + hi` | ||
| /// is the rotation. | ||
| #[inline(always)] | ||
| fn rotl(x: u128, pow: felt252, two64: NonZero<u128>) -> u128 { |
There was a problem hiding this comment.
Severity 4 — High: SHAKE-256 is ~68% of every verification's cost and rotl is most of it.
Measured on this branch: one hash_to_point costs ~36M sierra gas — 68% (hint) / 57% (direct) of a full verification (~52.5M / ~62.7M). rotl runs ~5,900 times per verification (29× per round × 24 rounds × ~8.5 permutations), each doing a fallible felt252 → u128 try_into().unwrap(), a generic u128_safe_divmod, and a checked add (~3,000 gas apiece).
The PR already vendors the cheap tools (u128s_from_felt252 in falcon512_fast.cairo, bounded_int_div_rem in packing.cairo) but doesn't use them here.
Suggested fix: type the lane as BoundedInt<0, 0xffff_ffff_ffff_ffff>, split the shifted product with bounded_int_div_rem by UnitInt<2^64> and add the halves with a bounded add. Estimated saving: 10–15M gas per verification (~20–25% of total).
| ) | ||
| } | ||
|
|
||
| fn is_valid_public_key(public_key: Span<felt252>) -> bool { |
There was a problem hiding this comment.
Severity 3 — Medium: is_valid_public_key accepts keys that brick the account at deploy.
Only the 29-felt canonical base-Q packing is checked, so (a) a standard coefficient-domain Falcon h (instead of the NTT-domain h_ntt the verifiers require) and (b) the all-zero key both deploy successfully and can never validate a signature; recovery via set_public_key is impossible since it requires a self-call. The NTT-domain requirement appears only in the doc comment at account.cairo:9, not in the README or preset constructor docs.
Suggested fix: state the NTT-domain requirement in packages/account/README.md and both preset constructors; consider rejecting the all-zero key here.
| snforge test -p openzeppelin_account --release --features falcon_fast_tests falcon_512 --max-n-steps 100000000 | ||
| snforge test -p openzeppelin_presets --release --features falcon_presets_tests falcon_512 --max-n-steps 100000000 | ||
|
|
||
| - name: Build release-profile presets |
There was a problem hiding this comment.
Severity 3 — Medium: CI never compiles the presets to CASM, and the default profile silently produces an undeclarable class.
With casm = false in packages/presets/Scarb.toml, this step only ever produces Sierra — nothing verifies the class lowers to CASM or fits the 81,920-felt bytecode cap. Verified on this branch:
- release profile:
Falcon512ShakeAccountUpgradeable= 57,120 CASM felts (69.7% of the cap), direct = 57,048; 2.16 MB Sierra vs the ~4.09 MB class-size cap. - dev profile: plain
scarb build -p openzeppelin_presetsemits a 32.6 MBcontract_class.json(8× over the declare limit, debug info balloons), and withcasm = trueit hard-fails withOffsetOverflow—inlining-strategy = "avoid"still honors#[inline(always)]on the 9,480-line NTT body.
A future regression lands with green CI, and headroom is ~24.8k felts.
Suggested fix: enable CASM for the release target and assert bytecode size < 81,920 in CI; document that these presets are release-profile-only; consider recording them in scripts/benchmarking.
| ### Interfaces | ||
|
|
||
| - [`ISRC6`](https://docs.openzeppelin.com/contracts-cairo/3.x/api/account#ISRC6) | ||
| - [`IFeltArrayDeployable`](https://docs.openzeppelin.com/contracts-cairo/3.x/api/account#IFeltArrayDeployable) |
There was a problem hiding this comment.
Severity 2 — Low: broken doc links and misplaced maintainer content.
- The anchors here (
#Falcon512AccountComponent,#IFeltArrayDeployable, …, and#Falcon512ShakeAccountUpgradeableinpackages/presets/README.md) target adoc pages that were added in 96110a6 and deleted in 53aa1ee — they resolve nowhere. The URLs also hardcode/contracts-cairo/3.x/while the package is4.0.0-alpha.1. - The "regenerate the NTT sources" section (lines 48–57) is maintainer-only and references
scripts/falcon_512/, which isn't in the published package — better inCONTRIBUTING.md. - Also missing from any README: the new presets aren't in
sncast_scripts/src/declare_presets.cairo/ itsbuild-external-contracts, so they won't be pre-declared on Sepolia as the presets docs promise.
| run: scarb fmt --check --workspace | ||
|
|
||
| - name: Run tests | ||
| run: snforge test --workspace --features fuzzing --fuzzer-runs 200 |
There was a problem hiding this comment.
Severity 2 — Low: CI runs the expensive Falcon suite redundantly.
The 65 dev-profile Falcon tests run twice (here and in the coverage run at line 67), several above 1e9 L2 gas, serial in one job — and the coverage run doesn't enable falcon_presets_tests, so the Falcon presets (SRC9 routing, upgrade's assert_only_self) report as entirely untested in Codecov anyway. Two further cost sinks: test_fast_ntt_matches_generic_reference_for_every_basis_vector alone costs 7.65B gas (~76M steps) and is single-handedly why the 100M step cap is needed (the boundary/pseudorandom test already covers the interesting cases — sample every 16th basis vector or feature-gate it); and the #[cfg(test)] felt wrapper re-inlines the 31k-statement NTT body, making ~26.7% of the test program duplicate code and a 44 MB test artifact (have it call ntt_falcon512_fast_u16_unchecked and convert instead).
| msg(), key.span(), with_appended(valid_signature.span(), 0).span(), | ||
| ), | ||
| ); | ||
| assert!( |
There was a problem hiding this comment.
Severity 2 — Low: several rejection tests don't actually pin the guards they target.
- Non-canonical-slot and out-of-range-salt rejection is asserted by replacing a slot wholesale, which also changes the coefficients/hash — the assertion still passes if the guard is deleted. Tamper additively (
signature[0] + Q_POW_9) instead, and probe indices 28 and 59, not just 0 (the last-slot guards inpacking.cairo:122-125never execute throughverify). - No
component_state_for_testing()block (unliketest_account.cairo), leaving unreachable:initializercalled twice, thestored_len == new_public_key.len()branch ataccount.cairo:397,read_public_keyon empty storage, andset_public_keywith a non-canonical correct-length key. test_repeated_key_rotation_keeps_exact_storage_lengthrotates twice to the same key and asserts only the length — rotate to a distinct third key and compare full content.- In-range tampering is tested for
salt_abut neversalt_b; SRC9 replay is checked only via the nonce flag, never by resubmitting.
| } | ||
|
|
||
| /// Verifier for the 60-felt SHAKE-256 signature carrying a polynomial-product hint. | ||
| pub impl Falcon512ShakeVerifier of Falcon512SignatureVerifier { |
There was a problem hiding this comment.
Severity 1 — Info: cross-variant signature malleability.
Since the hint layout is s1(29) || salt(2) || mul_hint(29), truncating a 60-felt hint signature to its first 31 felts yields a valid direct signature for the same key and message hash (the preset tests rely on this via copy_prefix). Not exploitable for replay — the tx hash and SNIP-12 domain bind the account address — but integrations that deduplicate or key state off exact signature bytes would see two distinct byte strings authenticating the same message. Worth one sentence in the README.
| let two16: NonZero<u64> = 0x10000_u64.try_into().unwrap(); | ||
| let b256: NonZero<u64> = 0x100_u64.try_into().unwrap(); | ||
| let mut coeffs: Array<u16> = array![]; | ||
| loop { |
There was a problem hiding this comment.
Severity 1 — Info: the squeeze loop has no iteration cap and over-runs after completion.
Termination is probabilistic (an attacker can't cheaply steer SHAKE output, so not exploitable), and once 512 coefficients are collected the loop still walks the remaining lanes of the current block (~60 wasted candidate evaluations) while push_candidate re-checks coeffs.len() != 512 per candidate (~580×). A hard block cap plus breaking out of the lane loop on completion would bound the worst case explicitly and shave a little gas.
3 participants
Fixes #1725
Summary
Falcon512AccountComponentwith SHAKE-256 hint and direct verification strategies for canonical 29-felt Falcon-512 public keys.Falcon512ShakeAccountUpgradeableandFalcon512ShakeDirectAccountUpgradeablepresets with SRC9 outside execution and self-authorized class upgrades.The verifiers implement the SHAKE-256 verification relation from the FALCON submission selected by NIST using contract-specific felt encodings. They are not FN-DSA (FIPS 206) implementations.
PR Checklist