diff --git a/Cargo.lock b/Cargo.lock index 034b2c3c..8b47c991 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8103,7 +8103,7 @@ dependencies = [ [[package]] name = "pallet-shielded-pool" -version = "0.15.0" +version = "0.16.0" dependencies = [ "ark-bn254", "ark-ff 0.5.0", diff --git a/frame/evm/precompile/shielded-pool/src/tests.rs b/frame/evm/precompile/shielded-pool/src/tests.rs index f6c9a89d..67e4f7f2 100644 --- a/frame/evm/precompile/shielded-pool/src/tests.rs +++ b/frame/evm/precompile/shielded-pool/src/tests.rs @@ -10,6 +10,19 @@ use crate::{ ShieldedPoolPrecompile, }; +/// A distinct, canonical 32-byte field value for `seed`. +/// +/// Commitments and nullifiers are now checked against the BN254 modulus, and a +/// repeated byte at or above 0x30 exceeds it — `p` starts at 0x30. Real values +/// come out of Poseidon and are always canonical, so a filler that is not would +/// exercise a shape the chain never produces. +fn canon(seed: u8) -> [u8; 32] { + let mut b = [0u8; 32]; + b[0] = seed; + b[1] = 0xA5; + b +} + // ─── Assertion helpers ─────────────────────────────────────────────────────── fn expect_error(result: Result) { @@ -407,7 +420,7 @@ fn shield_stores_commitment_and_updates_balance() { fn shield_multiple_commitments_are_all_stored() { new_test_ext().execute_with(|| { for (i, byte) in [0x11u8, 0x22, 0x33].iter().enumerate() { - do_shield([*byte; 32], 500); + do_shield(canon(*byte), 500); assert_eq!( pallet_shielded_pool::MerkleTreeSize::::get(), (i + 1) as u32 @@ -424,7 +437,7 @@ fn shield_multiple_commitments_are_all_stored() { fn shield_updates_merkle_root_after_each_insertion() { new_test_ext().execute_with(|| { let root_before = current_root(); - do_shield([0x42; 32], 1_000); + do_shield(canon(0x42), 1_000); let root_after = current_root(); assert_ne!(root_before, root_after, "root must change after shield"); }); @@ -433,7 +446,7 @@ fn shield_updates_merkle_root_after_each_insertion() { #[test] fn shield_with_zero_value_rejected() { new_test_ext().execute_with(|| { - let input = encode_shield(0, [0xAA; 32], &[0x00; 180]); + let input = encode_shield(0, canon(0xAA), &[0x00; 180]); let mut h = MockHandle::with_value(input, 0); expect_error(ShieldedPoolPrecompile::::execute(&mut h)); }); @@ -454,14 +467,14 @@ fn private_transfer_rejects_truncated_input() { #[test] fn private_transfer_rejects_empty_proof() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); // empty proof → MockZkVerifier returns Err let input = encode_private_transfer( &[], root, &[[0x11; 32], [0x22; 32]], - &[[0x33; 32], [0x44; 32]], + &[canon(0x33), canon(0x44)], &[vec![0xAA; 180], vec![0xBB; 180]], 0, 0, @@ -477,7 +490,7 @@ fn private_transfer_rejects_zero_nullifiers() { // Calling with an empty nullifier array must be rejected at the precompile // boundary before touching the pallet. new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let input = encode_private_transfer( &[0x01], @@ -498,13 +511,13 @@ fn private_transfer_rejects_zero_nullifiers() { fn private_transfer_rejects_mismatched_nullifier_commitment_count() { // 2 nullifiers but 1 commitment — structurally inconsistent. new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let input = encode_private_transfer( &[0x01], root, &[[0x11; 32], [0x22; 32]], // 2 nullifiers - &[[0x33; 32]], // 1 commitment + &[canon(0x33)], // 1 commitment &[vec![0xAA; 180]], // 1 memo 0, 0, @@ -519,14 +532,14 @@ fn private_transfer_rejects_mismatched_nullifier_commitment_count() { fn private_transfer_rejects_mismatched_commitment_memo_count() { // 2 commitments but 1 memo — structurally inconsistent. new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let input = encode_private_transfer( &[0x01], root, - &[[0x11; 32], [0x22; 32]], // 2 nullifiers - &[[0x33; 32], [0x44; 32]], // 2 commitments - &[vec![0xAA; 180]], // 1 memo — mismatch + &[[0x11; 32], [0x22; 32]], // 2 nullifiers + &[canon(0x33), canon(0x44)], // 2 commitments + &[vec![0xAA; 180]], // 1 memo — mismatch 0, 0, 1, @@ -539,12 +552,12 @@ fn private_transfer_rejects_mismatched_commitment_memo_count() { #[test] fn private_transfer_happy_path() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let nullifier_1 = [0x11; 32]; let nullifier_2 = [0x22; 32]; - let commitment_1 = [0x33; 32]; - let commitment_2 = [0x44; 32]; + let commitment_1 = canon(0x33); + let commitment_2 = canon(0x44); let input = encode_private_transfer( &[0x01, 0x02, 0x03], @@ -585,9 +598,9 @@ fn private_transfer_happy_path() { #[test] fn private_transfer_rejects_double_spend() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); - let nullifier = [0xDE; 32]; + let nullifier = canon(0xDE); let input = encode_private_transfer( &[0x01], @@ -611,14 +624,14 @@ fn private_transfer_rejects_double_spend() { #[test] fn private_transfer_root_updates_after_outputs() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root_before = current_root(); let input = encode_private_transfer( &[0x01], root_before, &[[0x11; 32], [0x22; 32]], - &[[0x33; 32], [0x44; 32]], + &[canon(0x33), canon(0x44)], &[vec![0xAA; 180], vec![0xBB; 180]], 0, 0, @@ -650,12 +663,12 @@ fn unshield_rejects_truncated_input() { #[test] fn unshield_rejects_empty_proof() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let input = encode_unshield( &[], root, - [0x77; 32], + canon(0x77), 0, 100, recipient_bytes(), @@ -672,9 +685,9 @@ fn unshield_rejects_empty_proof() { #[test] fn unshield_happy_path() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); - let nullifier = [0x77; 32]; + let nullifier = canon(0x77); let input = encode_unshield( &[0x09, 0x09], @@ -707,9 +720,9 @@ fn unshield_happy_path() { #[test] fn unshield_rejects_double_spend() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); - let nullifier = [0x77; 32]; + let nullifier = canon(0x77); let input = encode_unshield( &[0x09, 0x09], @@ -734,12 +747,12 @@ fn unshield_rejects_double_spend() { #[test] fn unshield_full_balance() { new_test_ext().execute_with(|| { - do_shield([0x55; 32], 1_000); + do_shield(canon(0x55), 1_000); let root = current_root(); let input = encode_unshield( &[0x01], root, - [0x99; 32], + canon(0x99), 0, 1_000, recipient_bytes(), @@ -759,12 +772,12 @@ fn unshield_rejects_zero_recipient() { // AccountId32 of all zeros is a permanent burn address. The precompile // must reject it before dispatching to avoid silent token destruction. new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let input = encode_unshield( &[0x09, 0x09], root, - [0x77; 32], + canon(0x77), 0, 100, [0u8; 32], // zero AccountId32 @@ -783,12 +796,12 @@ fn unshield_rejects_zero_amount() { // amount = 0 is semantically invalid and must be rejected at the precompile // level before dispatch. new_test_ext().execute_with(|| { - do_shield([0x55; 32], 5_000); + do_shield(canon(0x55), 5_000); let root = current_root(); let input = encode_unshield( &[0x09, 0x09], root, - [0x77; 32], + canon(0x77), 0, 0, // zero amount recipient_bytes(), @@ -811,14 +824,14 @@ fn unshield_rejects_zero_amount() { fn full_lifecycle_shield_transfer_unshield() { new_test_ext().execute_with(|| { // 1. Shield - do_shield([0xAA; 32], 10_000); + do_shield(canon(0xAA), 10_000); assert_eq!(pallet_shielded_pool::MerkleTreeSize::::get(), 1); // 2. Private transfer let root_1 = current_root(); - let nullifier_in = [0xBB; 32]; - let commitment_out_1 = [0xCC; 32]; - let commitment_out_2 = [0xDD; 32]; + let nullifier_in = canon(0xBB); + let commitment_out_1 = canon(0xCC); + let commitment_out_2 = canon(0xDD); let pt_input = encode_private_transfer( &[0x01], @@ -836,7 +849,7 @@ fn full_lifecycle_shield_transfer_unshield() { // 3. Unshield one of the outputs let root_2 = current_root(); - let nullifier_out = [0xEE; 32]; + let nullifier_out = canon(0xEE); let unshield_input = encode_unshield( &[0x02], root_2, diff --git a/frame/shielded-pool/CHANGELOG.md b/frame/shielded-pool/CHANGELOG.md index 0c3e4c5b..6c00ae4c 100644 --- a/frame/shielded-pool/CHANGELOG.md +++ b/frame/shielded-pool/CHANGELOG.md @@ -2,39 +2,54 @@ All notable changes to `pallet-shielded-pool` will be documented in this file. -## [0.15.0] - 2026-08-06 - -### Added -- New Config constant **`SealedTreePrunedBelowLevel`** (production 10) and an - `on_idle` hook that reclaims internal Merkle nodes from **sealed** trees. - - A sealed tree kept ~1,048,574 `MerkleNodes` entries forever — roughly 72 MiB - each, growing without bound across up to 4096 trees, and every full node had to - retain all of it. That storage serves exactly one purpose: handing Merkle paths - to wallets so they can build a spend proof. No dispatchable reads it, so - dropping it cannot affect whether a note is spendable. - - Nodes concentrate at the bottom of the tree: level 1 holds half of them, level - 10 holds 0.1%. Cutting at level 10 therefore frees **99.8%** (1,048,574 → 2,046 - per tree) while a path costs 2^10 leaf reads and 1,023 Poseidon hashes — - measured at 58.1 µs/hash, so ~60 ms native and ~180 ms in Wasm. Level 12 would - free only 0.15% more for four times the work. +## [0.16.0] - 2026-08-07 - The level is configurable rather than fixed because the recompute cost tracks - validator hardware. `integrity_test` rejects a cut outside `1..tree_depth`. +### Security -### Changed -- `get_merkle_path` rebuilds pruned siblings from `MerkleLeaves` on demand. Only - the sibling subtree is recomputed, never the whole tree, and a **sealed** tree - is immutable so the result is byte-identical to what was stored. The active - tree is untouched: still 20 point reads and zero hashes. +- **Canonical-encoding guard on commitments and nullifiers.** Both are stored as + raw bytes and used as raw `StorageMap` keys, while byte-to-field conversion + reduces modulo the BN254 prime `p`. Without a canonicity check, `n` and `n + p` + are two distinct keys for the **same** field element — so a nullifier could be + presented twice under different bytes, and one note spent twice. + + `Commitment::is_canonical` and `Nullifier::is_canonical` are now checked on + every write path: `shield` (where the depositor picks the bytes with no proof + constraining them, the least guarded way into the tree), `private_transfer` + (both nullifiers and output commitments), and `unshield` (the nullifier and + the change commitment). + + Nothing reached storage before this: the verifier passes public inputs through + `to_field_elements`, which re-encodes and compares byte for byte. But that was + a single check on a single path — any future route that records a nullifier + without full verification (a relayer pre-check, a mempool dedup cache, a + runtime API) would have lost it silently. The rule now lives at the type + boundary that every path crosses. + + The helpers this uses (`FieldElement::is_canonical_le`) already existed in + `zk-core` with a doc saying callers should reject non-canonical input at the + trust boundary. They had no callers. + + Canonicity is kept separate from `validate()` / `is_valid()`: an all-zero + nullifier marks a dummy input slot and is canonical, so folding the two + together would have rejected legitimate transfers. + +- **Zero commitments are refused.** Zero *is* a canonical field value, so the + canonicity check alone admitted it — a dev-node probe caught this after the + guards were in. It has to be refused separately: the tree represents an absent + leaf with `[0u8; 32]` (`get_zero_hash_cached(0)`), so a stored zero leaf is + indistinguishable from an empty slot when `subtree_root` rebuilds a pruned + level. Nobody can prove a preimage for it either, so it would sit in the tree + permanently as dead weight. `shield` and `private_transfer` now check + `is_valid()` alongside `is_canonical()`; `unshield` already skipped a zero + change commitment through its `has_change` branch. ### Fixed + - **`zero_hash_at_level` is iterative.** It was defined recursively, one stack frame per level, and `get_zero_hash_cached` falls through to it for any level past its 21-entry table. `usize` is 32 bits under Wasm, so a caller passing a - large level would exhaust the runtime's fixed 1 MB stack — and a stack - overflow there aborts the process rather than raising a catchable panic. + large level would exhaust the runtime's fixed 1 MB stack — and that aborts the + process rather than raising a catchable panic. Measured on a 1 MiB thread: a recursion of this shape returns at 10,000 frames and aborts at 20,000. The loop returns at 5,000 with the same stack and would @@ -42,7 +57,7 @@ All notable changes to `pallet-shielded-pool` will be documented in this file. Unreachable today, and not just by configuration: every `level` at every call site comes from a `0..depth` loop bound by `DEFAULT_TREE_DEPTH`, so no external - input selects one. Latent because the ladder is `pub` and the bound lives in + input selects one. Latent because the ladder is `pub` and the bound lived in the callers rather than in the function. The digests are unchanged, which is the part that matters: these hashes stand @@ -58,6 +73,65 @@ All notable changes to `pallet-shielded-pool` will be documented in this file. belonged on the type rather than only in the runtime's `integrity_test`. A const assertion now fails the build instead. +### Notes + +- **Breaking for callers that submit non-canonical bytes.** Nothing the chain + produces is affected — commitments and nullifiers come out of Poseidon and are + always canonical — but a client generating raw 32-byte values without reducing + mod `p` will now be rejected with `InvalidPublicSignals`. + + Several of this pallet's own tests were doing exactly that, using fillers like + `[0xC1; 32]` and `[0x31; 32]`. Both sit above `p`, whose top byte is `0x30`. + They were testing a shape the chain never produces and have been moved onto + canonical values. + +### Verification +321 pallet tests (14 new) and 65 precompile tests, clippy clean under the CI feature set, and two +dev-node runs against the Merkle changes: one reads the zero-hash ladder back +out of real Merkle paths and matches all 19 levels against pinned digests +(10/10), the other is adversarial — `u32::MAX` leaf indices, 64 KiB and non-hex +commitments, a 50-request burst, storage queries at levels 21 through 255 — +checking block height after each batch (30/30). + +The canonicity guard's decisive test builds `n` and `n + p`, asserts they are +different bytes but the same field element, and requires that only the canonical +one is accepted. Without that second assertion the test would pass against a +guard that rejected everything. + +A dev-node run (18/18) submits the same pair on-chain and confirms only one ends +up in the reverse index, that `p` itself and all-ones are refused while `p - 1` +is accepted, and that 25 rejected shields in a burst grow the tree by nothing — +checking block height after each batch. It is what surfaced the zero-commitment +gap above. + +## [0.15.0] - 2026-08-06 + +### Added +- New Config constant **`SealedTreePrunedBelowLevel`** (production 10) and an + `on_idle` hook that reclaims internal Merkle nodes from **sealed** trees. + + A sealed tree kept ~1,048,574 `MerkleNodes` entries forever — roughly 72 MiB + each, growing without bound across up to 4096 trees, and every full node had to + retain all of it. That storage serves exactly one purpose: handing Merkle paths + to wallets so they can build a spend proof. No dispatchable reads it, so + dropping it cannot affect whether a note is spendable. + + Nodes concentrate at the bottom of the tree: level 1 holds half of them, level + 10 holds 0.1%. Cutting at level 10 therefore frees **99.8%** (1,048,574 → 2,046 + per tree) while a path costs 2^10 leaf reads and 1,023 Poseidon hashes — + measured at 58.1 µs/hash, so ~60 ms native and ~180 ms in Wasm. Level 12 would + free only 0.15% more for four times the work. + + The level is configurable rather than fixed because the recompute cost tracks + validator hardware. `integrity_test` rejects a cut outside `1..tree_depth`. + +### Changed +- `get_merkle_path` rebuilds pruned siblings from `MerkleLeaves` on demand. Only + the sibling subtree is recomputed, never the whole tree, and a **sealed** tree + is immutable so the result is byte-identical to what was stored. The active + tree is untouched: still 20 point reads and zero hashes. + +### Fixed - `hash_pair_poseidon` clamps its output copy instead of slicing `&bytes[..32]` raw. BN254 `Fr` always yields 32 bytes, so the clamp never binds today — but this runs on the block-import path, where slicing past the end panics the node diff --git a/frame/shielded-pool/Cargo.toml b/frame/shielded-pool/Cargo.toml index 7fdcd5be..9b0521c1 100644 --- a/frame/shielded-pool/Cargo.toml +++ b/frame/shielded-pool/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pallet-shielded-pool" -version = "0.15.0" +version = "0.16.0" description = "Shielded pool pallet for private transactions using ZK proofs" authors = ["Orbinum Team"] license = "GPL-3.0-or-later" diff --git a/frame/shielded-pool/src/benchmarking.rs b/frame/shielded-pool/src/benchmarking.rs index f1f71212..19cd3116 100644 --- a/frame/shielded-pool/src/benchmarking.rs +++ b/frame/shielded-pool/src/benchmarking.rs @@ -79,11 +79,24 @@ mod benchmarks { if scaled > floor { scaled } else { floor } } + /// A distinct, canonical 32-byte field value for `seed`. + /// + /// Commitments and nullifiers are checked against the BN254 modulus, and a + /// repeated byte at or above 0x30 exceeds it — `p` starts at 0x30. Real + /// values come out of Poseidon and are always canonical, so a filler that is + /// not would benchmark a call the chain would reject. + fn canonical_bytes(seed: u8) -> [u8; 32] { + let mut b = [0u8; 32]; + b[0] = seed; + b[1] = 0xA5; + b + } + #[benchmark] fn shield() { let (caller, asset_id) = setup_benchmark_env::(); let amount: BalanceOf = bench_amount::(); - let commitment = Commitment([1u8; 32]); + let commitment = Commitment(canonical_bytes(1)); // Memo must be exactly 180 bytes (MAX_ENCRYPTED_MEMO_SIZE): nonce(12) + data(120) + MAC(16) + ephPk(32) let memo_bytes = vec![0u8; MAX_ENCRYPTED_MEMO_SIZE as usize]; let encrypted_memo = FrameEncryptedMemo(memo_bytes.try_into().unwrap()); @@ -105,7 +118,7 @@ mod benchmarks { let mut operations = Vec::new(); for i in 0..n { - let commitment = Commitment([i as u8; 32]); + let commitment = Commitment(canonical_bytes(i as u8)); let memo_bytes = vec![0u8; MAX_ENCRYPTED_MEMO_SIZE as usize]; let encrypted_memo = FrameEncryptedMemo(memo_bytes.try_into().unwrap()); operations.push((asset_id, amount, commitment, encrypted_memo)); @@ -131,8 +144,8 @@ mod benchmarks { let mut comms = Vec::new(); let mut memos = Vec::new(); for i in 0..n { - nulls.push(Nullifier([(0x20 + i) as u8; 32])); - comms.push(Commitment([(0x30 + i) as u8; 32])); + nulls.push(Nullifier(canonical_bytes((0x20 + i) as u8))); + comms.push(Commitment(canonical_bytes((0x30 + i) as u8))); let memo_bytes = vec![0u8; MAX_ENCRYPTED_MEMO_SIZE as usize]; memos.push(FrameEncryptedMemo(memo_bytes.try_into().unwrap())); } @@ -178,7 +191,7 @@ mod benchmarks { ); let proof: BoundedVec> = vec![0u8; 128].try_into().unwrap(); - let nullifier = Nullifier([4u8; 32]); + let nullifier = Nullifier(canonical_bytes(4)); // Must be >= T::Relayer::min_relay_fee() to pass the FeeTooLow check. let fee: BalanceOf = T::Relayer::min_relay_fee().saturated_into(); @@ -243,7 +256,7 @@ mod benchmarks { // Accumulate relay fees for the validator. T::Relayer::accumulate_relay_fee(&caller, asset_id, amount_u128); - let commitment = Commitment([0x11u8; 32]); + let commitment = Commitment(canonical_bytes(0x11)); // public_signals layout (76 bytes): commitment(32) | value_le(8) | asset_id_le(4) | owner_hash(32) let mut public_signals = vec![0u8; 76]; diff --git a/frame/shielded-pool/src/operations/private_transfer.rs b/frame/shielded-pool/src/operations/private_transfer.rs index 7bf1cb6d..28b3d8f1 100644 --- a/frame/shielded-pool/src/operations/private_transfer.rs +++ b/frame/shielded-pool/src/operations/private_transfer.rs @@ -53,15 +53,18 @@ impl PrivateTransferOperation { if nullifier.0 == [0u8; 32] { continue; // dummy input — skipped by circuit, no nullifier to check } + ensure!(nullifier.is_canonical(), Error::::InvalidPublicSignals); ensure!( !NullifierRepository::is_used::(nullifier), Error::::NullifierAlreadyUsed ); } - // Prevent both inputs being dummy (all-zero nullifiers = value 0+0). - // A 2-dummy transfer creates 2 commitments in the Merkle tree at zero cost, - // enabling spam without any economic disincentive. + for commitment in commitments.iter() { + ensure!(commitment.is_canonical(), Error::::InvalidPublicSignals); + ensure!(commitment.is_valid(), Error::::InvalidPublicSignals); + } + let all_dummy = nullifiers.iter().all(|n| n.0 == [0u8; 32]); ensure!(!all_dummy, Error::::InvalidAmount); @@ -165,12 +168,19 @@ mod tests { BoundedVec::try_from(vec![0x01u8; 72]).unwrap() } + fn canonical_bytes(seed: u8) -> [u8; 32] { + let mut b = [0u8; 32]; + b[0] = seed; + b[1] = 0xA5; // keep values apart without touching the high bytes + b + } + fn make_nullifier(seed: u8) -> Nullifier { - Nullifier::new([seed; 32]) + Nullifier::new(canonical_bytes(seed)) } fn make_commitment(seed: u8) -> Commitment { - Commitment::new([seed; 32]) + Commitment::new(canonical_bytes(seed)) } fn make_memo() -> EncryptedMemo { diff --git a/frame/shielded-pool/src/operations/shield.rs b/frame/shielded-pool/src/operations/shield.rs index 444bbd7b..3fb52bd5 100644 --- a/frame/shielded-pool/src/operations/shield.rs +++ b/frame/shielded-pool/src/operations/shield.rs @@ -28,6 +28,8 @@ impl ShieldOperation { encrypted_memo.0.len() == MAX_ENCRYPTED_MEMO_SIZE as usize, Error::::InvalidMemoSize ); + ensure!(commitment.is_canonical(), Error::::InvalidPublicSignals); + ensure!(commitment.is_valid(), Error::::InvalidPublicSignals); ensure!( !CommitmentRepository::exists::(&commitment), @@ -103,8 +105,17 @@ mod tests { EncryptedMemo::new(vec![0x01u8; 32]).unwrap() } + /// A distinct, canonical 32-byte commitment for `seed`. + /// + /// The seed goes in the low byte rather than filling all 32: a repeated high + /// byte puts the value above the BN254 modulus (`p` starts at 0x30), which + /// the canonicity guard refuses. Real commitments come out of Poseidon and + /// are always canonical. fn commitment(seed: u8) -> Commitment { - Commitment::new([seed; 32]) + let mut b = [0u8; 32]; + b[0] = seed; + b[1] = 0xA5; + Commitment::new(b) } // ── execute ─────────────────────────────────────────────────────────────── @@ -157,6 +168,78 @@ mod tests { }); } + /// A zero commitment is refused even though zero is a canonical field value. + /// + /// The tree represents an absent leaf with `[0u8; 32]`, so a stored zero is + /// indistinguishable from an empty slot when `subtree_root` rebuilds a pruned + /// level — and nobody can prove a preimage for it, so it would sit in the + /// tree forever as dead weight. The canonicity check alone lets it through; + /// this needs its own guard, which a dev-node probe caught missing. + #[test] + fn execute_zero_commitment_fails() { + new_test_ext().execute_with(|| { + let asset_id = setup_asset(); + let zero = Commitment::new([0u8; 32]); + assert!(zero.is_canonical(), "zero is canonical — that is the trap"); + + assert_noop!( + ShieldOperation::execute::(acc(1), asset_id, 500u128, zero, memo_valid()), + crate::pallet::Error::::InvalidPublicSignals + ); + }); + } + + /// The modular twin of a value must not be storable alongside it: both reduce + /// to the same field element, so accepting each would give one note two + /// identities in the tree and in the reverse index. + #[test] + fn execute_non_canonical_commitment_fails() { + new_test_ext().execute_with(|| { + use ark_bn254::Fr; + use ark_ff::{BigInteger, PrimeField}; + + let asset_id = setup_asset(); + + let mut canonical = [0u8; 32]; + canonical[0] = 7; + + // n + p: same field element, different bytes. + let p_minus_1 = (-Fr::from(1u64)).into_bigint().to_bytes_le(); + let mut twin = [0u8; 32]; + twin[..p_minus_1.len()].copy_from_slice(&p_minus_1); + let mut carry = 1u16 + 7; + for b in twin.iter_mut() { + let v = *b as u16 + carry; + *b = (v & 0xff) as u8; + carry = v >> 8; + } + + assert_eq!( + Fr::from_le_bytes_mod_order(&canonical), + Fr::from_le_bytes_mod_order(&twin), + "the pair must reduce to one element, or this proves nothing" + ); + + assert_ok!(ShieldOperation::execute::( + acc(1), + asset_id, + 500u128, + Commitment::new(canonical), + memo_valid(), + )); + assert_noop!( + ShieldOperation::execute::( + acc(1), + asset_id, + 500u128, + Commitment::new(twin), + memo_valid() + ), + crate::pallet::Error::::InvalidPublicSignals + ); + }); + } + #[test] fn execute_zero_amount_fails() { new_test_ext().execute_with(|| { diff --git a/frame/shielded-pool/src/operations/unshield.rs b/frame/shielded-pool/src/operations/unshield.rs index 203ae648..d7a443bf 100644 --- a/frame/shielded-pool/src/operations/unshield.rs +++ b/frame/shielded-pool/src/operations/unshield.rs @@ -71,6 +71,7 @@ impl UnshieldOperation { MerkleRepository::is_known_root::(&merkle_root), Error::::UnknownMerkleRoot ); + ensure!(nullifier.is_canonical(), Error::::InvalidPublicSignals); ensure!( !NullifierRepository::is_used::(&nullifier), Error::::NullifierAlreadyUsed @@ -80,6 +81,7 @@ impl UnshieldOperation { let has_change = change_commitment != [0u8; 32]; if has_change { let change_comm = Commitment::new(change_commitment); + ensure!(change_comm.is_canonical(), Error::::InvalidPublicSignals); ensure!( !CommitmentRepository::exists::(&change_comm), Error::::CommitmentAlreadyExists @@ -273,6 +275,19 @@ mod tests { const KNOWN_ROOT: [u8; 32] = [0xAAu8; 32]; + /// A distinct, canonical 32-byte field value for `seed`. + /// + /// The seed goes in the low byte rather than filling all 32: a repeated high + /// byte puts the value above the BN254 modulus (`p` starts at 0x30), which + /// the canonicity guard refuses. Real nullifiers and commitments come out of + /// Poseidon and are always canonical. + fn canonical_bytes(seed: u8) -> [u8; 32] { + let mut b = [0u8; 32]; + b[0] = seed; + b[1] = 0xA5; + b + } + fn setup_asset() -> u32 { let name = frame_support::BoundedVec::try_from(b"Orbinum".to_vec()).unwrap(); let symbol = frame_support::BoundedVec::try_from(b"ORB".to_vec()).unwrap(); @@ -291,7 +306,7 @@ mod tests { } fn nullifier(seed: u8) -> Nullifier { - Nullifier::new([seed; 32]) + Nullifier::new(canonical_bytes(seed)) } fn proof() -> &'static [u8] { @@ -678,7 +693,7 @@ mod tests { fund_pool(asset_id, 1_000u128); MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); - let change_comm_bytes = [0xCCu8; 32]; + let change_comm_bytes = canonical_bytes(0xCC); let amount = 600u128; assert_ok!(UnshieldOperation::execute::( @@ -788,7 +803,7 @@ mod tests { fund_pool(asset_id, 2_000u128); MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); - let change_comm_bytes = [0xDDu8; 32]; + let change_comm_bytes = canonical_bytes(0xDD); let change_comm = Commitment::new(change_comm_bytes); // Mark commitment as already existing in the pool. @@ -950,7 +965,7 @@ mod tests { depositor, asset_id, 1000u128, - Commitment::new([0x31u8; 32]), + Commitment::new(canonical_bytes(0x31)), FrameEncryptedMemo::from_bytes(&[0u8; 180]).unwrap(), )); assert_eq!(tracked(asset_id), pool_physical()); diff --git a/frame/shielded-pool/src/types/ids.rs b/frame/shielded-pool/src/types/ids.rs index 957de0ca..275086d2 100644 --- a/frame/shielded-pool/src/types/ids.rs +++ b/frame/shielded-pool/src/types/ids.rs @@ -40,6 +40,9 @@ impl Commitment { pub fn is_valid(&self) -> bool { self.0 != [0u8; 32] } + pub fn is_canonical(&self) -> bool { + orbinum_zk_core::FieldElement::is_canonical_le(&self.0) + } pub fn is_zero(&self) -> bool { self.0 == [0u8; 32] } @@ -91,9 +94,13 @@ impl Nullifier { pub fn new(bytes: [u8; 32]) -> Self { Self(bytes) } + pub fn validate(&self) -> bool { self.0 != [0u8; 32] } + pub fn is_canonical(&self) -> bool { + orbinum_zk_core::FieldElement::is_canonical_le(&self.0) + } pub fn as_bytes(&self) -> &[u8; 32] { &self.0 } @@ -177,3 +184,101 @@ impl core::fmt::Display for AssetId { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Little-endian encoding of `value + p`, which reduces to `value` but is a + /// different byte string — the shape a malleability attempt takes. + fn plus_modulus(value: u8) -> [u8; 32] { + use ark_bn254::Fr; + use ark_ff::{BigInteger, PrimeField}; + + // p as bytes: encode p-1, then add 1 with carry. + let p_minus_1 = (-Fr::from(1u64)).into_bigint().to_bytes_le(); + let mut out = [0u8; 32]; + out[..p_minus_1.len()].copy_from_slice(&p_minus_1); + let mut carry = 1u16 + value as u16; + for b in out.iter_mut() { + let v = *b as u16 + carry; + *b = (v & 0xff) as u8; + carry = v >> 8; + } + out + } + + /// The property the guard exists for: `n` and `n + p` are the same field + /// element but different bytes. Both are stored raw and keyed raw, so + /// accepting both would give one note two identities — and two spends. + #[test] + fn a_nullifier_and_its_modular_twin_are_not_both_accepted() { + use ark_bn254::Fr; + use ark_ff::PrimeField; + + let mut canonical = [0u8; 32]; + canonical[0] = 7; + let twin = plus_modulus(7); + + assert_ne!(canonical, twin, "the two encodings must differ as bytes"); + assert_eq!( + Fr::from_le_bytes_mod_order(&canonical), + Fr::from_le_bytes_mod_order(&twin), + "but they must be the same field element — otherwise this proves nothing" + ); + + assert!(Nullifier::new(canonical).is_canonical()); + assert!( + !Nullifier::new(twin).is_canonical(), + "n + p must be refused, or it becomes a second key for a spent note" + ); + } + + #[test] + fn a_commitment_and_its_modular_twin_are_not_both_accepted() { + let mut canonical = [0u8; 32]; + canonical[0] = 9; + assert!(Commitment::new(canonical).is_canonical()); + assert!(!Commitment::new(plus_modulus(9)).is_canonical()); + } + + /// Zero is canonical and must stay usable: an all-zero nullifier marks a + /// dummy input slot, which the circuit skips. Folding canonicity into + /// `validate` would have rejected it. + #[test] + fn zero_is_canonical_but_not_a_real_value() { + let zero_n = Nullifier::new([0u8; 32]); + assert!(zero_n.is_canonical(), "dummy inputs must survive the guard"); + assert!(!zero_n.validate(), "but zero is not a real nullifier"); + + let zero_c = Commitment::new([0u8; 32]); + assert!(zero_c.is_canonical()); + assert!(!zero_c.is_valid()); + } + + /// The modulus itself reduces to zero, so it would alias the dummy marker. + #[test] + fn the_modulus_itself_is_refused() { + let p = plus_modulus(0); + assert!(!Nullifier::new(p).is_canonical()); + assert!(!Commitment::new(p).is_canonical()); + } + + /// All-ones is the largest possible 32-byte string and far above p. + #[test] + fn all_ones_is_refused() { + assert!(!Nullifier::new([0xffu8; 32]).is_canonical()); + assert!(!Commitment::new([0xffu8; 32]).is_canonical()); + } + + /// Values the chain actually uses must pass, or the guard breaks real spends. + #[test] + fn ordinary_values_are_accepted() { + for seed in [1u8, 42, 0x7f, 0xaa] { + let mut b = [0u8; 32]; + b[0] = seed; + assert!(Nullifier::new(b).is_canonical(), "seed {seed} rejected"); + assert!(Commitment::new(b).is_canonical(), "seed {seed} rejected"); + } + } +}