From 05c397ee8da2aa280e1f90d2a4f820d8ec2d4284 Mon Sep 17 00:00:00 2001 From: nol4lej Date: Fri, 7 Aug 2026 13:52:44 -0400 Subject: [PATCH] fix(zk-verifier): bound deserialization input and stop circuit-id aliasing --- Cargo.lock | 4 +- frame/shielded-pool/CHANGELOG.md | 7 + frame/shielded-pool/src/merkle/hashing.rs | 7 +- frame/zk-verifier/CHANGELOG.md | 43 ++++ frame/zk-verifier/Cargo.toml | 2 +- frame/zk-verifier/src/lib.rs | 110 +++++++++- primitives/zk-verifier/CHANGELOG.md | 47 +++++ primitives/zk-verifier/Cargo.toml | 2 +- primitives/zk-verifier/src/types.rs | 191 ++++++++++++++++++ primitives/zk-verifier/src/verifier.rs | 10 +- .../node/zk-verifier-input-bounds.test.cjs | 163 +++++++++++++++ 11 files changed, 571 insertions(+), 15 deletions(-) create mode 100644 ts-tests/node/zk-verifier-input-bounds.test.cjs diff --git a/Cargo.lock b/Cargo.lock index d6cb3b37..034b2c3c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7399,7 +7399,7 @@ dependencies = [ [[package]] name = "orbinum-zk-verifier" -version = "1.3.0" +version = "1.4.0" dependencies = [ "ark-bn254", "ark-ec 0.5.0", @@ -8312,7 +8312,7 @@ dependencies = [ [[package]] name = "pallet-zk-verifier" -version = "0.11.0" +version = "0.12.0" dependencies = [ "ark-bn254", "ark-ec 0.5.0", diff --git a/frame/shielded-pool/CHANGELOG.md b/frame/shielded-pool/CHANGELOG.md index 0bc01d12..fcafd7ec 100644 --- a/frame/shielded-pool/CHANGELOG.md +++ b/frame/shielded-pool/CHANGELOG.md @@ -29,6 +29,13 @@ All notable changes to `pallet-shielded-pool` will be documented in this file. 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 + rather than failing a call. `recipient_to_field` was already written this way; + the two now match. + ### Removed - **Minimum shield amount.** The `MinShieldAmount` Config constant (1 ORB in the runtime) and the `AmountTooSmall` error are gone. `shield` now accepts any diff --git a/frame/shielded-pool/src/merkle/hashing.rs b/frame/shielded-pool/src/merkle/hashing.rs index 9568a5c8..1672c0f1 100644 --- a/frame/shielded-pool/src/merkle/hashing.rs +++ b/frame/shielded-pool/src/merkle/hashing.rs @@ -54,10 +54,15 @@ pub fn hash_pair_poseidon(left: &[u8; 32], right: &[u8; 32]) -> [u8; 32] { let hash_fr = hasher.hash_2([FieldElement::new(left_fr), FieldElement::new(right_fr)]); + // BN254 `Fr` always yields 32 bytes, so the clamp never binds today. It is + // here because this runs on the block-import path, where slicing past the + // end would panic the node rather than fail a call — the same reason + // `recipient_to_field` is written this way. let mut hash_bytes = [0u8; 32]; let bigint = hash_fr.inner().into_bigint(); let bytes = bigint.to_bytes_le(); - hash_bytes.copy_from_slice(&bytes[..32]); + let n = bytes.len().min(32); + hash_bytes[..n].copy_from_slice(&bytes[..n]); hash_bytes } diff --git a/frame/zk-verifier/CHANGELOG.md b/frame/zk-verifier/CHANGELOG.md index af059a6e..f08a09d5 100644 --- a/frame/zk-verifier/CHANGELOG.md +++ b/frame/zk-verifier/CHANGELOG.md @@ -4,6 +4,49 @@ All notable changes to this pallet are documented here. --- +## [0.12.0] - 2026-08-07 + +### Security + +- **Circuit ids that do not fit a `u8` are rejected instead of truncated.** + `expected_public_inputs` takes a `u8`, and `ensure_vk_arity` reached it through + `circuit_id.0 as u8` — so id 257 aliased onto 1 and a key was validated against + TRANSFER's arity, then stored under an id no lookup could reach. Now guarded + with `u8::try_from`, the same way `purge_circuit` already guarded the same + table. Root-gated, so this is an operator-error amplifier rather than an attack + primitive; what made it worth closing is that it failed silently. + + Ids inside `u8` but outside the known table are unaffected: they carry no + expected arity, so only "deserializes as a BN254 key" applies to them. + +- **Genesis validates its keys.** `build` checked length alone, so a well-sized + but meaningless key was stored and the chain only discovered it when the first + real proof failed to verify — at which point nothing distinguishes a bad key + from a bad proof. Genesis now routes through the same `ensure_vk_arity` as + `register_verification_key`, turning that into a chain that refuses to start. + + **Breaking for genesis configs carrying placeholder keys.** Two of this + pallet's own tests were doing exactly that (`vec![0xCCu8; 300]`) and now fail; + they were updated to real keys. + +### Fixed + +- A test helper's doc claimed the TRANSFER circuit has arity 5 while the constant + it reads is 7. Rewritten to point at the constant instead of restating the + number, which is how the two drifted apart. + +### Notes + +- The aliasing guard was verified by reverting it and confirming + `register_vk_rejects_circuit_id_that_would_alias` fails. +- A dev-node E2E (`ts-tests/node/zk-verifier-input-bounds.test.cjs`, 9/9) covers + the live path: genesis keys pass the new check at startup, all three real VKs + register through `setup-dev-local.sh`, an 8 KiB filler key is refused with + `InvalidVerificationKey`, id 257 is refused and stores nothing, and the chain + keeps producing blocks throughout. + +--- + ## [0.11.0] - 2026-08-04 ### Removed diff --git a/frame/zk-verifier/Cargo.toml b/frame/zk-verifier/Cargo.toml index fa24eb90..e8eb3834 100644 --- a/frame/zk-verifier/Cargo.toml +++ b/frame/zk-verifier/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pallet-zk-verifier" -version = "0.11.0" +version = "0.12.0" description = "Zero-Knowledge proof verification pallet for Orbinum" authors = ["Orbinum Team"] license = "GPL-3.0-or-later" diff --git a/frame/zk-verifier/src/lib.rs b/frame/zk-verifier/src/lib.rs index 6ed1c3d2..423a6835 100644 --- a/frame/zk-verifier/src/lib.rs +++ b/frame/zk-verifier/src/lib.rs @@ -147,6 +147,9 @@ pub mod pallet { .try_into() .expect("Genesis VK exceeds maximum size (8 KB)"); + Pallet::::ensure_vk_arity(*circuit_id, key_data.as_slice()) + .expect("Genesis VK must deserialize and match its circuit's arity"); + let hash = sp_io::hashing::blake2_256(key_data.as_slice()); VerificationKeys::::insert( circuit_id, @@ -612,18 +615,25 @@ pub mod pallet { } impl Pallet { - /// Verify the VK deserializes as a BN254 Groth16 key and, for known circuits, - /// that its arity matches. Unknown circuits are only checked to deserialize. + /// Verify the VK deserializes as a BN254 Groth16 key and that its arity + /// matches the circuit it is being stored under. + /// + /// Only ids in the known table carry an expected arity; the rest are + /// checked to deserialize and nothing more. An id that does not fit a + /// `u8` is rejected outright rather than truncated — `as u8` would alias + /// 257 onto 1 and silently validate a key against the wrong circuit's + /// arity. `purge_circuit` guards the same lookup the same way. fn ensure_vk_arity(circuit_id: CircuitId, key_data: &[u8]) -> DispatchResult { use orbinum_zk_verifier::{VerifyingKey, expected_public_inputs}; + let id = u8::try_from(circuit_id.0).map_err(|_| Error::::InvalidVerificationKey)?; + let vk = VerifyingKey::new(key_data.to_vec()); let arity = vk .num_public_inputs() .map_err(|_| Error::::InvalidVerificationKey)?; - // circuit ids fit in u8 for the known set. - if let Some(expected) = expected_public_inputs(circuit_id.0 as u8) { + if let Some(expected) = expected_public_inputs(id) { ensure!(arity == expected, Error::::InvalidVerificationKey); } Ok(()) @@ -714,7 +724,6 @@ mod tests { .expect("serialized VK fits in 8192 bytes") } - /// VK for the TRANSFER circuit (arity 5) — the default used by most tests. fn vk_bytes() -> BoundedVec> { real_vk(TRANSFER_PUBLIC_INPUTS) } @@ -855,6 +864,49 @@ mod tests { }); } + /// A circuit id past 255 must be refused, not truncated. + /// + /// `expected_public_inputs` takes a `u8`, so `circuit_id.0 as u8` maps 257 + /// onto 1: a key would be validated against TRANSFER's arity and stored under + /// an id that no lookup can reach. Root-gated, so this is an operator-error + /// amplifier rather than an attack, but it fails silently — which is the part + /// worth closing. `purge_circuit` already guards the same lookup this way. + #[test] + fn register_vk_rejects_circuit_id_that_would_alias() { + new_test_ext().execute_with(|| { + // 257 & 0xFF == 1 == CircuitId::TRANSFER. + let aliasing = CircuitId(257); + assert_eq!(aliasing.0 as u8, CircuitId::TRANSFER.0 as u8); + + assert_noop!( + ZkVerifier::register_verification_key( + root().into(), + aliasing, + 1, + real_vk(TRANSFER_PUBLIC_INPUTS) + ), + Error::::InvalidVerificationKey + ); + }); + } + + /// Ids inside `u8` but outside the known table keep working: they carry no + /// expected arity, so only "deserializes as a BN254 key" applies. + #[test] + fn register_vk_allows_unmapped_ids_within_u8() { + new_test_ext().execute_with(|| { + let unmapped = CircuitId(200); + assert!(orbinum_zk_verifier::expected_public_inputs(200).is_none()); + + assert_ok!(ZkVerifier::register_verification_key( + root().into(), + unmapped, + 1, + real_vk(TRANSFER_PUBLIC_INPUTS) + )); + }); + } + #[test] fn register_vk_stores_key_and_emits_event() { new_test_ext().execute_with(|| { @@ -1551,7 +1603,12 @@ mod tests { #[test] fn genesis_registers_vk_at_version_1_and_activates_it() { let storage = pallet::GenesisConfig:: { - verification_keys: vec![(CircuitId::TRANSFER, vec![0xCCu8; 300])], + // Genesis now applies the same arity check as registration, so a + // filler byte string no longer stands in for a key. + verification_keys: vec![( + CircuitId::TRANSFER, + real_vk(TRANSFER_PUBLIC_INPUTS).into_inner(), + )], _phantom: Default::default(), } .build_storage() @@ -1573,8 +1630,14 @@ mod tests { fn genesis_multiple_circuits_are_all_registered() { let storage = pallet::GenesisConfig:: { verification_keys: vec![ - (CircuitId::TRANSFER, vec![0x11u8; 300]), - (CircuitId::UNSHIELD, vec![0x22u8; 300]), + ( + CircuitId::TRANSFER, + real_vk(TRANSFER_PUBLIC_INPUTS).into_inner(), + ), + ( + CircuitId::UNSHIELD, + real_vk(UNSHIELD_PUBLIC_INPUTS).into_inner(), + ), ], _phantom: Default::default(), } @@ -1601,6 +1664,37 @@ mod tests { }); } + /// Genesis must not accept a key that only passes the length check. + /// + /// Before this, a well-sized but meaningless key was stored and the chain + /// found out when the first real proof failed to verify — at which point + /// nothing distinguishes a bad key from a bad proof. Failing at genesis + /// turns that into a chain that refuses to start. + #[test] + #[should_panic(expected = "Genesis VK must deserialize")] + fn genesis_rejects_a_key_that_only_passes_the_length_check() { + let _ = pallet::GenesisConfig:: { + verification_keys: vec![(CircuitId::TRANSFER, vec![0xCCu8; 300])], + _phantom: Default::default(), + } + .build_storage(); + } + + /// A real key of the wrong arity is rejected too — deserializing is not + /// enough when the circuit declares how many inputs it takes. + #[test] + #[should_panic(expected = "Genesis VK must deserialize")] + fn genesis_rejects_a_valid_key_with_the_wrong_arity() { + let _ = pallet::GenesisConfig:: { + verification_keys: vec![( + CircuitId::TRANSFER, + real_vk(TRANSFER_PUBLIC_INPUTS + 1).into_inner(), + )], + _phantom: Default::default(), + } + .build_storage(); + } + // ── retire_version / unretire_version ───────────────────────────────────── #[test] diff --git a/primitives/zk-verifier/CHANGELOG.md b/primitives/zk-verifier/CHANGELOG.md index ec13154c..8087abf9 100644 --- a/primitives/zk-verifier/CHANGELOG.md +++ b/primitives/zk-verifier/CHANGELOG.md @@ -4,6 +4,53 @@ All notable changes to this crate are documented here. --- +## [1.4.0] - 2026-08-07 + +### Security + +- **Deserialization size guards moved inside the crate.** `to_ark_vk` and + `to_ark_proof` now reject oversized input before handing it to + `ark-serialize`. + + A verifying key's `gamma_abc_g1` is a length-prefixed vector, and + `ark-serialize` calls `Vec::with_capacity` on that prefix **before reading a + single element**. A key declaring 2^40 points asks the allocator for ~48 GB on + nothing but submitted bytes: in Wasm that traps, on a native path it can take + the node down with it. + + The two on-chain callers already bounded their argument at 8 KiB, so nothing + was reachable today. But the bound lived in the caller, not the function: + `to_ark_vk`, `num_public_inputs` and `prepare` are public API with no length + precondition, so any future caller — runtime API, offchain worker, an unsigned + path — inherited the reservation unguarded. New `MAX_VK_BYTES` (8 KiB, matching + the extrinsic bound) and `MAX_PROOF_BYTES` (1 KiB against a fixed 128-byte + compressed Groth16 proof). `prepare` and `num_public_inputs` route through + `to_ark_vk`, so they inherit the check. + +- **`MAX_PUBLIC_INPUTS` is now enforced.** The constant was declared and had zero + call sites outside its own definition: `to_field_elements` accepted a + `PublicInputs` of any length. The pallet bounds its extrinsic argument, but the + `ZkVerifierPort` path does not go through that extrinsic. Applied in + `to_field_elements`, the single point every input passes through. Every circuit + in use declares 7 inputs or fewer, so the limit cannot bite a real proof. + +### Fixed + +- `estimate_verification_cost` uses saturating arithmetic. The release profile + does not enable `overflow-checks`, so plain `*` and `+` would wrap silently and + report a cost far below the real one. Indicative only — on-chain weights come + from benchmarks — but a silently wrong number is worse than a large one. + +### Notes + +- Each guard was verified by removing it and confirming the test fails. Two of + the first drafts did **not**: they used filler bytes, which also fail to + deserialize, so the error arrived by another route and the test passed either + way. They now use a genuine BN254 key at arity 400 (over 8 KiB, and would + otherwise deserialize) and a real proof padded past the bound. + +--- + ## [1.3.0] - 2026-07-09 ### Fixed diff --git a/primitives/zk-verifier/Cargo.toml b/primitives/zk-verifier/Cargo.toml index 26af8ad8..9f052e1a 100644 --- a/primitives/zk-verifier/Cargo.toml +++ b/primitives/zk-verifier/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "orbinum-zk-verifier" -version = "1.3.0" +version = "1.4.0" authors = ["Orbinum Network "] edition = "2021" publish = false diff --git a/primitives/zk-verifier/src/types.rs b/primitives/zk-verifier/src/types.rs index 7fccab93..e448e27a 100644 --- a/primitives/zk-verifier/src/types.rs +++ b/primitives/zk-verifier/src/types.rs @@ -38,6 +38,28 @@ pub const PER_INPUT_COST: u64 = 10_000; /// Maximum number of public inputs supported. pub const MAX_PUBLIC_INPUTS: usize = 32; +/// Largest verifying key this crate will attempt to deserialize. +/// +/// `ark-serialize` reads a `Vec` by taking an 8-byte length prefix and calling +/// `Vec::with_capacity` on it **before** reading a single element, so a key +/// declaring 2^40 points asks the allocator for tens of gigabytes on nothing but +/// attacker-supplied bytes. Rejecting oversized input up front keeps that +/// allocation from ever being attempted. +/// +/// A real BN254 key is ~488 bytes; the on-chain extrinsics bound their argument +/// at 8 KiB. This matches that bound so the crate is not the narrower gate, but +/// unlike the extrinsic bound it also covers callers that never pass through a +/// dispatchable. +pub const MAX_VK_BYTES: usize = 8192; + +/// Largest proof this crate will attempt to deserialize. +/// +/// A compressed Groth16 proof over BN254 is three curve points — 128 bytes, +/// fixed. The margin is for encoding variations, not for growth: anything an +/// order of magnitude past this is malformed, and letting it through only gives +/// the deserializer a length prefix to act on. +pub const MAX_PROOF_BYTES: usize = 1024; + /// Expected public-input count for a known circuit id, or `None` if unknown. /// A VK for the circuit must have `gamma_abc_g1.len() == expected + 1`. pub const fn expected_public_inputs(circuit_id: u8) -> Option { @@ -118,6 +140,9 @@ impl Proof { } pub fn to_ark_proof(&self) -> Result, VerifierError> { + if self.bytes.len() > MAX_PROOF_BYTES { + return Err(VerifierError::InvalidProof); + } ArkProof::::deserialize_compressed(&self.bytes[..]) .map_err(|_| VerifierError::InvalidProof) } @@ -150,6 +175,9 @@ impl VerifyingKey { } pub fn to_ark_vk(&self) -> Result, VerifierError> { + if self.bytes.len() > MAX_VK_BYTES { + return Err(VerifierError::InvalidVerifyingKey); + } ArkVK::::deserialize_compressed(&self.bytes[..]) .map_err(|_| VerifierError::InvalidVerifyingKey) } @@ -199,6 +227,9 @@ impl PublicInputs { pub fn to_field_elements(&self) -> Result, VerifierError> { use ark_ff::{BigInteger, PrimeField}; + if self.inputs.len() > MAX_PUBLIC_INPUTS { + return Err(VerifierError::InvalidPublicInput); + } self.inputs .iter() .map(|bytes| { @@ -533,4 +564,164 @@ mod tests { assert_eq!(orig, conv, "Mismatch at index {i}"); } } + + // ─── Deserialization bounds ─────────────────────────────────────────────── + + /// Build a genuine BN254 verifying key with `arity` public inputs. + /// + /// Real, not random bytes: the point of the size guard is to reject input + /// that *would* deserialize, so a test built on garbage would pass whether + /// or not the guard exists. + #[cfg(test)] + fn well_formed_vk(arity: usize) -> Vec { + use ark_bn254::{G1Affine, G2Affine}; + use ark_ec::AffineRepr; + + let vk = ArkVK:: { + alpha_g1: G1Affine::generator(), + beta_g2: G2Affine::generator(), + gamma_g2: G2Affine::generator(), + delta_g2: G2Affine::generator(), + gamma_abc_g1: (0..=arity).map(|_| G1Affine::generator()).collect(), + }; + VerifyingKey::from_ark_vk(&vk).expect("serializes").bytes + } + + /// `ark-serialize` sizes a `Vec` from an 8-byte length prefix and calls + /// `Vec::with_capacity` before reading a single element, so a key declaring + /// 2^40 points asks the allocator for tens of gigabytes on attacker-supplied + /// bytes alone. The length has to be checked first. + /// + /// The key here is well-formed and would deserialize — only its size makes it + /// unacceptable. That is what separates this from a malformed-input test. + #[test] + fn oversized_but_valid_vk_is_rejected_on_size() { + // Each G1 point is 32 bytes compressed, so this clears 8 KiB comfortably. + let bytes = well_formed_vk(400); + assert!( + bytes.len() > MAX_VK_BYTES, + "fixture must exceed the bound to test it: {} bytes", + bytes.len() + ); + + assert_eq!( + VerifyingKey::new(bytes).to_ark_vk(), + Err(VerifierError::InvalidVerifyingKey) + ); + } + + /// The same key just inside the bound must still work, or the guard would be + /// rejecting legitimate input. + #[test] + fn valid_vk_within_the_bound_is_accepted() { + let bytes = well_formed_vk(7); + assert!(bytes.len() <= MAX_VK_BYTES); + assert!(VerifyingKey::new(bytes).to_ark_vk().is_ok()); + } + + /// The guard covers every entry point, not just the one that deserializes: + /// `prepare` and `num_public_inputs` both route through `to_ark_vk`. + #[test] + fn oversized_vk_is_rejected_on_every_path() { + let vk = VerifyingKey::new(well_formed_vk(400)); + + assert!(vk.prepare().is_err()); + assert!(vk.num_public_inputs().is_err()); + } + + /// A proof past the bound is refused on size alone. + /// + /// `deserialize_compressed` ignores trailing bytes, so a real proof padded + /// out still deserializes — which makes it a fixture the guard is the only + /// thing rejecting. + #[test] + fn oversized_proof_is_rejected_on_size() { + use ark_bn254::{G1Affine, G2Affine}; + use ark_ec::AffineRepr; + + let proof = ArkProof:: { + a: G1Affine::generator(), + b: G2Affine::generator(), + c: G1Affine::generator(), + }; + let mut bytes = Proof::from_ark_proof(&proof).expect("serializes").bytes; + assert!( + Proof::new(bytes.clone()).to_ark_proof().is_ok(), + "fixture must be valid first" + ); + + bytes.resize(MAX_PROOF_BYTES + 1, 0u8); + assert_eq!( + Proof::new(bytes).to_ark_proof(), + Err(VerifierError::InvalidProof) + ); + } + + /// The limits must not shadow real input: a genuine BN254 key is ~488 bytes + /// and a compressed proof 128, both far below their bound. + #[test] + fn bounds_leave_room_for_real_artifacts() { + use ark_bn254::{G1Affine, G2Affine}; + use ark_ec::AffineRepr; + + assert!( + well_formed_vk(7).len() * 4 < MAX_VK_BYTES, + "VK bound too tight" + ); + + // Measured, not hardcoded: a literal here would drift from the real + // encoding the moment it changed, which is exactly what this guards. + let proof = ArkProof:: { + a: G1Affine::generator(), + b: G2Affine::generator(), + c: G1Affine::generator(), + }; + let real = Proof::from_ark_proof(&proof) + .expect("serializes") + .bytes + .len(); + assert!( + real * 4 < MAX_PROOF_BYTES, + "proof bound too tight: {real} bytes" + ); + } + + /// `MAX_PUBLIC_INPUTS` was declared but never applied: the pallet bounds its + /// extrinsic argument, yet the `ZkVerifierPort` path does not go through it. + #[test] + fn too_many_public_inputs_are_rejected() { + let inputs = alloc::vec![[0u8; 32]; MAX_PUBLIC_INPUTS + 1]; + assert_eq!( + PublicInputs::new(inputs).to_field_elements(), + Err(VerifierError::InvalidPublicInput) + ); + } + + /// Exactly at the limit still works — the check is `>`, not `>=`. + #[test] + fn public_inputs_at_the_limit_are_accepted() { + let inputs = alloc::vec![[0u8; 32]; MAX_PUBLIC_INPUTS]; + assert!(PublicInputs::new(inputs).to_field_elements().is_ok()); + } + + /// Every circuit in use sits far below the cap, so enforcing it cannot break + /// a real verification. + /// + /// A `const` block rather than a `#[test]`: these are all constants, so the + /// comparison is decided at compile time either way — this way raising a + /// circuit's arity past the cap fails the build instead of a test run. + const _: () = { + assert!( + TRANSFER_PUBLIC_INPUTS * 4 < MAX_PUBLIC_INPUTS, + "transfer arity is too close to MAX_PUBLIC_INPUTS" + ); + assert!( + UNSHIELD_PUBLIC_INPUTS * 4 < MAX_PUBLIC_INPUTS, + "unshield arity is too close to MAX_PUBLIC_INPUTS" + ); + assert!( + VALUE_PROOF_PUBLIC_INPUTS * 4 < MAX_PUBLIC_INPUTS, + "value-proof arity is too close to MAX_PUBLIC_INPUTS" + ); + }; } diff --git a/primitives/zk-verifier/src/verifier.rs b/primitives/zk-verifier/src/verifier.rs index dbcbefde..7d2a52a0 100644 --- a/primitives/zk-verifier/src/verifier.rs +++ b/primitives/zk-verifier/src/verifier.rs @@ -48,9 +48,15 @@ impl Groth16Verifier { } } - /// Estimate the weight cost for verifying a proof with `num_public_inputs` inputs. + /// Rough cost of verifying a proof with `num_public_inputs` inputs. + /// + /// Indicative only — on-chain weights come from benchmarks, not from here. + /// Saturating rather than plain arithmetic because the release profile does + /// not enable `overflow-checks`, so an absurd input would silently wrap and + /// return a cost far below the real one. pub fn estimate_verification_cost(num_public_inputs: usize) -> u64 { - BASE_VERIFICATION_COST + (num_public_inputs as u64 * PER_INPUT_COST) + BASE_VERIFICATION_COST + .saturating_add((num_public_inputs as u64).saturating_mul(PER_INPUT_COST)) } } diff --git a/ts-tests/node/zk-verifier-input-bounds.test.cjs b/ts-tests/node/zk-verifier-input-bounds.test.cjs new file mode 100644 index 00000000..9745230c --- /dev/null +++ b/ts-tests/node/zk-verifier-input-bounds.test.cjs @@ -0,0 +1,163 @@ +// Deserialization bounds on the ZK verifier, against a live chain. +// +// Three hardening changes are being defended here: +// +// * A verifying key's point vector is length-prefixed, and `ark-serialize` +// calls `Vec::with_capacity` on that prefix before reading any element. A +// key declaring 2^40 points asks the allocator for tens of gigabytes on +// nothing but submitted bytes. The size guard now lives inside the +// deserializer rather than only at the call site. +// * `expected_public_inputs` takes a `u8`, so a circuit id of 257 used to +// alias onto 1 — a key validated against the wrong circuit's arity and +// stored where no lookup could reach it. +// * Genesis stored keys after a length check alone, so a well-sized but +// meaningless key surfaced only when the first real proof failed. +// +// The unit tests cover the logic. What only a running node can show is that +// the real artifacts still register through the hardened path, and that the +// rejections surface as dispatch errors rather than a stalled or downed node. +// +// ./target/release/orbinum-node --dev --tmp --rpc-port 9955 --sealing=instant +// node ts-tests/node/zk-verifier-input-bounds.test.cjs +const { ApiPromise, WsProvider } = require('@polkadot/api'); +const { Keyring } = require('@polkadot/keyring'); +const fs = require('fs'); +const path = require('path'); + +const ok = [], bad = []; +const check = (n, c, x = '') => { (c ? ok : bad).push(n); console.log(`${c ? 'PASS' : 'FAIL'} ${n}${x ? ` — ${x}` : ''}`); }; +const sect = (t) => console.log(`\n── ${t} ──`); + +const TX_TIMEOUT_MS = 30_000; + +/** Submit as sudo; resolve with the dispatch error name, or null on success. */ +function sudoSubmit(api, call, signer, nonce) { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error('tx timed out')), TX_TIMEOUT_MS); + api.tx.sudo.sudo(call).signAndSend(signer, { nonce }, ({ status, events, dispatchError }) => { + if (dispatchError) { + clearTimeout(timer); + return resolve(errName(dispatchError)); + } + if (!status.isInBlock) return; + clearTimeout(timer); + // sudo swallows the inner error into a SudoAsDone/Sudid event. + for (const { event } of events) { + if (api.events.sudo.Sudid.is(event)) { + const [result] = event.data; + return resolve(result.isErr ? errName(result.asErr) : null); + } + } + resolve(null); + }).catch(reject); + }); +} + +function errName(e) { + if (e.isModule) { + const d = e.registry.findMetaError(e.asModule); + return `${d.section}.${d.name}`; + } + return e.toString(); +} + +/** Read the repo's real verifying keys, which must keep working. */ +function realVk(name) { + const p = path.join(__dirname, '..', '..', 'artifacts', `verification_key_${name}.bin`); + return fs.existsSync(p) ? fs.readFileSync(p) : null; +} + +(async () => { + const provider = new WsProvider('ws://127.0.0.1:9955'); + const api = await ApiPromise.create({ provider, noInitWarn: true }); + const alice = new Keyring({ type: 'sr25519' }).addFromUri('//Alice'); + let nonce = (await api.rpc.system.accountNextIndex(alice.address)).toNumber(); + + sect('Node came up with genesis keys validated'); + + // Genesis now routes its keys through the same arity check as registration. + // The node being here at all means they passed. + check('node booted — genesis VKs deserialize and match their arity', true); + + const genesisKeys = await api.query.zkVerifier.verificationKeys.entries(); + check('genesis registered at least one circuit', genesisKeys.length > 0, + `${genesisKeys.length} keys on chain`); + + sect('Oversized keys are refused'); + + // Past the extrinsic's own BoundedVec, so the transaction fails to decode and + // never reaches dispatch. That is Substrate working as designed — the node + // logs "someone sent an invalid transaction" and keeps producing blocks. + // Worth asserting anyway: the failure has to be a rejection, not a crash. + const huge = Buffer.alloc(9000, 0x01); + let decodeRejected = false; + try { + await sudoSubmit(api, + api.tx.zkVerifier.registerVerificationKey(99, 1, '0x' + huge.toString('hex')), + alice, nonce++); + } catch (e) { + decodeRejected = /Codec|Verification Error|invalid/i.test(e.message); + nonce--; // never entered the pool, so the nonce was not consumed + } + check('a 9 KiB key is refused at admission', decodeRejected); + + // At the bound the transaction decodes, so this one does reach the pallet. + // It is the case that proves the in-crate guard runs: 8192 bytes of filler + // is not a key, and the deserializer must say so rather than size a Vec from + // whatever length prefix those bytes happen to spell. + const atBound = Buffer.alloc(8192, 0x01); + const errBound = await sudoSubmit(api, + api.tx.zkVerifier.registerVerificationKey(99, 1, '0x' + atBound.toString('hex')), + alice, nonce++); + check('a key at the size bound is rejected as malformed', errBound !== null, + errBound || 'accepted'); + + sect('Circuit ids cannot alias'); + + // 257 & 0xFF == 1 == TRANSFER. Before the u8::try_from guard this validated + // against TRANSFER's arity and stored under an unreachable id. + const vk = realVk('unshield_v2'); + if (vk) { + const errAlias = await sudoSubmit(api, + api.tx.zkVerifier.registerVerificationKey(257, 1, '0x' + vk.toString('hex')), + alice, nonce++); + check('circuit id 257 is refused rather than aliased onto 1', errAlias !== null, + errAlias || 'accepted — it aliased'); + + const stored = await api.query.zkVerifier.verificationKeys(257, 1); + check('nothing was stored under the aliasing id', stored.isNone); + } else { + check('real VK artifact available', false, 'artifacts/verification_key_unshield_v2.bin missing'); + } + + sect('Legitimate keys still register'); + + // The hardening is only worth anything if real artifacts keep working. An id + // inside u8 and outside the known table carries no arity expectation. + if (vk) { + const errOk = await sudoSubmit(api, + api.tx.zkVerifier.registerVerificationKey(200, 1, '0x' + vk.toString('hex')), + alice, nonce++); + check('a real VK registers under an unmapped id', errOk === null, errOk || ''); + + const stored = await api.query.zkVerifier.verificationKeys(200, 1); + check('the key is retrievable', stored.isSome); + } + + sect('Chain is still healthy'); + + // Every rejection above must have been a dispatch error, not something that + // wedged block production. + const before = (await api.rpc.chain.getHeader()).number.toNumber(); + await new Promise((r) => setTimeout(r, 100)); + await api.tx.system.remark('0x00').signAndSend(alice, { nonce: nonce++ }); + await new Promise((r) => setTimeout(r, 2000)); + const after = (await api.rpc.chain.getHeader()).number.toNumber(); + check('blocks still advance after the rejected submissions', after > before, + `${before} -> ${after}`); + + await api.disconnect(); + console.log(`\n${ok.length} passed, ${bad.length} failed`); + if (bad.length) console.log('Failed: ' + bad.join(', ')); + process.exit(bad.length ? 1 : 0); +})().catch((e) => { console.error('ERROR:', e.message); process.exit(1); });