diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 4637f53b..df40c311 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -58,7 +58,7 @@ jobs: ${{ runner.os }}-cargo- - name: Run sccache - uses: mozilla-actions/sccache-action@v0.0.9 + uses: mozilla-actions/sccache-action@v0.0.11 - name: Install Rust toolchain run: make setup diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a6f9ed66..b5039775 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -125,7 +125,7 @@ jobs: ${{ runner.os }}-cargo- - name: Run sccache - uses: mozilla-actions/sccache-action@v0.0.9 + uses: mozilla-actions/sccache-action@v0.0.11 - name: Install Rust toolchain run: make setup diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7654d59e..6749e6ec 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -67,7 +67,7 @@ jobs: ${{ runner.os }}-cargo- - name: Run sccache - uses: mozilla-actions/sccache-action@v0.0.9 + uses: mozilla-actions/sccache-action@v0.0.11 - name: Install Rust toolchain run: make setup @@ -119,7 +119,7 @@ jobs: ${{ runner.os }}-cargo- - name: Run sccache - uses: mozilla-actions/sccache-action@v0.0.9 + uses: mozilla-actions/sccache-action@v0.0.11 - name: Install Rust toolchain run: make setup @@ -175,7 +175,7 @@ jobs: fail-on-cache-miss: true - name: Run sccache - uses: mozilla-actions/sccache-action@v0.0.9 + uses: mozilla-actions/sccache-action@v0.0.11 - name: Install Rust toolchain run: make setup @@ -223,7 +223,7 @@ jobs: fail-on-cache-miss: true - name: Run sccache - uses: mozilla-actions/sccache-action@v0.0.9 + uses: mozilla-actions/sccache-action@v0.0.11 - name: Install Rust toolchain run: make setup diff --git a/Cargo.lock b/Cargo.lock index 68664480..ff0b3424 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3365,6 +3365,7 @@ dependencies = [ "libsecp256k1", "log", "pallet-evm", + "pallet-evm-precompile-shielded-pool", "pallet-relayer-runtime-api", "pallet-shielded-pool-runtime-api", "parity-scale-codec", @@ -7894,7 +7895,7 @@ dependencies = [ [[package]] name = "pallet-evm-precompile-shielded-pool" -version = "0.4.0" +version = "0.6.0" dependencies = [ "fp-evm", "frame-support", @@ -8103,7 +8104,7 @@ dependencies = [ [[package]] name = "pallet-shielded-pool" -version = "0.17.0" +version = "0.18.0" dependencies = [ "ark-bn254", "ark-ff 0.5.0", diff --git a/client/rpc/Cargo.toml b/client/rpc/Cargo.toml index 4b3562ae..35da8066 100644 --- a/client/rpc/Cargo.toml +++ b/client/rpc/Cargo.toml @@ -66,6 +66,10 @@ pallet-shielded-pool-runtime-api = { workspace = true, features = ["std"] } [dev-dependencies] tempfile = "3.21.0" +# Pins the relay's selector whitelist against the precompile's own constants +# (see relay::validation tests). A dev-dependency only: the relay must not +# depend on runtime crates at build time. +pallet-evm-precompile-shielded-pool = { workspace = true, features = ["std"] } # Substrate sc-block-builder = { workspace = true } sc-client-db = { workspace = true, features = ["rocksdb"] } diff --git a/client/rpc/src/relay/mod.rs b/client/rpc/src/relay/mod.rs index 28d174cf..b76974ee 100644 --- a/client/rpc/src/relay/mod.rs +++ b/client/rpc/src/relay/mod.rs @@ -9,7 +9,7 @@ //! //! The relay only accepts calls to the ShieldedPool precompile //! (`0x0000000000000000000000000000000000000801`) with selector -//! `0x47fc44a2` (unshield) or `0x8c0f5d24` (privateTransfer). +//! `0x4e505348` (unshield) or `0x1ec439cf` (privateTransfer). //! It checks the fee embedded in ABI slot 6 is ≥ the current `min_relay_fee` from //! `pallet-relayer` (queried dynamically via Runtime API so forkless upgrades take effect immediately). //! @@ -127,11 +127,15 @@ where // Compute 2× gas floor: the relay must earn at least twice what it spends on EVM gas. // 1 wei == 1 plank in Orbinum, so no unit conversion is required. + // Saturate rather than `as_u128()`, which panics on a gas_price ≥ 2^128. + // The value comes from the runtime, not calldata, but a panic here would + // still take down the relay RPC — mirror the fee-word hardening in + // operations.rs::fee_at_slot_6. let base_fee_wei: u128 = self .client .runtime_api() .gas_price(best_hash) - .map(|p| p.as_u128()) + .map(|p| p.try_into().unwrap_or(u128::MAX)) .unwrap_or(0); let effective_min_fee = compute_effective_min_fee(min_fee_planck, base_fee_wei); @@ -275,7 +279,12 @@ where .map(|cfg| cfg.min_fee_planck) .unwrap_or(MIN_RELAY_FEE_FALLBACK); - let base_fee_wei: u128 = api.gas_price(best_hash).map(|p| p.as_u128()).unwrap_or(0); + // Saturate rather than `as_u128()` (panics ≥ 2^128) — see the sibling + // call above; runtime-sourced, but a panic still kills the relay RPC. + let base_fee_wei: u128 = api + .gas_price(best_hash) + .map(|p| p.try_into().unwrap_or(u128::MAX)) + .unwrap_or(0); let min_fee = compute_effective_min_fee(min_fee_planck, base_fee_wei); let balance = { diff --git a/client/rpc/src/relay/operations.rs b/client/rpc/src/relay/operations.rs index 05feab1b..5f0c2a47 100644 --- a/client/rpc/src/relay/operations.rs +++ b/client/rpc/src/relay/operations.rs @@ -11,10 +11,16 @@ use ethereum_types::U256; /// 4-byte ABI selector for `unshield(...)`. -pub(crate) const SELECTOR_UNSHIELD: [u8; 4] = [0x47, 0xfc, 0x44, 0xa2]; +/// `keccak256("unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256,bytes32,bytes,uint32)")[0..4]`. +/// Must match the precompile's `calls::unshield::SELECTOR` — see the +/// consistency test in `validation.rs`. +pub(crate) const SELECTOR_UNSHIELD: [u8; 4] = [0x4e, 0x50, 0x53, 0x48]; /// 4-byte ABI selector for `privateTransfer(...)`. -pub(crate) const SELECTOR_PRIVATE_TRANSFER: [u8; 4] = [0x8c, 0x0f, 0x5d, 0x24]; +/// `keccak256("privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)")[0..4]`. +/// Must match the precompile's `calls::private_transfer::SELECTOR` — see the +/// consistency test in `validation.rs` (guards against ME-8 recurring). +pub(crate) const SELECTOR_PRIVATE_TRANSFER: [u8; 4] = [0x1e, 0xc4, 0x39, 0xcf]; /// Describes how to validate calldata for a specific relayable on-chain operation. /// @@ -38,9 +44,32 @@ pub(crate) trait RelayableOperation: Send + Sync { fn extract_fee(&self, calldata: &[u8]) -> u128; } -/// `unshield(proof, root, nullifier, asset_id, amount, recipient, fee)` — `0x47fc44a2` +/// Reads the relay fee from ABI slot 6 (`calldata[196..228]`), the position both +/// operations share. /// -/// Fee is in ABI slot 6: `calldata[196..228]`. +/// Saturates instead of panicking on a value above `u128::MAX`. Calldata reaches +/// this from an unauthenticated RPC call, and `U256::as_u128` panics outright on +/// anything wider — one crafted 32-byte word would take down the handler. +/// Saturating is safe because the result is only ever compared against the fee +/// floor: an absurd fee clears it here and is then rejected by the EVM dry-run, +/// which is what would have happened anyway. +/// +/// The slice is bounds-checked by the caller's `min_calldata_len()` gate (292 or +/// 324, both well past 228). +fn fee_at_slot_6(calldata: &[u8]) -> u128 { + let Ok(bytes) = <[u8; 32]>::try_from(&calldata[196..228]) else { + return 0; // unreachable behind the length gate; a zero fee fails the floor + }; + U256::from_big_endian(&bytes) + .try_into() + .unwrap_or(u128::MAX) +} + +/// `unshield(proof, root, nullifier, asset_id, amount, recipient, fee, +/// change_commitment, change_encrypted_memo, circuit_version)` — `0x4e505348` +/// +/// Fee is in ABI slot 6: `calldata[196..228]`. The head is 10 slots (320 bytes) +/// plus the 4-byte selector = 324 minimum. pub(crate) struct UnshieldOp; impl RelayableOperation for UnshieldOp { @@ -53,18 +82,19 @@ impl RelayableOperation for UnshieldOp { } fn min_calldata_len(&self) -> usize { - 228 + 324 } fn extract_fee(&self, calldata: &[u8]) -> u128 { - let bytes: [u8; 32] = calldata[196..228].try_into().unwrap(); - U256::from_big_endian(&bytes).as_u128() + fee_at_slot_6(calldata) } } -/// `privateTransfer(proof, root, nullifiers, commitments, memos, asset_id, fee)` — `0x8c0f5d24` +/// `privateTransfer(proof, root, nullifiers, commitments, memos, asset_id, fee, +/// circuit_version, ovk_blob)` — `0x1ec439cf` /// -/// Fee is in ABI slot 6: `calldata[196..228]`. +/// Fee is in ABI slot 6: `calldata[196..228]`. The head is 9 slots (288 bytes) +/// plus the 4-byte selector = 292 minimum. pub(crate) struct PrivateTransferOp; impl RelayableOperation for PrivateTransferOp { @@ -77,12 +107,11 @@ impl RelayableOperation for PrivateTransferOp { } fn min_calldata_len(&self) -> usize { - 228 + 292 } fn extract_fee(&self, calldata: &[u8]) -> u128 { - let bytes: [u8; 32] = calldata[196..228].try_into().unwrap(); - U256::from_big_endian(&bytes).as_u128() + fee_at_slot_6(calldata) } } diff --git a/client/rpc/src/relay/validation.rs b/client/rpc/src/relay/validation.rs index 3793af5b..acef2b1c 100644 --- a/client/rpc/src/relay/validation.rs +++ b/client/rpc/src/relay/validation.rs @@ -33,9 +33,12 @@ pub(crate) const RELAY_GAS_LIMIT: u64 = 2_000_000; pub(crate) const MIN_RELAY_FEE_FALLBACK: u128 = 1_000_000_000_000_000; // 0.001 ORB in planck /// Static fallback selector whitelist for when the Runtime API is unavailable. +/// +/// Built from the operation constants rather than re-typed, so this list cannot +/// drift from what `default_operations` actually dispatches on. pub(crate) const SELECTORS_FALLBACK: [[u8; 4]; 2] = [ - [0x47, 0xfc, 0x44, 0xa2], // unshield - [0x8c, 0x0f, 0x5d, 0x24], // privateTransfer + super::operations::SELECTOR_UNSHIELD, + super::operations::SELECTOR_PRIVATE_TRANSFER, ]; /// Maximum calldata size accepted by the relay (32 KB). @@ -75,7 +78,8 @@ pub(crate) fn compute_effective_min_fee(min_fee_planck: u128, base_fee_wei: u128 /// (or `MIN_RELAY_FEE_FALLBACK` if the API is unavailable). /// `allowed_selectors` — from `relay_config().allowed_selectors`. /// -/// Both `unshield` and `privateTransfer` share the same ABI head layout: +/// Both `unshield` and `privateTransfer` agree up to slot 6, which is all this +/// function reads: /// ```text /// bytes [0..4] selector /// bytes [4..36] slot 0 — offset pointer for proof (bytes/dynamic) @@ -86,7 +90,9 @@ pub(crate) fn compute_effective_min_fee(min_fee_planck: u128, base_fee_wei: u128 /// bytes [164..196] slot 5 — bytes32 recipient / uint32 asset_id /// bytes [196..228] slot 6 — uint256 fee ← checked here /// ``` -/// Minimum head size = 4 + 7 × 32 = 228 bytes. +/// Past slot 6 the layouts diverge, so the 228-byte minimum above is only a +/// cheap first gate: each operation declares its own `min_calldata_len()` +/// (unshield 324, privateTransfer 292), checked after selector dispatch. pub(crate) fn validate_relay_calldata( data: &[u8], min_fee_wei: u128, @@ -152,7 +158,7 @@ mod tests { /// Build minimal valid calldata for the given selector and fee. /// - /// Head layout (228 bytes total): + /// Head layout (first 228 bytes; the buffer is padded to the largest op's head): /// ```text /// [0..4] selector /// [4..36] slot 0 — proof offset: 0xE0 (= 7×32 = 224, past all head slots) @@ -164,7 +170,10 @@ mod tests { /// [196..228] slot 6 — uint256 fee /// ``` fn build_calldata(selector: [u8; 4], fee_wei: u128) -> Vec { - let mut data = vec![0u8; 228]; + // unshield's head is 10 slots (4 + 320 = 324 bytes); privateTransfer's is + // 9 slots (292). Build the larger of the two — extra zero head slots are + // harmless, the fee stays at slot 6 either way. + let mut data = vec![0u8; 324]; data[..4].copy_from_slice(&selector); // Proof-bytes offset: 7×32 = 224 = 0xE0 (big-endian U256 → only last byte set) data[35] = 0xE0; @@ -203,6 +212,53 @@ mod tests { // ── Selector checks ──────────────────────────────────────────────────── + /// The whitelist must carry the selectors the precompile actually decodes. + /// + /// This is the ME-8 guard. It compares against the DECODER's own constants, + /// not against a signature string copied into this file — two copies of a + /// signature can drift together and a test on each side would still pass. + /// The failure is silent: a wrong selector is merely "unsupported", so the + /// rejection tests stay green while relaying stops working entirely. + #[test] + fn whitelist_selectors_match_the_precompile_decoder() { + use pallet_evm_precompile_shielded_pool::selectors; + + assert_eq!(SELECTOR_PRIVATE_TRANSFER, selectors::PRIVATE_TRANSFER); + assert_eq!(SELECTOR_UNSHIELD, selectors::UNSHIELD); + } + + /// And both are genuine keccak output, not bytes that happen to agree. + #[test] + fn selectors_are_keccak_of_the_abi_signatures() { + let pt = sp_core::hashing::keccak_256( + b"privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)", + ); + let un = sp_core::hashing::keccak_256( + b"unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256,bytes32,bytes,uint32)", + ); + assert_eq!(pt[..4], SELECTOR_PRIVATE_TRANSFER); + assert_eq!(un[..4], SELECTOR_UNSHIELD); + } + + /// The runtime fallback list must agree with the client fallback list. + #[test] + fn fallback_selectors_contain_both_operations() { + assert!(SELECTORS_FALLBACK.contains(&SELECTOR_UNSHIELD)); + assert!(SELECTORS_FALLBACK.contains(&SELECTOR_PRIVATE_TRANSFER)); + } + + /// privateTransfer calldata shorter than its 9-slot head (292 bytes) is + /// rejected even though it clears the global 228-byte minimum. + #[test] + fn rejects_private_transfer_calldata_between_228_and_292() { + let mut data = build_calldata(SELECTOR_PRIVATE_TRANSFER, MIN_RELAY_FEE_FALLBACK); + data.truncate(291); + assert_eq!( + validate_relay_calldata(&data, MIN_RELAY_FEE_FALLBACK, &SELECTORS_FALLBACK), + Err("calldata too short") + ); + } + #[test] fn rejects_unknown_selector() { let mut data = build_calldata(SELECTOR_UNSHIELD, MIN_RELAY_FEE_FALLBACK); @@ -226,6 +282,25 @@ mod tests { // ── Fee checks ───────────────────────────────────────────────────────── + /// A fee word above `u128::MAX` must be rejected, not panic. + /// + /// Regression: `U256::as_u128` panics on anything wider than 128 bits, and + /// this calldata arrives from an unauthenticated RPC call — 32 crafted bytes + /// were enough to take down the handler, repeatably and for free. + #[test] + fn huge_fee_saturates_instead_of_panicking() { + let mut data = build_calldata(SELECTOR_PRIVATE_TRANSFER, 0); + // Set a byte in the HIGH 128 bits of the fee slot (data[196..228]). + data[196 + 15] = 0x01; + + // Saturates to u128::MAX, so it clears the floor here and is left for + // the dry-run to reject — the same outcome as any other absurd fee. + assert_eq!( + validate_relay_calldata(&data, MIN_RELAY_FEE_FALLBACK, &SELECTORS_FALLBACK), + Ok(()) + ); + } + #[test] fn rejects_zero_fee() { let data = build_calldata(SELECTOR_UNSHIELD, 0); @@ -513,3 +588,215 @@ mod tests { assert!(err.contains("OutOfFund"), "{err}"); } } + +// --------------------------------------------------------------------------- +// Adversarial battery — the relay is reachable UNAUTHENTICATED over RPC +// --------------------------------------------------------------------------- +// +// `validate_relay_calldata` is the first code in the node to touch bytes that +// an anonymous internet caller fully controls. A panic here is a remote node +// crash, not a rejected request. These tests try to cause one. + +#[cfg(test)] +mod adversarial { + use super::*; + use crate::relay::operations::{SELECTOR_PRIVATE_TRANSFER, SELECTOR_UNSHIELD}; + + /// Well-formed base calldata for privateTransfer, long enough to pass the + /// length gates so the later checks are actually reached. + fn base_private_transfer(fee: u128) -> Vec { + let mut d = SELECTOR_PRIVATE_TRANSFER.to_vec(); + d.resize(4 + 288, 0); + let mut fee_word = [0u8; 32]; + fee_word[16..32].copy_from_slice(&fee.to_be_bytes()); + d[4 + 192..4 + 224].copy_from_slice(&fee_word); + d + } + + /// Every length from 0 to just past the minimum: no panic, and the boundary + /// must be exact (227 rejected, 228 reaches the selector check). + #[test] + fn attack_every_calldata_length_is_handled_without_panic() { + for len in 0..400usize { + let data = vec![0xAAu8; len]; + let _ = validate_relay_calldata(&data, 0, &SELECTORS_FALLBACK); + } + // Boundary is exact. + assert_eq!( + validate_relay_calldata(&vec![0u8; 227], 0, &SELECTORS_FALLBACK), + Err("calldata too short") + ); + // 228 bytes of zeros passes the length gate and dies on the selector. + assert_eq!( + validate_relay_calldata(&vec![0u8; 228], 0, &SELECTORS_FALLBACK), + Err("unsupported selector") + ); + } + + /// A valid selector with calldata between the global 228 gate and the + /// operation's own minimum must be refused by the per-op gate, not read + /// past its end. + #[test] + fn attack_length_between_global_and_per_op_minimum_is_refused() { + for len in 228..292usize { + let mut d = SELECTOR_PRIVATE_TRANSFER.to_vec(); + d.resize(len, 0); + assert_eq!( + validate_relay_calldata(&d, 0, &SELECTORS_FALLBACK), + Err("calldata too short"), + "privateTransfer at {len} bytes must be refused" + ); + } + for len in 228..324usize { + let mut d = SELECTOR_UNSHIELD.to_vec(); + d.resize(len, 0); + assert_eq!( + validate_relay_calldata(&d, 0, &SELECTORS_FALLBACK), + Err("calldata too short"), + "unshield at {len} bytes must be refused" + ); + } + } + + /// The calldata cap must hold exactly: one byte over is refused, and the + /// oversized buffer must never be walked. + #[test] + fn attack_oversized_calldata_is_refused_at_the_exact_boundary() { + let mut ok = base_private_transfer(0); + ok.resize(MAX_CALLDATA_BYTES, 0); + // At the cap: passes the size gate (fails later or succeeds, but not "too large"). + assert_ne!( + validate_relay_calldata(&ok, 0, &SELECTORS_FALLBACK), + Err("calldata too large") + ); + + let mut over = base_private_transfer(0); + over.resize(MAX_CALLDATA_BYTES + 1, 0); + assert_eq!( + validate_relay_calldata(&over, 0, &SELECTORS_FALLBACK), + Err("calldata too large") + ); + } + + /// A fee word of all 0xFF (u256::MAX) must saturate, never panic, and must + /// COMPARE as above any minimum — a panic here is remote node death. + #[test] + fn attack_max_fee_word_saturates_and_passes_the_floor() { + let mut d = base_private_transfer(0); + d[4 + 192..4 + 224].copy_from_slice(&[0xFFu8; 32]); + assert_eq!( + validate_relay_calldata(&d, u128::MAX, &SELECTORS_FALLBACK), + Ok(()), + "a saturated fee must clear even the maximum floor" + ); + } + + /// A fee one planck below the floor must be refused; exactly at the floor + /// must pass. Off-by-one here is free money for the attacker or a broken relay. + #[test] + fn attack_fee_floor_boundary_is_exact() { + let floor = 1_000_000_000_000_000u128; + assert_eq!( + validate_relay_calldata( + &base_private_transfer(floor - 1), + floor, + &SELECTORS_FALLBACK + ), + Err("fee below minimum") + ); + assert_eq!( + validate_relay_calldata(&base_private_transfer(floor), floor, &SELECTORS_FALLBACK), + Ok(()) + ); + } + + /// An empty whitelist must reject everything — a governance misconfiguration + /// must fail closed, never open. + #[test] + fn attack_empty_whitelist_fails_closed() { + assert_eq!( + validate_relay_calldata(&base_private_transfer(0), 0, &[]), + Err("unsupported selector") + ); + } + + /// A selector the governance whitelist allows but the node does not + /// implement must be refused, not dispatched to a wrong decoder. + #[test] + fn attack_whitelisted_but_unimplemented_selector_is_refused() { + let mut d = base_private_transfer(0); + d[..4].copy_from_slice(&[0xDE, 0xAD, 0xBE, 0xEF]); + assert_eq!( + validate_relay_calldata(&d, 0, &[[0xDE, 0xAD, 0xBE, 0xEF]]), + Err("unsupported selector"), + "a selector with no registered operation must fail closed" + ); + } + + /// The gas floor must saturate rather than overflow: base_fee near u128::MAX + /// multiplied by 2×gas_limit would wrap and produce a floor of ~0, letting + /// every transfer through for free. + #[test] + fn attack_gas_floor_saturates_instead_of_wrapping() { + let floor = compute_effective_min_fee(1, u128::MAX); + assert_eq!( + floor, + u128::MAX, + "a wrapped multiplication would collapse the floor to near zero" + ); + // And a realistic value still behaves. + let normal = compute_effective_min_fee(1_000_000_000_000_000, 1_000_000_000); + assert!(normal >= 1_000_000_000_000_000); + } + + /// Deterministic byte fuzz over the whole calldata: the only requirement is + /// that no input, however malformed, panics the validator. + #[test] + fn attack_calldata_fuzz_never_panics() { + let base = base_private_transfer(1_000_000_000_000_000); + let mut seed: u64 = 0xDEADBEEFCAFEBABE; + for _ in 0..20_000 { + let mut data = base.clone(); + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let muts = 1 + (seed >> 60) as usize % 12; + for _ in 0..muts { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let pos = (seed >> 33) as usize % data.len(); + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + data[pos] = (seed >> 40) as u8; + } + // Sometimes truncate too. + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + if seed % 3 == 0 { + let cut = (seed >> 33) as usize % data.len().max(1); + data.truncate(cut); + } + let _ = validate_relay_calldata(&data, 1_000_000_000_000_000, &SELECTORS_FALLBACK); + } + } + + /// Fuzz the fee slot specifically with full-width random words — this is the + /// field that historically panicked via `U256::as_u128()`. + #[test] + fn attack_fee_slot_fuzz_never_panics() { + let mut seed: u64 = 0x1234_5678_9ABC_DEF0; + for _ in 0..20_000 { + let mut d = base_private_transfer(0); + for byte in d[4 + 192..4 + 224].iter_mut() { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + *byte = (seed >> 40) as u8; + } + let _ = validate_relay_calldata(&d, 1_000_000_000_000_000, &SELECTORS_FALLBACK); + } + } +} diff --git a/frame/evm/precompile/shielded-pool/CHANGELOG.md b/frame/evm/precompile/shielded-pool/CHANGELOG.md index 055c8f4e..23b2898b 100644 --- a/frame/evm/precompile/shielded-pool/CHANGELOG.md +++ b/frame/evm/precompile/shielded-pool/CHANGELOG.md @@ -2,6 +2,36 @@ All notable changes to `pallet-evm-precompile-shielded-pool` will be documented in this file. +## [0.6.0] - 2026-08-10 + +### Changed + +- **`privateTransfer` takes a trailing `bytes` — the 56-byte OVK blob — and its + selector changes with it.** A memo is sealed toward the recipient, so its + sender cannot reopen it; the blob wraps that memo's shared secret under the + sender's outgoing viewing key so a sender who loses their vault can still + recover what they sent. + + ``` + privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32) 0x66ed2cd4 + privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes) 0x1ec439cf + ``` + + The ABI head grows from 8 slots (256 bytes) to 9 (288). The decoder requires + the blob to be **exactly 56 bytes** — the SCALE route gets that from the type, + but calldata carries a dynamic `bytes`, so the EVM route has to pin it here. + + **Breaking, in both directions.** A caller on the old selector is rejected as + unsupported; a caller on the new one against an old runtime is too. Wallet and + runtime must ship together. `transaction_version` is bumped 2 → 3. + +- **The selectors are now exported** as `selectors::{SHIELD, PRIVATE_TRANSFER, + UNSHIELD, CLAIM_SHIELDED_FEES}`, so the relay whitelist can be pinned against + the decoder's own constants in a test instead of keeping a hand-copied list in + sync. That copy drifting is ME-8, and it fails silently: a wrong selector is + merely "unsupported", so rejection tests stay green while relaying stops + working. + ## [0.5.0] - 2026-08-07 ### Security diff --git a/frame/evm/precompile/shielded-pool/Cargo.toml b/frame/evm/precompile/shielded-pool/Cargo.toml index c589b199..10c141b3 100644 --- a/frame/evm/precompile/shielded-pool/Cargo.toml +++ b/frame/evm/precompile/shielded-pool/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pallet-evm-precompile-shielded-pool" -version = "0.5.0" +version = "0.6.0" authors = { workspace = true } edition = "2021" description = "EVM Precompile for Orbinum Shielded Pool Pallet." diff --git a/frame/evm/precompile/shielded-pool/src/calls/claim_shielded_fees.rs b/frame/evm/precompile/shielded-pool/src/calls/claim_shielded_fees.rs index 52e6882e..59e7e344 100644 --- a/frame/evm/precompile/shielded-pool/src/calls/claim_shielded_fees.rs +++ b/frame/evm/precompile/shielded-pool/src/calls/claim_shielded_fees.rs @@ -5,24 +5,44 @@ //! `keccak256("claimShieldedFees(bytes32,uint256,uint32,bytes,bytes,bytes,uint32)")[0..4]` //! = `0x88d9deba` //! -//! ## ABI layout (`input[4..]`) -//! | Slot (bytes) | Type | Field | -//! |-------------|-----------|--------------------| -//! | 0..32 | `bytes32` | `commitment` | -//! | 32..64 | `uint256` | `amount` | -//! | 64..96 | `uint32` | `asset_id` | -//! | 96..128 | `uint256` | offset → `memo` | -//! | 128..160 | `uint256` | offset → `proof` | -//! | 160..192 | `uint256` | offset → `public_signals` | -//! | 192..224 | `uint32` | `circuit_version` | +//! ## ABI layout (`input[4..]`) — standard head/tail encoding //! -//! The **validator** origin is derived from `handle.context().caller` -//! (the EVM address that sent the transaction), mapped to an `AccountId` -//! via `AddressMapping`. It must match the address registered in -//! `pallet-relayer` that has accumulated pending fees. +//! Seven 32-byte head slots. Dynamic types (`bytes`) store an OFFSET here and +//! their real data in the tail; fixed types are inline. The `#` column matches +//! the numbered steps in [`decode`] below, so the layout and the code that +//! reads it stay in the same order. //! -//! ## public_signals layout (76 bytes, off-chain encoded) -//! `commitment[0..32] | value[32..40] | asset_id[40..44] | owner_hash[44..76]` +//! | # | Slot (bytes) | Type | Field | +//! |---|--------------|-----------|---------------------------| +//! | 1 | 0..32 | `bytes32` | `commitment` | +//! | 2 | 32..64 | `uint256` | `amount` | +//! | 3 | 64..96 | `uint32` | `asset_id` | +//! | 4 | 96..128 | `uint256` | offset → `memo` | +//! | 5 | 128..160 | `uint256` | offset → `proof` | +//! | 6 | 160..192 | `uint256` | offset → `public_signals` | +//! | 7 | 192..224 | `uint32` | `circuit_version` | +//! +//! ## Field notes +//! +//! - **The claiming validator is not in the ABI.** It comes from +//! `handle.context().caller` — the EVM address that sent the transaction — +//! mapped to an `AccountId` via `AddressMapping`, and it must match the +//! address `pallet-relayer` has accumulated pending fees for. Taking it from +//! calldata would let anyone claim another validator's fees. +//! - **`public_signals`** is a 76-byte off-chain blob, laid out as +//! `commitment[0..32] | value[32..40] | asset_id[40..44] | owner_hash[44..76]`. +//! It is what BINDS the head parameters to the proof: the pallet checks that +//! slots 1–3 match the values embedded here, so a caller cannot present a +//! valid proof and then claim a different amount or asset with it. Hence the +//! exact-length check in step 6 — a short blob would leave those comparisons +//! reading past the end or against garbage. +//! +//! ## Why the length gate is not a single check +//! +//! The head is 224 bytes, but the gate at step 0 only requires 192. That is +//! deliberate: `circuit_version` (slot 7) was appended by a later upgrade, so +//! it is checked where it is read (step 7) and the error names the missing +//! field instead of reporting a generic "too short". use alloc::vec::Vec; @@ -39,12 +59,27 @@ pub const SELECTOR: [u8; 4] = [0x88, 0xd9, 0xde, 0xba]; /// Maximum byte length of a serialised Groth16 proof accepted by the pallet. const MAX_PROOF_LEN: u32 = 512; +/// Exact length of the `public_signals` blob — see the module header for its +/// layout. Not a maximum: the pallet indexes fixed offsets inside it. +const PUBLIC_SIGNALS_LEN: usize = 76; + +/// Minimum `params` length: the six head slots this call has always had. +/// Slot 7 came later and is checked where it is read. +const HEAD_SIZE_BASE: usize = 192; +/// The full seven-slot head, through `circuit_version`. +const HEAD_SIZE_FULL: usize = 224; + /// Decodes the ABI-encoded `input` and returns a ready-to-dispatch /// `claim_shielded_fees` call. /// -/// The validator `AccountId` is NOT part of the ABI — it is derived from -/// `handle.context().caller` so the pallet can look up the correct pending -/// fee balance in `pallet-relayer`. +/// The steps below are numbered to match the ABI table in the module header. +/// The validator `AccountId` is NOT part of the ABI — the pallet derives it +/// from `handle.context().caller` to look up the right pending-fee balance in +/// `pallet-relayer`, which is why `_handle` goes unused here. +/// +/// Everything here decodes UNTRUSTED calldata — an EVM caller controls every +/// byte — so each helper is bounds-checked and every word is rejected rather +/// than truncated when it does not fit its declared type. pub fn decode( _handle: &impl PrecompileHandle, input: &[u8], @@ -53,15 +88,26 @@ where T: pallet_shielded_pool::Config, pallet_shielded_pool::BalanceOf: TryFrom, { - // Minimum head section: 6 fixed slots × 32 bytes = 192 bytes. let params = &input[4..]; - if params.len() < 192 { + + // ── 0. Head gate ───────────────────────────────────────────────────────── + // Covers slots 1–6, the reads done unconditionally below. Slot 7 is gated + // where it is read. + if params.len() < HEAD_SIZE_BASE { return Err(err("claimShieldedFees: input too short")); } + // ── 1. commitment (slot 0, inline) ─────────────────────────────────────── + // The fee note's commitment. Cross-checked against `public_signals[0..32]` + // by the pallet — see step 6. let commitment = pallet_shielded_pool::Commitment::from(abi::read_bytes32(params, 0)?); - // Reject zero-amount calls early. + // ── 2. amount (slot 1, inline) ─────────────────────────────────────────── + // Zero is refused before anything else is built: claiming nothing would + // still insert a leaf and move the Merkle root. Then two fallible narrowing + // steps — the ABI word is a `uint256` while the pallet's balance is at most + // `u128`. `try_into` rather than `as_u128()`, which panics above 2^128 on a + // word the caller fully controls. let amount_u256 = U256::from_big_endian(¶ms[32..64]); if amount_u256.is_zero() { return Err(err("claimShieldedFees: amount must be non-zero")); @@ -74,12 +120,20 @@ where .map_err(|_| err("claimShieldedFees: amount conversion failed"))? }; + // ── 3. asset_id (slot 2, inline) ───────────────────────────────────────── let asset_id = abi::decode_u32(¶ms[64..96])?; + // ── 4. memo (slot 3 → tail) ────────────────────────────────────────────── + // `FrameEncryptedMemo::new` pins the pallet's exact size; the chain never + // reads inside it. let memo_bytes: Vec = abi::decode_bytes_at_slot(params, 96)?; let memo = pallet_shielded_pool::FrameEncryptedMemo::new(memo_bytes) .map_err(|_| err("claimShieldedFees: memo too long or wrong size"))?; + // ── 5. proof (slot 4 → tail) ───────────────────────────────────────────── + // Bounded on the way in: `MAX_PROOF_LEN` is the pallet's ceiling, so an + // oversized proof is refused here rather than at dispatch. Empty is refused + // separately — the ZK verifier would reject it anyway, but far later. let proof: frame_support::BoundedVec> = abi::decode_bytes_at_slot(params, 128)? .try_into() @@ -89,16 +143,24 @@ where return Err(err("claimShieldedFees: proof must be non-empty")); } + // ── 6. public_signals (slot 5 → tail) ──────────────────────────────────── + // EXACTLY 76 bytes, not "at most": the pallet reads fixed offsets inside + // this blob to check that steps 1–3 match what the proof actually attests + // to. A short blob would leave those comparisons reading past the end, and + // a long one would hide trailing bytes nothing verifies. let public_signals_raw: Vec = abi::decode_bytes_at_slot(params, 160)?; - if public_signals_raw.len() != 76 { + if public_signals_raw.len() != PUBLIC_SIGNALS_LEN { return Err(err("claimShieldedFees: public_signals must be 76 bytes")); } let public_signals = public_signals_raw .try_into() .map_err(|_| err("claimShieldedFees: public_signals too long"))?; - if params.len() < 224 { + // ── 7. circuit_version (slot 6, inline) ────────────────────────────────── + // Appended by a later upgrade, so it gets its own gate with an error that + // names it rather than a generic "too short". + if params.len() < HEAD_SIZE_FULL { return Err(err( "claimShieldedFees: input too short (missing circuitVersion)", )); diff --git a/frame/evm/precompile/shielded-pool/src/calls/private_transfer.rs b/frame/evm/precompile/shielded-pool/src/calls/private_transfer.rs index 5a1862fd..ba531dc7 100644 --- a/frame/evm/precompile/shielded-pool/src/calls/private_transfer.rs +++ b/frame/evm/precompile/shielded-pool/src/calls/private_transfer.rs @@ -1,23 +1,35 @@ //! ABI decoding and call construction for -//! `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32)`. +//! `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)`. //! //! ## Selector -//! `keccak256("privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32)")[0..4]` -//! = `0x66ed2cd4` +//! `keccak256("privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)")[0..4]` +//! = `0x1ec439cf` //! //! ## ABI layout (`input[4..]`) — standard head/tail encoding -//! | Slot (bytes) | Type | Field | -//! |-------------|-------------|--------------------| -//! | 0..32 | `uint256` | offset → `proof` | -//! | 32..64 | `bytes32` | `merkle_root` | -//! | 64..96 | `uint256` | offset → nullifiers| -//! | 96..128 | `uint256` | offset → commitments| -//! | 128..160 | `uint256` | offset → memos | -//! | 160..192 | `uint32` | `asset_id` | -//! | 192..224 | `uint256` | `fee` | -//! | 224..256 | `uint32` | `circuit_version` | //! -//! `relayer` is derived from `handle.context().caller` — not part of the ABI. +//! Nine 32-byte head slots. Dynamic types (`bytes`, arrays) store an OFFSET +//! here and their real data in the tail; fixed types are inline. The `#` column +//! matches the numbered steps in [`decode`] below, so the layout and the code +//! that reads it stay in the same order. +//! +//! | # | Slot (bytes) | Type | Field | +//! |---|--------------|-----------|----------------------| +//! | 1 | 0..32 | `uint256` | offset → `proof` | +//! | 2 | 32..64 | `bytes32` | `merkle_root` | +//! | 3 | 64..96 | `uint256` | offset → nullifiers | +//! | 4 | 96..128 | `uint256` | offset → commitments | +//! | 5 | 128..160 | `uint256` | offset → memos | +//! | 7 | 160..192 | `uint32` | `asset_id` | +//! | 8 | 192..224 | `uint256` | `fee` | +//! | 9 | 224..256 | `uint32` | `circuit_version` | +//! | 10| 256..288 | `uint256` | offset → `ovk_blob` | +//! +//! Step 6 has no slot of its own: it cross-checks the three arrays decoded in +//! steps 3–5 against each other. +//! +//! `relayer` is not in the ABI at all — it is taken from +//! `handle.context().caller`, so a caller cannot name someone else as the fee +//! recipient (step 11). use fp_evm::{ExitError, PrecompileFailure, PrecompileHandle}; use frame_support::BoundedVec; @@ -25,21 +37,34 @@ use sp_core::U256; use crate::abi; -/// `keccak256("privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32)")[0..4]` -/// The trailing `uint32` is `circuitVersion` — the circuit version the spent -/// notes were created under, so the proof is verified against that version's VK. -pub const SELECTOR: [u8; 4] = [0x66, 0xed, 0x2c, 0xd4]; +/// `keccak256("privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)")[0..4]` +/// +/// Two arguments are easy to misread from the signature alone: the `uint32` +/// before the final `bytes` is `circuitVersion` (the version the spent notes +/// were created under, so the proof is verified against that version's VK), and +/// the trailing `bytes` is the 56-byte OVK blob. +pub const SELECTOR: [u8; 4] = [0x1e, 0xc4, 0x39, 0xcf]; /// Maximum byte length of a serialised Groth16 proof accepted by the pallet. const MAX_PROOF_LEN: u32 = 512; /// Maximum number of input nullifiers / output commitments in a single transfer. const MAX_NOTES: u32 = 2; +/// Minimum `params` length: nine 32-byte head slots. Every fixed-offset read +/// below is covered by this one gate, so it must stay in step with the table. +const HEAD_SIZE: usize = 288; /// Decodes the ABI-encoded `input` and returns a ready-to-dispatch /// `private_transfer` call. /// -/// `handle.context().caller` is forwarded as the `relayer` field so -/// `pallet-relayer` can route fees to the registered Substrate account. +/// The steps below are numbered to match the ABI table in the module header. +/// Order is not arbitrary: the cheap length gate runs first (step 0) so a +/// malformed call is rejected before any allocation, and the structural checks +/// (step 6) run before the fee and blob work, so a nonsensical call never +/// reaches the pallet. +/// +/// Everything here decodes UNTRUSTED calldata — an EVM caller controls every +/// byte — so each helper is bounds-checked and every word is rejected rather +/// than truncated when it does not fit its declared type. pub fn decode( handle: &impl PrecompileHandle, input: &[u8], @@ -49,10 +74,19 @@ where pallet_shielded_pool::BalanceOf: TryFrom, { let params = &input[4..]; - if params.len() < 256 { + + // ── 0. Head gate ───────────────────────────────────────────────────────── + // One length check covering every fixed-offset read below, done before any + // decoding so malformed calldata costs nothing. + if params.len() < HEAD_SIZE { return Err(err("privateTransfer: input too short")); } + // ── 1. proof (slot 0 → tail) ───────────────────────────────────────────── + // Bounded on the way in: `MAX_PROOF_LEN` is the pallet's ceiling, so an + // oversized proof is refused here rather than at dispatch. Empty is refused + // separately — the ZK verifier would reject it anyway, but far later and + // with a worse message. let proof: BoundedVec> = abi::decode_bytes_at_slot(params, 0)? .try_into() @@ -62,8 +96,10 @@ where return Err(err("privateTransfer: proof must be non-empty")); } + // ── 2. merkle_root (slot 1, inline) ────────────────────────────────────── let merkle_root: pallet_shielded_pool::Hash = abi::read_bytes32(params, 32)?; + // ── 3. nullifiers (slot 2 → tail) ──────────────────────────────────────── let nullifiers: BoundedVec< pallet_shielded_pool::Nullifier, frame_support::traits::ConstU32, @@ -74,6 +110,7 @@ where .try_into() .map_err(|_| err("privateTransfer: too many nullifiers"))?; + // ── 4. commitments (slot 3 → tail) ─────────────────────────────────────── let commitments: BoundedVec< pallet_shielded_pool::Commitment, frame_support::traits::ConstU32, @@ -84,6 +121,7 @@ where .try_into() .map_err(|_| err("privateTransfer: too many commitments"))?; + // ── 5. encrypted_memos (slot 4 → tail) ─────────────────────────────────── let encrypted_memos: BoundedVec< pallet_shielded_pool::FrameEncryptedMemo, frame_support::traits::ConstU32, @@ -97,10 +135,11 @@ where .try_into() .map_err(|_| err("privateTransfer: too many memos"))?; - // Structural consistency: at least one real input note is required, and the three - // parallel arrays must have the same length. The ZK proof enforces value balance, - // but mismatched array lengths would produce a nonsensical call that reaches the - // pallet unnecessarily. + // ── 6. Structural consistency across steps 3–5 ─────────────────────────── + // The three arrays run in parallel — input i, output i, memo i — so a length + // mismatch is a call that cannot mean anything. The ZK proof enforces value + // balance, not array arity, and the pallet re-checks this; catching it here + // keeps a nonsensical call from consuming dispatch weight at all. if nullifiers.is_empty() { return Err(err("privateTransfer: at least one nullifier required")); } @@ -111,8 +150,13 @@ where return Err(err("privateTransfer: commitment/memo count mismatch")); } + // ── 7. asset_id (slot 5, inline) ───────────────────────────────────────── let asset_id = abi::decode_u32(¶ms[160..192])?; + // ── 8. fee (slot 6, inline) ────────────────────────────────────────────── + // Two narrowing steps, both fallible: the ABI word is a `uint256` while the + // pallet's balance is at most `u128`. `try_into` rather than `as_u128()`, + // which panics above 2^128 — and this word is fully caller-controlled. let fee: pallet_shielded_pool::BalanceOf = { let raw: u128 = U256::from_big_endian(¶ms[192..224]) .try_into() @@ -121,10 +165,26 @@ where .map_err(|_| err("privateTransfer: fee conversion failed"))? }; - let relayer = Some(handle.context().caller); - + // ── 9. circuit_version (slot 7, inline) ────────────────────────────────── let circuit_version = abi::decode_u32(¶ms[224..256])?; + // ── 10. ovk_blob (slot 8 → tail) ───────────────────────────────────────── + // Calldata carries a dynamic `bytes`, so this is where the 56-byte length is + // pinned on the EVM route — the SCALE route gets it from the type itself. + // Length is ALL that is checked: the blob is ciphertext and no key exists on + // chain, so any 56 bytes are valid, zeros included. + let ovk_blob = { + let raw = abi::decode_bytes_at_slot(params, 256)?; + pallet_shielded_pool::OvkBlob::from_bytes(&raw) + .map_err(|_| err("privateTransfer: ovk blob must be exactly 56 bytes"))? + }; + + // ── 11. relayer — from the CALLER, never from calldata ──────────────────── + // Taking this from the ABI would let anyone name a third party as the fee + // recipient. `pallet-relayer` resolves the caller's registered Substrate + // account from it. + let relayer = Some(handle.context().caller); + Ok(pallet_shielded_pool::Call::::private_transfer { proof, merkle_root, @@ -135,6 +195,7 @@ where fee, relayer, circuit_version, + ovk_blob, }) } diff --git a/frame/evm/precompile/shielded-pool/src/calls/shield.rs b/frame/evm/precompile/shielded-pool/src/calls/shield.rs index 9ec0ef80..d5b0ebaf 100644 --- a/frame/evm/precompile/shielded-pool/src/calls/shield.rs +++ b/frame/evm/precompile/shielded-pool/src/calls/shield.rs @@ -3,16 +3,24 @@ //! ## Selector //! `keccak256("shield(uint32,bytes32,bytes)")[0..4]` = `0x9feb22ea` //! -//! ## ABI layout (`input[4..]`) -//! | Slot (bytes) | Type | Field | -//! |-------------|-----------|-----------------| -//! | 0..32 | `uint32` | `asset_id` | -//! | 32..64 | `bytes32` | `commitment` | -//! | 64..96 | `uint256` | offset → memo | -//! | at offset | `bytes` | `encrypted_memo`| +//! ## ABI layout (`input[4..]`) — standard head/tail encoding //! -//! The token **amount** is read from `msg.value` — the EVM executor transfers it to -//! the precompile's address before `execute` runs, so no explicit amount slot is needed. +//! Three 32-byte head slots. The dynamic `bytes` stores an OFFSET here and its +//! real data in the tail; fixed types are inline. The `#` column matches the +//! numbered steps in [`decode`] below, so the layout and the code that reads it +//! stay in the same order. +//! +//! | # | Slot (bytes) | Type | Field | +//! |---|--------------|-----------|------------------| +//! | 1 | 0..32 | `uint32` | `asset_id` | +//! | 3 | 32..64 | `bytes32` | `commitment` | +//! | 4 | 64..96 | `uint256` | offset → memo | +//! | | at offset | `bytes` | `encrypted_memo` | +//! +//! Step 2 has no slot: the token **amount** is not in the ABI at all. It comes +//! from `msg.value` — the EVM executor transfers it to the precompile's address +//! before `execute` runs — which is also why `shield` is the one call here that +//! is payable. use fp_evm::{ExitError, PrecompileFailure, PrecompileHandle}; @@ -21,9 +29,19 @@ use crate::abi; /// `keccak256("shield(uint32,bytes32,bytes)")[0..4]` pub const SELECTOR: [u8; 4] = [0x9f, 0xeb, 0x22, 0xea]; +/// Minimum `params` length: three 32-byte head slots. Every fixed-offset read +/// below is covered by this one gate, so it must stay in step with the table. +const HEAD_SIZE: usize = 96; + /// Decodes the ABI-encoded `input` and returns a ready-to-dispatch `shield` call. /// -/// `handle` is consulted only for `apparent_value` (the `msg.value` ETH amount). +/// The steps below are numbered to match the ABI table in the module header. +/// `handle` is consulted only for `apparent_value` (the `msg.value` amount), +/// which is step 2 and has no ABI slot. +/// +/// Everything here decodes UNTRUSTED calldata — an EVM caller controls every +/// byte — so each helper is bounds-checked and every word is rejected rather +/// than truncated when it does not fit its declared type. pub fn decode( handle: &impl PrecompileHandle, input: &[u8], @@ -33,20 +51,30 @@ where pallet_shielded_pool::BalanceOf: TryFrom, { let params = &input[4..]; - if params.len() < 96 { + + // ── 0. Head gate ───────────────────────────────────────────────────────── + // One length check covering every fixed-offset read below, done before any + // decoding so malformed calldata costs nothing. + if params.len() < HEAD_SIZE { return Err(err("shield: input too short")); } + // ── 1. asset_id (slot 0, inline) ───────────────────────────────────────── let asset_id = abi::decode_u32(¶ms[0..32])?; - // Reject zero-value calls at the precompile boundary (defense-in-depth; - // the pallet also rejects them, but this produces a cleaner error before - // reaching the dispatch layer). + // ── 2. amount — from msg.value, NOT from calldata ──────────────────────── + // The executor has already moved this to the precompile's address, so it is + // the one argument a caller cannot lie about. Zero is refused here as + // defense in depth: the pallet rejects it too, but this fails before + // dispatch and names the problem. let apparent_value = handle.context().apparent_value; if apparent_value.is_zero() { return Err(err("shield: amount must be non-zero")); } + // Two narrowing steps, both fallible: `msg.value` is a `U256` while the + // pallet's balance is at most `u128`. `try_into` rather than `as_u128()`, + // which panics above 2^128. let amount: pallet_shielded_pool::BalanceOf = { let raw: u128 = apparent_value .try_into() @@ -55,8 +83,16 @@ where .map_err(|_| err("shield: amount conversion failed"))? }; + // ── 3. commitment (slot 1, inline) ─────────────────────────────────────── + // Canonicity is NOT checked here — the pallet does it. `shield` is the least + // guarded way into the tree (the depositor picks these bytes with no proof + // constraining them), so that check belongs at the type boundary every route + // crosses, not in one decoder. let commitment = pallet_shielded_pool::Commitment::from(abi::read_bytes32(params, 32)?); + // ── 4. encrypted_memo (slot 2 → tail) ──────────────────────────────────── + // `FrameEncryptedMemo::new` pins the exact 180-byte size; the chain never + // reads inside it. let memo_bytes = abi::decode_bytes_at_slot(params, 64)?; let encrypted_memo = pallet_shielded_pool::FrameEncryptedMemo::new(memo_bytes) .map_err(|_| err("shield: memo too long or wrong size"))?; diff --git a/frame/evm/precompile/shielded-pool/src/calls/unshield.rs b/frame/evm/precompile/shielded-pool/src/calls/unshield.rs index ddd0fc46..956e09ab 100644 --- a/frame/evm/precompile/shielded-pool/src/calls/unshield.rs +++ b/frame/evm/precompile/shielded-pool/src/calls/unshield.rs @@ -5,31 +5,46 @@ //! `keccak256("unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256,bytes32,bytes,uint32)")[0..4]` //! = `0x4e505348` //! -//! ## ABI layout (`input[4..]`) -//! | Slot (bytes) | Type | Field | -//! |-------------|-----------|-----------------| -//! | 0..32 | `uint256` | offset → `proof`| -//! | 32..64 | `bytes32` | `merkle_root` | -//! | 64..96 | `bytes32` | `nullifier` | -//! | 96..128 | `uint32` | `asset_id` | -//! | 128..160 | `uint256` | `amount` | -//! | 160..192 | `bytes32` | `recipient` (AccountId32) | -//! | 192..224 | `uint256` | `fee` | -//! | 224..256 | `bytes32` | `change_commitment` | -//! | 256..288 | `uint256` | offset → `change_encrypted_memo` | -//! | 288..320 | `uint32` | `circuit_version` | +//! ## ABI layout (`input[4..]`) — standard head/tail encoding //! -//! `recipient` is an `AccountId32` encoded as a 32-byte ABI `bytes32` slot. -//! This can be a Substrate-native account or the `AccountId32` derived from -//! an H160 address (`H160 ++ [0x00; 12]`). +//! Ten 32-byte head slots. Dynamic types (`bytes`) store an OFFSET here and +//! their real data in the tail; fixed types are inline. The `#` column matches +//! the numbered steps in [`decode`] below, so the layout and the code that +//! reads it stay in the same order. //! -//! `change_commitment` is `[0u8; 32]` for a total unshield (no change note). -//! For a partial unshield it is `NoteCommitment(change_value, asset_id, change_owner_pk, change_blinding)`. +//! | # | Slot (bytes) | Type | Field | +//! |----|--------------|-----------|----------------------------------| +//! | 1 | 0..32 | `uint256` | offset → `proof` | +//! | 2 | 32..64 | `bytes32` | `merkle_root` | +//! | 3 | 64..96 | `bytes32` | `nullifier` | +//! | 4 | 96..128 | `uint32` | `asset_id` | +//! | 5 | 128..160 | `uint256` | `amount` | +//! | 6 | 160..192 | `bytes32` | `recipient` (AccountId32) | +//! | 7 | 192..224 | `uint256` | `fee` | +//! | 8 | 224..256 | `bytes32` | `change_commitment` | +//! | 9 | 256..288 | `uint256` | offset → `change_encrypted_memo` | +//! | 10 | 288..320 | `uint32` | `circuit_version` | //! -//! `change_encrypted_memo` is a dynamic `bytes` field (176 bytes for partial unshield, 0 bytes for total). -//! For a partial unshield it contains: nonce(12) || ciphertext(132) || ephPk(32). +//! ## Field notes //! -//! `relayer` is derived from `handle.context().caller` — not part of the ABI. +//! - **`recipient`** is an `AccountId32` in a `bytes32` slot: either a +//! Substrate-native account or the one derived from an H160 +//! (`H160 ++ [0x00; 12]`). +//! - **`change_commitment`** is `[0u8; 32]` for a TOTAL unshield (nothing left +//! over, so no change note). For a PARTIAL one it is +//! `NoteCommitment(change_value, asset_id, change_owner_pk, change_blinding)`. +//! This single value is what decides how the memo in step 9 is treated. +//! - **`change_encrypted_memo`** is a dynamic `bytes`: the full 180-byte memo +//! for a partial unshield, empty for a total one. +//! - **`relayer`** is not in the ABI at all — it comes from +//! `handle.context().caller` (step 11). +//! +//! ## Why the length gate is not a single check +//! +//! The head is 320 bytes, but the gate at step 0 only requires 256. That is +//! deliberate: slots 9 and 10 were appended by later upgrades, and each is +//! checked at the point it is read (steps 9 and 10) so the error names the +//! field that is missing rather than reporting a generic "too short". use alloc::vec::Vec; @@ -47,10 +62,26 @@ pub const SELECTOR: [u8; 4] = [0x4e, 0x50, 0x53, 0x48]; /// Maximum byte length of a serialised Groth16 proof accepted by the pallet. const MAX_PROOF_LEN: u32 = 512; +/// Minimum `params` length: the eight head slots this call has always had. +/// Slots 9 and 10 came later and are checked where they are read — see the +/// module header. +const HEAD_SIZE_BASE: usize = 256; +/// Through slot 9 (`change_encrypted_memo`'s offset word). +const HEAD_SIZE_WITH_MEMO: usize = 288; +/// The full ten-slot head, through `circuit_version`. +const HEAD_SIZE_FULL: usize = 320; + /// Decodes the ABI-encoded `input` and returns a ready-to-dispatch `unshield` call. /// -/// `handle.context().caller` is forwarded as the `relayer` field so -/// `pallet-relayer` can route fees to the registered Substrate account. +/// The steps below are numbered to match the ABI table in the module header. +/// Order is not arbitrary: the cheap length gate runs first (step 0) so a +/// malformed call is rejected before any allocation, and the two +/// value-destroying inputs — a zero amount and the zero recipient — are refused +/// before anything else is built. +/// +/// Everything here decodes UNTRUSTED calldata — an EVM caller controls every +/// byte — so each helper is bounds-checked and every word is rejected rather +/// than truncated when it does not fit its declared type. pub fn decode( handle: &impl PrecompileHandle, input: &[u8], @@ -61,10 +92,18 @@ where ::AccountId: From<[u8; 32]>, { let params = &input[4..]; - if params.len() < 256 { + + // ── 0. Head gate ───────────────────────────────────────────────────────── + // Covers slots 1–8, the reads done unconditionally below. Slots 9 and 10 are + // gated where they are read. + if params.len() < HEAD_SIZE_BASE { return Err(err("unshield: input too short")); } + // ── 1. proof (slot 0 → tail) ───────────────────────────────────────────── + // Bounded on the way in: `MAX_PROOF_LEN` is the pallet's ceiling, so an + // oversized proof is refused here rather than at dispatch. Empty is refused + // separately — the ZK verifier would reject it anyway, but far later. let proof: BoundedVec> = abi::decode_bytes_at_slot(params, 0)? .try_into() @@ -74,13 +113,21 @@ where return Err(err("unshield: proof must be non-empty")); } + // ── 2. merkle_root (slot 1, inline) ────────────────────────────────────── let merkle_root: pallet_shielded_pool::Hash = abi::read_bytes32(params, 32)?; + // ── 3. nullifier (slot 2, inline) ──────────────────────────────────────── let nullifier = pallet_shielded_pool::Nullifier::from(abi::read_bytes32(params, 64)?); + // ── 4. asset_id (slot 3, inline) ───────────────────────────────────────── let asset_id = abi::decode_u32(¶ms[96..128])?; - // Reject zero-amount unshield early (defense-in-depth before dispatch). + // ── 5. amount (slot 4, inline) ─────────────────────────────────────────── + // Zero is refused before anything else is built: the pallet rejects it too, + // but failing here names the field and costs no dispatch weight. Then two + // fallible narrowing steps — the ABI word is a `uint256` while the pallet's + // balance is at most `u128`. `try_into` rather than `as_u128()`, which + // panics above 2^128 on a word the caller fully controls. let amount_u256 = U256::from_big_endian(¶ms[128..160]); if amount_u256.is_zero() { return Err(err("unshield: amount must be non-zero")); @@ -93,14 +140,18 @@ where .map_err(|_| err("unshield: amount conversion failed"))? }; - // Reject the zero AccountId32 (all-zeros): transferring to this address - // permanently destroys tokens with no possibility of recovery. + // ── 6. recipient (slot 5, inline) ──────────────────────────────────────── + // The all-zero AccountId32 is refused: it is a valid-looking address that + // nobody holds the key to, so unshielding to it destroys the tokens with no + // route to recovery. Unlike a wrong-but-real address, this one is always a + // mistake. let recipient_bytes = abi::read_bytes32(params, 160)?; if recipient_bytes == [0u8; 32] { return Err(err("unshield: recipient must not be the zero address")); } let recipient: ::AccountId = recipient_bytes.into(); + // ── 7. fee (slot 6, inline) ────────────────────────────────────────────── let fee: pallet_shielded_pool::BalanceOf = { let raw: u128 = U256::from_big_endian(¶ms[192..224]) .try_into() @@ -109,14 +160,18 @@ where .map_err(|_| err("unshield: fee conversion failed"))? }; + // ── 8. change_commitment (slot 7, inline) ──────────────────────────────── + // Zero here means TOTAL unshield — the whole note leaves the pool and there + // is no change note. This flag decides how step 9 treats a missing memo. let change_commitment: pallet_shielded_pool::Hash = abi::read_bytes32(params, 224)?; let is_total_unshield = change_commitment == [0u8; 32]; - // Decode change_encrypted_memo as a dynamic bytes field (offset pointer at - // slot 256). For a partial unshield (non-zero change_commitment) a malformed - // offset must fail loudly — silently defaulting to an empty memo would make - // the change note unrecoverable. A total unshield legitimately has no memo. - let change_encrypted_memo_bytes = if params.len() >= 288 { + // ── 9. change_encrypted_memo (slot 8 → tail) ───────────────────────────── + // Absence is only tolerated for a TOTAL unshield, which legitimately has no + // change note. On a PARTIAL one a malformed offset must fail loudly: + // defaulting to an empty memo would commit a change note whose owner can + // never open it — funds locked in the tree forever. + let change_encrypted_memo_bytes = if params.len() >= HEAD_SIZE_WITH_MEMO { match abi::decode_bytes_at_slot(params, 256) { Ok(bytes) => bytes, Err(e) if is_total_unshield => { @@ -129,25 +184,30 @@ where Vec::new() }; - // Convert to EncryptedMemo (max 176 bytes per pallet definition). - // Empty bytes (total unshield) is allowed and results in an empty EncryptedMemo. + // `EncryptedMemo::new` pins the pallet's exact size (180 bytes); the empty + // default is the total-unshield case, and the chain never reads inside it. let change_encrypted_memo: pallet_shielded_pool::types::EncryptedMemo = if change_encrypted_memo_bytes.is_empty() { - // Total unshield: empty memo pallet_shielded_pool::types::EncryptedMemo::default() } else { - // Partial unshield: create from bytes pallet_shielded_pool::types::EncryptedMemo::new(change_encrypted_memo_bytes) .map_err(|_| err("unshield: invalid change_encrypted_memo"))? }; - let relayer = Some(handle.context().caller); - - if params.len() < 320 { + // ── 10. circuit_version (slot 9, inline) ───────────────────────────────── + // Appended by a later upgrade, so it gets its own gate with an error that + // names it rather than a generic "too short". + if params.len() < HEAD_SIZE_FULL { return Err(err("unshield: input too short (missing circuitVersion)")); } let circuit_version = abi::decode_u32(¶ms[288..320])?; + // ── 11. relayer — from the CALLER, never from calldata ─────────────────── + // Taking this from the ABI would let anyone name a third party as the fee + // recipient. `pallet-relayer` resolves the caller's registered Substrate + // account from it. + let relayer = Some(handle.context().caller); + Ok(pallet_shielded_pool::Call::::unshield { proof, merkle_root, diff --git a/frame/evm/precompile/shielded-pool/src/lib.rs b/frame/evm/precompile/shielded-pool/src/lib.rs index 570872b3..c2f3135b 100644 --- a/frame/evm/precompile/shielded-pool/src/lib.rs +++ b/frame/evm/precompile/shielded-pool/src/lib.rs @@ -6,6 +6,20 @@ pub(crate) mod abi; pub(crate) mod calls; pub(crate) mod dispatch; +/// The ABI selectors this precompile answers to. +/// +/// Exported so the relay whitelist can be pinned against them in a test rather +/// than kept in sync by hand. That copy drifting from this one is not a +/// hypothetical: it is what ME-8 was, and it went unnoticed because a wrong +/// selector still yields "unsupported selector" — the rejection tests stay +/// green while the accept path quietly stops working. +pub mod selectors { + pub use crate::calls::claim_shielded_fees::SELECTOR as CLAIM_SHIELDED_FEES; + pub use crate::calls::private_transfer::SELECTOR as PRIVATE_TRANSFER; + pub use crate::calls::shield::SELECTOR as SHIELD; + pub use crate::calls::unshield::SELECTOR as UNSHIELD; +} + use core::marker::PhantomData; use fp_evm::{ExitError, Precompile, PrecompileFailure, PrecompileHandle, PrecompileResult}; @@ -19,7 +33,7 @@ use sp_runtime::traits::Dispatchable; /// | Selector | Solidity signature | /// |-------------|-----------------------------------------------------------------------------------------------| /// | `0x9feb22ea` | `shield(uint32,bytes32,bytes)` — payable, amount = `msg.value` | -/// | `0x66ed2cd4` | `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32)` | +/// | `0x1ec439cf` | `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)` | /// | `0x4e505348` | `unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256,bytes32,bytes,uint32)` | /// | `0x88d9deba` | `claimShieldedFees(bytes32,uint256,uint32,bytes,bytes,bytes,uint32)` — signed (validators) | /// diff --git a/frame/evm/precompile/shielded-pool/src/tests.rs b/frame/evm/precompile/shielded-pool/src/tests.rs index 67e4f7f2..09349cf0 100644 --- a/frame/evm/precompile/shielded-pool/src/tests.rs +++ b/frame/evm/precompile/shielded-pool/src/tests.rs @@ -37,6 +37,23 @@ fn expect_error(result: Result) { ); } +/// Like `expect_error`, but pins the REASON. +/// +/// `expect_error` only checks the error variant, and this decoder has a dozen +/// ways to fail before it ever reaches the field under test — so a test named +/// after one rejection can pass on a completely different one. +fn expect_error_msg(result: Result, needle: &str) { + match result { + Err(PrecompileFailure::Error { + exit_status: ExitError::Other(msg), + }) => assert!( + msg.contains(needle), + "expected an error containing {needle:?}, got: {msg:?}" + ), + other => panic!("expected PrecompileFailure::Error(Other), got: {other:?}"), + } +} + fn assert_success(result: Result) { match result { Ok(out) => assert_eq!(out.exit_status, fp_evm::ExitSucceed::Stopped), @@ -117,9 +134,9 @@ fn encode_shield(asset_id: u32, commitment: [u8; 32], memo: &[u8]) -> Vec { input } -/// `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256)` selector `0x8c0f5d24` +/// `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)` selector `0x1ec439cf` #[allow(clippy::too_many_arguments)] -fn encode_private_transfer( +fn encode_private_transfer_with_blob( proof: &[u8], merkle_root: [u8; 32], nullifiers: &[[u8; 32]], @@ -128,20 +145,23 @@ fn encode_private_transfer( asset_id: u32, fee: u128, circuit_version: u32, + ovk_blob: &[u8], ) -> Vec { let proof_enc = encode_bytes(proof); let nullifiers_enc = encode_bytes32_array(nullifiers); let commitments_enc = encode_bytes32_array(commitments); let memos_enc = encode_bytes_array(memos); + let blob_enc = encode_bytes(ovk_blob); - // head: 8 slots × 32 = 256 bytes (added trailing uint32 circuitVersion) - let head_size = 256usize; + // head: 9 slots × 32 = 288 bytes (trailing bytes = ovk_blob) + let head_size = 288usize; let off_proof = head_size; let off_nullifiers = off_proof + proof_enc.len(); let off_commitments = off_nullifiers + nullifiers_enc.len(); let off_memos = off_commitments + commitments_enc.len(); + let off_blob = off_memos + memos_enc.len(); - let mut input = vec![0x66, 0xed, 0x2c, 0xd4]; + let mut input = crate::calls::private_transfer::SELECTOR.to_vec(); let mut head = vec![0u8; head_size]; head[0..32].copy_from_slice(&u256_word(off_proof)); head[32..64].copy_from_slice(&merkle_root); @@ -151,15 +171,42 @@ fn encode_private_transfer( head[188..192].copy_from_slice(&asset_id.to_be_bytes()); head[192..224].copy_from_slice(&u256_word_u128(fee)); head[252..256].copy_from_slice(&circuit_version.to_be_bytes()); + head[256..288].copy_from_slice(&u256_word(off_blob)); input.extend_from_slice(&head); input.extend_from_slice(&proof_enc); input.extend_from_slice(&nullifiers_enc); input.extend_from_slice(&commitments_enc); input.extend_from_slice(&memos_enc); + input.extend_from_slice(&blob_enc); input } +/// Same as `encode_private_transfer_with_blob`, with a valid-length blob. +#[allow(clippy::too_many_arguments)] +fn encode_private_transfer( + proof: &[u8], + merkle_root: [u8; 32], + nullifiers: &[[u8; 32]], + commitments: &[[u8; 32]], + memos: &[Vec], + asset_id: u32, + fee: u128, + circuit_version: u32, +) -> Vec { + encode_private_transfer_with_blob( + proof, + merkle_root, + nullifiers, + commitments, + memos, + asset_id, + fee, + circuit_version, + &[0x0Bu8; 56], + ) +} + /// `unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256,bytes32,bytes,uint32)` selector `0x4e505348` #[allow(clippy::too_many_arguments)] fn encode_unshield( @@ -176,7 +223,7 @@ fn encode_unshield( ) -> Vec { // head: 10 slots × 32 = 320 bytes (added trailing uint32 circuitVersion); // tails (proof, memo) appended after. - let mut input = vec![0x4e, 0x50, 0x53, 0x48]; + let mut input = crate::calls::unshield::SELECTOR.to_vec(); let mut head = vec![0u8; 320]; let proof_offset = 320usize; let memo_offset = proof_offset + encode_bytes(proof).len(); @@ -459,7 +506,7 @@ fn shield_with_zero_value_rejected() { #[test] fn private_transfer_rejects_truncated_input() { new_test_ext().execute_with(|| { - let mut h = MockHandle::new(vec![0x8c, 0x0f, 0x5d, 0x24]); + let mut h = MockHandle::new(crate::calls::private_transfer::SELECTOR.to_vec()); expect_error(ShieldedPoolPrecompile::::execute(&mut h)); }); } @@ -549,6 +596,103 @@ fn private_transfer_rejects_mismatched_commitment_memo_count() { }); } +#[test] +fn private_transfer_rejects_blob_of_55_bytes() { + new_test_ext().execute_with(|| { + do_shield(canon(0x55), 5_000); + let root = current_root(); + let input = encode_private_transfer_with_blob( + &[0x01], + root, + &[[0x11; 32]], + &[canon(0x33)], + &[vec![0xAA; 180]], + 0, + 0, + 1, + &[0x0B; 55], // one byte short + ); + let mut h = MockHandle::new(input); + expect_error_msg( + ShieldedPoolPrecompile::::execute(&mut h), + "ovk blob must be exactly 56 bytes", + ); + }); +} + +#[test] +fn private_transfer_rejects_blob_of_57_bytes() { + new_test_ext().execute_with(|| { + do_shield(canon(0x55), 5_000); + let root = current_root(); + let input = encode_private_transfer_with_blob( + &[0x01], + root, + &[[0x11; 32]], + &[canon(0x33)], + &[vec![0xAA; 180]], + 0, + 0, + 1, + &[0x0B; 57], // one byte long + ); + let mut h = MockHandle::new(input); + expect_error_msg( + ShieldedPoolPrecompile::::execute(&mut h), + "ovk blob must be exactly 56 bytes", + ); + }); +} + +#[test] +fn private_transfer_rejects_empty_blob() { + // The likeliest wrong shape in practice: a caller that knows about the new + // field but has nothing to put there emits `bytes` of length 0, not 55. If + // that decoded, the sender would silently lose recoverability. + new_test_ext().execute_with(|| { + do_shield(canon(0x55), 5_000); + let root = current_root(); + let input = encode_private_transfer_with_blob( + &[0x01], + root, + &[[0x11; 32]], + &[canon(0x33)], + &[vec![0xAA; 180]], + 0, + 0, + 1, + &[], + ); + let mut h = MockHandle::new(input); + expect_error_msg( + ShieldedPoolPrecompile::::execute(&mut h), + "ovk blob must be exactly 56 bytes", + ); + }); +} + +#[test] +fn private_transfer_rejects_missing_blob_slot() { + // Calldata with the old 8-slot head (256 bytes of params) must be rejected + // by the 288-byte minimum before any slot is decoded. + new_test_ext().execute_with(|| { + let mut input = crate::calls::private_transfer::SELECTOR.to_vec(); + input.extend_from_slice(&[0u8; 256]); + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + }); +} + +#[test] +fn private_transfer_selector_matches_signature() { + // The constant must be derived from the ABI signature — this is the guard + // that would have caught ME-8 (whitelist selector never matching the code). + let sig = + b"privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)"; + let hash = sp_io::hashing::keccak_256(sig); + assert_eq!(hash[..4], crate::calls::private_transfer::SELECTOR); +} + #[test] fn private_transfer_happy_path() { new_test_ext().execute_with(|| { @@ -888,3 +1032,413 @@ fn shield_asset_id_round_trips_through_abi() { ); } } + +// ───────────────────────────────────────────────────────────────────────────── +// Adversarial ABI battery — the attacker controls every byte of `input` +// +// This is the only surface where untrusted bytes reach the node directly: an +// EVM caller can send arbitrary calldata to the precompile address. Each test +// below is an attempt to make the decoder panic, over-allocate, or read out of +// bounds. A panic here is a node crash, not a rejected transaction. +// ───────────────────────────────────────────────────────────────────────────── + +/// Truncated calldata at every length from the selector to a full head. None of +/// these may panic — the decoder must reject each one cleanly. +#[test] +fn attack_truncation_at_every_offset_never_panics() { + new_test_ext().execute_with(|| { + let full = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + for len in 0..full.len().min(600) { + let mut h = MockHandle::new(full[..len].to_vec()); + // Must not panic. Any Result is acceptable. + let _ = ShieldedPoolPrecompile::::execute(&mut h); + } + }); +} + +/// An offset pointing back into the head makes the "length" word overlap the +/// caller-controlled head — a classic way to fabricate a huge length. +#[test] +fn attack_self_referential_offset_is_refused() { + new_test_ext().execute_with(|| { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + // Point the proof offset at slot 0 of the head (offset 0 → itself). + input[4..36].copy_from_slice(&[0u8; 32]); + let mut h = MockHandle::new(input); + let _ = ShieldedPoolPrecompile::::execute(&mut h); + }); +} + +/// Every dynamic offset set to u256::MAX. word_to_usize must reject before any +/// slicing arithmetic happens. +#[test] +fn attack_max_u256_offsets_are_refused_not_truncated() { + new_test_ext().execute_with(|| { + for slot in [0usize, 64, 96, 128, 256] { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + input[4 + slot..4 + slot + 32].copy_from_slice(&[0xFFu8; 32]); + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + } + }); +} + +/// 2^32 + small: the low 32 bits look like a valid offset while the value is +/// astronomically out of range. This is the exact shape that `low_u32` let +/// through historically. +#[test] +fn attack_offset_above_u32_that_looks_benign_is_refused() { + new_test_ext().execute_with(|| { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + // 2^32 + 288 — low 32 bits read as 288, a perfectly plausible offset. + let sneaky = U256::from(1u64 << 32) + U256::from(288u64); + let word = sneaky.to_big_endian(); + input[4..36].copy_from_slice(&word); + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + }); +} + +/// A declared array count of ~1e9 must be refused BEFORE Vec::with_capacity +/// reserves for it — otherwise one call OOMs the node. +#[test] +fn attack_huge_array_count_does_not_allocate() { + new_test_ext().execute_with(|| { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + // Find the nullifiers array offset and overwrite its count word. + let off = U256::from_big_endian(&input[4 + 64..4 + 96]).as_usize(); + let count_at = 4 + off; + if count_at + 32 <= input.len() { + let huge = U256::from(1u64 << 30).to_big_endian(); + input[count_at..count_at + 32].copy_from_slice(&huge); + } + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + }); +} + +/// A blob of every wrong length must be refused with the blob's own message — +/// not silently truncated or padded into a valid-looking 56 bytes. +#[test] +fn attack_every_wrong_blob_length_is_refused() { + new_test_ext().execute_with(|| { + for len in [0usize, 1, 32, 55, 57, 64, 1024] { + let input = encode_private_transfer_with_blob( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + &vec![0x0Bu8; len], + ); + let mut h = MockHandle::new(input); + expect_error_msg( + ShieldedPoolPrecompile::::execute(&mut h), + "ovk blob must be exactly 56 bytes", + ); + } + }); +} + +/// A fee word of u256::MAX must not panic converting to u128 — it must be +/// rejected as an overflow. +#[test] +fn attack_max_fee_word_is_refused_not_panicking() { + new_test_ext().execute_with(|| { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + input[4 + 192..4 + 224].copy_from_slice(&[0xFFu8; 32]); + let mut h = MockHandle::new(input); + expect_error_msg( + ShieldedPoolPrecompile::::execute(&mut h), + "fee overflow", + ); + }); +} + +/// asset_id and circuit_version live in u32 slots. A word with high bits set +/// must be rejected, never truncated to a plausible small number. +#[test] +fn attack_oversized_u32_slots_are_refused_not_truncated() { + new_test_ext().execute_with(|| { + for slot in [160usize, 224] { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + // 2^32 exactly: truncates to 0 if the decoder uses low_u32. + let word = U256::from(1u64 << 32).to_big_endian(); + input[4 + slot..4 + slot + 32].copy_from_slice(&word); + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + } + }); +} + +/// Random fuzz over the whole calldata: flip bytes everywhere and assert the +/// decoder never panics. Deterministic (fixed LCG) so a failure reproduces. +#[test] +fn attack_byte_fuzz_never_panics() { + new_test_ext().execute_with(|| { + let base = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + let mut seed: u64 = 0x2545F4914F6CDD1D; + for _ in 0..3000 { + let mut input = base.clone(); + // 1–8 mutations per round. + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let muts = 1 + (seed >> 60) as usize % 8; + for _ in 0..muts { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let pos = (seed >> 33) as usize % input.len(); + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + input[pos] = (seed >> 40) as u8; + } + let mut h = MockHandle::new(input); + // Only requirement: no panic. + let _ = ShieldedPoolPrecompile::::execute(&mut h); + } + }); +} + +/// Fuzz with truncation AND mutation combined — the shape most likely to hit an +/// unchecked slice near a boundary. +#[test] +fn attack_truncated_fuzz_never_panics() { + new_test_ext().execute_with(|| { + let base = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + let mut seed: u64 = 0x9E3779B97F4A7C15; + for _ in 0..2000 { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let cut = 4 + (seed >> 33) as usize % base.len().max(1); + let mut input = base[..cut.min(base.len())].to_vec(); + if !input.is_empty() { + seed = seed + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let pos = (seed >> 33) as usize % input.len(); + input[pos] = (seed >> 40) as u8; + } + let mut h = MockHandle::new(input); + let _ = ShieldedPoolPrecompile::::execute(&mut h); + } + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Adversarial: the OVK blob over the EVM route +// +// The SCALE route gets its 56-byte guarantee from the type itself ([u8;56]). +// The EVM route does not: calldata carries a dynamic `bytes`, so the decoder is +// the ONLY thing pinning the length. These probe that boundary from the side an +// attacker controls byte-for-byte. +// ───────────────────────────────────────────────────────────────────────────── + +/// A blob whose declared ABI length disagrees with the bytes that follow must +/// be refused — not padded, not truncated into a valid-looking 56. +#[test] +fn attack_blob_length_prefix_lying_about_its_payload_is_refused() { + new_test_ext().execute_with(|| { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + // Walk to the blob's length word (head slot 8 → offset) and claim 56 + // bytes are 4096 — the payload after it is still only 56. + let blob_off = U256::from_big_endian(&input[4 + 256..4 + 288]).as_usize(); + let len_at = 4 + blob_off; + if len_at + 32 <= input.len() { + let lie = U256::from(4096u64).to_big_endian(); + input[len_at..len_at + 32].copy_from_slice(&lie); + } + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + }); +} + +/// The blob's ABI offset pointing into the middle of another field must not let +/// the decoder reinterpret that field's bytes as a blob. +#[test] +fn attack_blob_offset_aliasing_another_field_is_refused_or_rejected() { + new_test_ext().execute_with(|| { + let mut input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + ); + // Point the blob offset at the proof's region: whatever it reads there + // is not a 56-byte blob, so it must fail rather than silently accept. + let proof_off = U256::from_big_endian(&input[4..36]); + input[4 + 256..4 + 288].copy_from_slice(&proof_off.to_big_endian()); + let mut h = MockHandle::new(input); + expect_error(ShieldedPoolPrecompile::::execute(&mut h)); + }); +} + +/// Every blob byte pattern of the CORRECT length must decode — the chain holds +/// no key and must not editorialize about ciphertext. Zeros included: the +/// all-zero blob is a wallet-side smell, never a consensus rule. +#[test] +fn attack_any_56_byte_blob_decodes_including_zeros() { + new_test_ext().execute_with(|| { + for pattern in [0x00u8, 0xFF, 0x0B, 0xAA] { + let input = encode_private_transfer_with_blob( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[vec![0x01u8; 180]], + 0, + 0, + 1, + &[pattern; 56], + ); + let mut h = MockHandle::new(input); + // Reaches dispatch (the mock has no registered asset, so the error + // is a dispatch one, never an ABI/blob-length rejection). + let result = ShieldedPoolPrecompile::::execute(&mut h); + if let Err(fp_evm::PrecompileFailure::Error { + exit_status: ExitError::Other(msg), + }) = &result + { + assert!( + !msg.contains("ovk blob"), + "pattern {pattern:#x} must not be rejected as a blob: {msg}" + ); + } + } + }); +} + +/// The memo bytes where `sourcePk` sits are ciphertext to the chain. Two calls +/// differing only there must be treated identically by the decoder — a decoder +/// that could tell them apart would mean the field was not encrypted. +#[test] +fn attack_decoder_is_blind_to_the_sourcepk_region_of_the_memo() { + new_test_ext().execute_with(|| { + let mut memo_a = vec![0x01u8; 180]; + let mut memo_b = vec![0x01u8; 180]; + memo_a[84..116].fill(0x00); + memo_b[84..116].fill(0xAB); + + let outcomes: Vec = [memo_a, memo_b] + .into_iter() + .map(|memo| { + let input = encode_private_transfer( + &[0x01u8; 72], + canon(0xBB), + &[canon(1)], + &[canon(2)], + &[memo], + 0, + 0, + 1, + ); + let mut h = MockHandle::new(input); + ShieldedPoolPrecompile::::execute(&mut h).is_ok() + }) + .collect(); + assert_eq!( + outcomes[0], outcomes[1], + "the decoder must not distinguish memos by their sourcePk region" + ); + }); +} diff --git a/frame/shielded-pool/CHANGELOG.md b/frame/shielded-pool/CHANGELOG.md index a5ac0713..8eaf3a8a 100644 --- a/frame/shielded-pool/CHANGELOG.md +++ b/frame/shielded-pool/CHANGELOG.md @@ -2,6 +2,84 @@ All notable changes to `pallet-shielded-pool` will be documented in this file. +## [0.18.0] - 2026-08-10 + +### Added + +- **`private_transfer` carries a 56-byte OVK blob, published as its own event.** + A memo is sealed toward the RECIPIENT, so the sender cannot reopen it — a + sender who loses their vault loses all record of what they sent. The blob + wraps that memo's shared secret under the sender's outgoing viewing key, so + the sender can reach the same plaintext the recipient does. + + The call takes a new trailing `ovk_blob: OvkBlob` argument and emits + `OutgoingBlobPublished { commitment, blob }`, bound to `commitments[0]` (the + recipient output by the wallet's positional convention). A separate event + rather than a field on `CommitmentsInserted`, which is shared with `shield`, + `shield_batch` and `unshield` — none of which carry a blob. + + `OvkBlob` is a fixed `[u8; 56]`, so the SCALE codec rejects any other length + before the extrinsic decodes. It has **no `Default`**: that would produce the + 56 zeros the design forbids, from a call that reads as harmless. + + The chain treats the blob as opaque and validates only its length. It cannot + do otherwise — the contents are ciphertext and no key exists on chain — so it + accepts *any* 56 bytes, zeros included. A sender who opts out of + recoverability publishes 56 RANDOM bytes: zeros would be greppable, marking + the opt-out forever and making every user who chose it a trivially + identifiable set. Presence and size therefore reveal nothing. + + **Breaking:** the extrinsic signature changes. `transaction_version` is + bumped 2 → 3, and wallet and runtime must ship together. + +### Security + +- **Pool admission tags one entry per NULLIFIER, in one namespace shared with + `unshield`.** `and_provides` contributes exactly ONE tag, so passing it a + `Vec` encoded the whole nullifier set (plus the relayer) into a single blob. + Three consequences, all of them free for an attacker since the fee is only + charged on execution: + + - reordering the two inputs produced a different tag, minting a second + admissible pool entry for the same spend; + - two transfers sharing only ONE note (A+B and A+C) did not collide at all, + so a single note could back an unbounded number of pool entries; + - `private_transfer` and `unshield` used different tag prefixes, so the same + note could back one of each simultaneously. + + Every variant propagates and is revalidated network-wide while at most one can + ever execute. Now each real nullifier contributes its own tag under a shared + `ShieldedPoolSpend` prefix, so pool admission mirrors the chain's rule: one + note, one entry. + + The `relayer` deliberately no longer enters the tag. Binding it made a copy + with a swapped fee recipient a *separate* entry, so anyone could rebroadcast + another user's spend pointed at their own account and have both sit in the + pool. Keyed on the nullifier the two are mutually exclusive, so taking the fee + requires out-bidding — which means actually paying it. + + Consensus is unaffected: this is admission policy, not state transition. Nodes + still running the old logic keep accepting the duplicate variants, so the + mitigation is only complete once the network updates. + +### Changed + +- **Weights re-benchmarked, clearing the STALE marker on `private_transfer`.** + The OVK argument landed without re-measuring, so the file carried an explicit + warning that its numbers predated the change. This run covers the pallet as it + now stands. + + Every measured value moved up: +1.9% on average, +9% worst case. + `private_transfer`'s marginal cost per output went 910M → 989M ps and remains + parameterised by output count — `private_transfer_weight_scales_with_outputs` + still passes, so the second leaf insert is not under-priced. + + Run: 2026-08-11, `ubuntu-32gb-hel1-1` (AMD EPYC-Genoa, 32 GB), steps 50 / + repeat 20, `--wasm-execution=compiled`. Note this is a **different instance** + from the `fsn1` host used through 0.17.0; same CPU family, but cloud instances + vary enough that small deltas across releases are measurement noise rather + than real cost changes. + ## [0.17.0] - 2026-08-07 ### Security diff --git a/frame/shielded-pool/Cargo.toml b/frame/shielded-pool/Cargo.toml index ae846927..f813b1a0 100644 --- a/frame/shielded-pool/Cargo.toml +++ b/frame/shielded-pool/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pallet-shielded-pool" -version = "0.17.0" +version = "0.18.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 19cd3116..b4faff37 100644 --- a/frame/shielded-pool/src/benchmarking.rs +++ b/frame/shielded-pool/src/benchmarking.rs @@ -171,6 +171,7 @@ mod benchmarks { fee, Some(relayer), 1u32, + crate::types::OvkBlob([0x0Bu8; crate::types::OVK_BLOB_SIZE]), ); } diff --git a/frame/shielded-pool/src/lib.rs b/frame/shielded-pool/src/lib.rs index bfd31aef..97b3135b 100644 --- a/frame/shielded-pool/src/lib.rs +++ b/frame/shielded-pool/src/lib.rs @@ -83,7 +83,7 @@ mod runtime_api_impl; pub use types::{ AssetId, AssetMetadata, Commitment, DEFAULT_TREE_DEPTH, DefaultMerklePath, EncryptedMemo as FrameEncryptedMemo, Hash, MAX_ENCRYPTED_MEMO_SIZE, MAX_TREE_DEPTH, MerklePath, - Note, Nullifier, + Note, Nullifier, OVK_BLOB_SIZE, OvkBlob, }; #[frame_support::pallet] @@ -567,6 +567,23 @@ pub mod pallet { leaf_indices: BoundedVec>, }, + /// The OVK blob of a private transfer, bound to its recipient output. + /// + /// Its own event rather than a field on `CommitmentsInserted`, because that + /// event is shared with shield, shield_batch and unshield — none of which + /// carry a blob. Widening it would force an `Option` on three unrelated + /// call sites and change the shape every historic event decodes under. + /// + /// Emitted for EVERY transfer, always 56 bytes: a sender who opts out of + /// recoverability publishes random bytes, so presence and size reveal + /// nothing about whether anyone opted in. + OutgoingBlobPublished { + /// Recipient output commitment the blob is bound to. + commitment: Commitment, + /// The 56-byte OVK blob (or 56 random bytes when the sender opted out). + blob: OvkBlob, + }, + /// Tokens were withdrawn from the shielded pool Unshielded { /// Nullifier of the spent note @@ -808,6 +825,10 @@ pub mod pallet { /// * `encrypted_memos` - Encrypted metadata for each new note /// * `asset_id` - Asset being transferred (public input of the proof) /// * `fee` - Gasless fee (must match proof's fee public input) + /// * `ovk_blob` - 56-byte outgoing-viewing-key blob for commitments[0], + /// letting the SENDER recover this transfer later. Opaque to the chain: + /// it is ciphertext, and no key to check it against exists on chain. + /// Only the length is guaranteed, and the type is what guarantees it. /// /// # Errors /// * `UnknownMerkleRoot` - Root is not in historic roots @@ -830,6 +851,7 @@ pub mod pallet { fee: BalanceOf, relayer: Option, circuit_version: u32, + ovk_blob: OvkBlob, ) -> DispatchResult { ensure_none(origin)?; @@ -844,6 +866,7 @@ pub mod pallet { fee, relayer, circuit_version, + ovk_blob, ) } @@ -889,7 +912,10 @@ pub mod pallet { // For partial unshield, must equal NoteCommitment(change_value, asset_id, change_owner_pk, change_blinding). change_commitment: Hash, // Encrypted memo for the change note. Must be [0u8; 0] for total unshield. - // For partial unshield, contains encrypted plaintext: [value_lo(8), value_hi(8), owner_pk(32), blinding(32), asset_id(4), counterparty_pk(32)]. + // For partial unshield it wraps the 120-byte plaintext: value_lo(8), + // value_hi(8), owner_pk(32), blinding(32), asset_id(4), source_pk(32), + // circuit_version(4). Opaque to the chain — the layout is the wallet's + // contract, listed here only as a reader's aid. change_encrypted_memo: FrameEncryptedMemo, // EVM address of the relay node that signed the tx (from precompile caller); None for direct Substrate. relayer: Option, @@ -1074,6 +1100,12 @@ pub mod pallet { fee, relayer, circuit_version, + // `ovk_blob` is deliberately not forwarded. The blob is + // unauthenticated ciphertext, so binding it into the `provides` + // tag would let anyone who saw a pending tx re-broadcast endless + // variants of it — same nullifiers, different blob, each landing + // as its own pool entry. The tag stays on the nullifiers, which + // the ZK proof does authenticate. .. } => crate::validate_unsigned::validate_private_transfer::( merkle_root, diff --git a/frame/shielded-pool/src/merkle/mod.rs b/frame/shielded-pool/src/merkle/mod.rs index ee311cb4..ddf849ba 100644 --- a/frame/shielded-pool/src/merkle/mod.rs +++ b/frame/shielded-pool/src/merkle/mod.rs @@ -1413,4 +1413,166 @@ mod prune_tests { and a value this far above the benchmarked batch would let one block \ absorb work it cannot pay for" ); + + // ── adversarial: the Merkle accounting ─────────────────────────────────── + // + // The tree IS the ledger: a leaf that cannot be proven is money nobody can + // spend, and a root that disagrees with its leaves lets a forged proof pass. + + /// A commitment may take a leaf ONCE. Two leaves for one note would let a + /// wallet's scan see the same money twice, while the nullifier set — keyed on + /// the note, not the leaf — allows only one spend. + /// + /// Note WHERE the guard lives: `insert_leaf` tests `CommitmentMemos`, which + /// is populated by `store_memo`, called by the operations right after each + /// insert. So the duplicate is caught once the memo of the first insert has + /// been stored — which is the real dispatch order (see + /// `operations/private_transfer.rs`, the insert/store_memo loop). Calling the + /// service in isolation, as a future refactor might, does NOT arm the guard; + /// this test pins the pairing so that dependency stays visible. + #[test] + fn attack_reinserting_a_commitment_is_refused() { + new_test_ext().execute_with(|| { + use crate::storage::CommitmentRepository; + use crate::types::{EncryptedMemo, MAX_ENCRYPTED_MEMO_SIZE}; + + let c = Commitment::new({ + let mut b = [0u8; 32]; + b[0] = 0x11; + b[1] = 0xA5; + b + }); + let memo = + EncryptedMemo::from_bytes(&[0x01u8; MAX_ENCRYPTED_MEMO_SIZE as usize]).unwrap(); + + // The production pairing: insert the leaf, then store its memo. + assert!(MerkleTreeService::insert_leaf::(c).is_ok()); + CommitmentRepository::store_memo::(c, memo); + + assert!( + MerkleTreeService::insert_leaf::(c).is_err(), + "a duplicate commitment must never take a second leaf" + ); + }); + } + + /// Rolling over into a fresh tree must not carry the previous tree's frontier: + /// a stale frontier makes the new tree's root a hash of leaves that are not + /// in it, so every proof against it fails. + #[test] + fn attack_tree_rollover_starts_from_a_clean_frontier() { + new_test_ext().execute_with(|| { + let cap = ::MaxLeavesPerTree::get(); + + // Fill tree 0 exactly, which seals it. + for i in 0..cap { + let mut b = [0u8; 32]; + b[0] = (i + 1) as u8; + b[1] = 0xA5; + assert!(MerkleTreeService::insert_leaf::(Commitment::new(b)).is_ok()); + } + + // After the seal the live root is the EMPTY root, not tree 0's root. + let empty = super::hashing::get_zero_hash_cached(crate::types::DEFAULT_TREE_DEPTH); + assert_eq!( + MerkleRepository::get_poseidon_root::(), + empty, + "a sealed rollover must reset the live root to the empty tree" + ); + assert_eq!( + MerkleRepository::get_frontier::(), + [[0u8; 32]; crate::types::DEFAULT_TREE_DEPTH], + "a stale frontier would root the new tree over leaves it does not hold" + ); + + // The first leaf of tree 1 must produce the same root as the first + // leaf of a brand-new tree — proof the rollover is clean. + let mut b = [0u8; 32]; + b[0] = 0xC1; + b[1] = 0xA5; + assert!(MerkleTreeService::insert_leaf::(Commitment::new(b)).is_ok()); + let after_rollover = MerkleRepository::get_poseidon_root::(); + + new_test_ext().execute_with(|| { + let mut b2 = [0u8; 32]; + b2[0] = 0xC1; + b2[1] = 0xA5; + assert!(MerkleTreeService::insert_leaf::(Commitment::new(b2)).is_ok()); + assert_eq!( + MerkleRepository::get_poseidon_root::(), + after_rollover, + "tree 1's first leaf must root exactly like a fresh tree's first leaf" + ); + }); + }); + } + + /// A sealed tree's root must stay accepted for spending: notes in it are + /// still live money. Losing it would strand every note in the sealed tree. + #[test] + fn attack_sealing_does_not_strand_the_notes_it_sealed() { + new_test_ext().execute_with(|| { + let cap = ::MaxLeavesPerTree::get(); + let mut last_root = [0u8; 32]; + for i in 0..cap { + let mut b = [0u8; 32]; + b[0] = (i + 1) as u8; + b[1] = 0xA5; + assert!(MerkleTreeService::insert_leaf::(Commitment::new(b)).is_ok()); + last_root = MerkleRepository::get_poseidon_root::(); + } + // `last_root` here is the empty root (the seal already ran), so check + // the SEALED root is retrievable and known. + let sealed = MerkleRepository::get_sealed_root::(0); + assert!(sealed.is_some(), "the sealed root must be recorded"); + assert!( + MerkleRepository::is_known_root::(&sealed.unwrap()), + "a sealed root must remain spendable, or its notes are stranded" + ); + let _ = last_root; + }); + } + + /// Leaf indices must be dense and monotonic across a rollover. A gap or a + /// repeat desynchronises every wallet's scan cursor from the chain. + #[test] + fn attack_leaf_indices_stay_dense_across_a_rollover() { + new_test_ext().execute_with(|| { + let cap = ::MaxLeavesPerTree::get(); + let total = cap + 3; // force a rollover and then some + for i in 0..total { + let mut b = [0u8; 32]; + b[0] = (i + 1) as u8; + b[1] = 0xA5; + let index = MerkleTreeService::insert_leaf::(Commitment::new(b)) + .expect("insert must succeed"); + assert_eq!(index, i, "leaf indices must be dense and monotonic"); + } + assert_eq!(MerkleRepository::get_tree_size::(), total); + }); + } + + /// The root must actually change on every insert. A root that repeats means + /// two different leaf sets share one root — a forged proof would verify. + #[test] + fn attack_every_insert_moves_the_root() { + new_test_ext().execute_with(|| { + let cap = ::MaxLeavesPerTree::get(); + let mut seen = alloc::vec::Vec::new(); + // Stop one short of the cap so the seal (which deliberately resets to + // the empty root) does not count as a repeat. + for i in 0..(cap - 1) { + let mut b = [0u8; 32]; + b[0] = (i + 1) as u8; + b[1] = 0xA5; + assert!(MerkleTreeService::insert_leaf::(Commitment::new(b)).is_ok()); + let root = MerkleRepository::get_poseidon_root::(); + assert!( + !seen.contains(&root), + "root repeated after insert {i} — distinct leaf sets must not share a root" + ); + seen.push(root); + } + }); + } } diff --git a/frame/shielded-pool/src/operations/private_transfer.rs b/frame/shielded-pool/src/operations/private_transfer.rs index 7832b206..96889a29 100644 --- a/frame/shielded-pool/src/operations/private_transfer.rs +++ b/frame/shielded-pool/src/operations/private_transfer.rs @@ -1,8 +1,39 @@ +//! `private_transfer` — spending shielded notes into new shielded notes. +//! +//! Nothing about the amounts is public: the ZK proof attests that inputs equal +//! outputs plus fee. What the pallet checks is everything the proof does NOT +//! cover — that the inputs exist, are unspent, and are spelled canonically, and +//! that the arrays line up. +//! +//! ## Order of steps +//! +//! Numbered below. The split matters: every validation (steps 1–6) runs BEFORE +//! any state changes (steps 7–9), and the expensive proof verification (step 6) +//! runs LAST among the checks, so a transaction that fails a cheap structural +//! test never costs a pairing. +//! +//! | # | Step | Touches state | +//! |---|-----------------------------------|---------------| +//! | 1 | asset registered and verified | no | +//! | 2 | arrays line up, memos exact-sized | no | +//! | 3 | Merkle root known | no | +//! | 4 | nullifiers canonical and unspent | no | +//! | 5 | commitments canonical and non-zero| no | +//! | 6 | fee floor, then verify the proof | no | +//! | 7 | burn the inputs (mark nullifiers) | yes | +//! | 8 | insert outputs, store memos | yes | +//! | 9 | accrue the relay fee, emit events | yes | +//! +//! Every check here is also enforced at pool admission +//! ([`crate::validate_unsigned::transfer`]), which may reject more but never +//! less: admission can be skipped by a malicious block author, so this is the +//! authority. + use crate::{ merkle::MerkleTreeService, pallet::{Config, Error, Event, Pallet}, storage::{AssetRepository, CommitmentRepository, MerkleRepository, NullifierRepository}, - types::{Commitment, EncryptedMemo, MAX_ENCRYPTED_MEMO_SIZE, Nullifier}, + types::{Commitment, EncryptedMemo, MAX_ENCRYPTED_MEMO_SIZE, Nullifier, OvkBlob}, }; use frame_support::{BoundedVec, pallet_prelude::*, traits::Currency}; use pallet_relayer::RelayerInterface as _; @@ -24,10 +55,17 @@ impl PrivateTransferOperation { fee: <::Currency as Currency<::AccountId>>::Balance, relayer_evm: Option, circuit_version: u32, + ovk_blob: OvkBlob, ) -> DispatchResult { + // ── 1. Asset ───────────────────────────────────────────────────────── let asset = AssetRepository::get_asset::(asset_id).ok_or(Error::::InvalidAssetId)?; ensure!(asset.is_verified, Error::::AssetNotVerified); + // ── 2. Array arity and memo size ───────────────────────────────────── + // The three arrays run in parallel — input i, output i, memo i — so a + // mismatch is a call that cannot mean anything. Memos are exact-sized, + // never merely bounded: the wallet slices them at fixed offsets, so a + // short one is a note nobody can open. ensure!( nullifiers.len() == commitments.len(), Error::::TooManyInputsOrOutputs @@ -44,11 +82,21 @@ impl PrivateTransferOperation { ); } + // ── 3. Merkle root ─────────────────────────────────────────────────── + // Membership is proven against a root the chain published; the retention + // window outlives `TX_LONGEVITY`, so a root valid at admission is still + // valid here. ensure!( MerkleRepository::is_known_root::(&merkle_root), Error::::UnknownMerkleRoot ); + // ── 4. Nullifiers ──────────────────────────────────────────────────── + // The dummy sentinel (all zeros) pads a single-input spend and is never + // inserted, so it is skipped rather than checked. For the rest: + // - canonical, because `n` and `n + p` are DIFFERENT storage keys for + // the same field element — two spellings would mean two spends; + // - unspent against the on-chain set. for nullifier in nullifiers.iter() { if nullifier.0 == [0u8; 32] { continue; // dummy input — skipped by circuit, no nullifier to check @@ -60,6 +108,11 @@ impl PrivateTransferOperation { ); } + // Two EQUAL real nullifiers would spend one input twice inside a single + // call: neither is in the set yet when the loop above runs, and the + // second `mark_as_used` is idempotent, so only this comparison catches + // it. The proof is expected to bind the inputs distinct; the chain does + // not rely on that. let non_dummy: sp_std::vec::Vec<&Nullifier> = nullifiers.iter().filter(|n| n.0 != [0u8; 32]).collect(); if non_dummy.len() == 2 { @@ -69,14 +122,24 @@ impl PrivateTransferOperation { ); } + // ── 5. Commitments ─────────────────────────────────────────────────── + // Canonical for the same reason as the nullifiers; non-zero because zero + // is the tree's empty-leaf sentinel. for commitment in commitments.iter() { ensure!(commitment.is_canonical(), Error::::InvalidPublicSignals); ensure!(commitment.is_valid(), Error::::InvalidPublicSignals); } + // All-dummy means no note is being spent, so the outputs would be minted + // out of nothing — free value, not just free tree growth. let all_dummy = nullifiers.iter().all(|n| n.0 == [0u8; 32]); ensure!(!all_dummy, Error::::InvalidAmount); + // ── 6. Fee floor, then the proof ───────────────────────────────────── + // The fee compare is cheap and goes first; proof verification is the + // most expensive thing in the call, so it runs only once everything + // structural has passed. The fee is a PUBLIC input to the circuit, which + // is what stops a relayer from inflating it after the fact. let nullifier_arrays: sp_std::vec::Vec<[u8; 32]> = nullifiers.iter().map(|n| n.0).collect(); let commitment_arrays: sp_std::vec::Vec<[u8; 32]> = commitments.iter().map(|c| c.0).collect(); @@ -112,6 +175,11 @@ impl PrivateTransferOperation { let _ = &proof; } + // ── 7. Burn the inputs ─────────────────────────────────────────────── + // From here on the call mutates state. Nullifiers are marked FIRST: if + // anything below fails, the runtime rolls the whole extrinsic back, but + // ordering the burn ahead of the mint keeps the invariant obvious — + // nothing is created before its input is consumed. let current_block = frame_system::Pallet::::block_number(); for nullifier in nullifiers.iter() { if nullifier.0 == [0u8; 32] { @@ -120,6 +188,10 @@ impl PrivateTransferOperation { NullifierRepository::mark_as_used::(*nullifier, current_block); } + // ── 8. Mint the outputs ────────────────────────────────────────────── + // `zip` pairs each commitment with its own memo — the arity check in + // step 2 is what makes that pairing total rather than silently dropping + // an output. let mut leaf_indices: BoundedVec> = BoundedVec::new(); for (commitment, memo) in commitments.iter().zip(encrypted_memos.iter()) { let index = MerkleTreeService::insert_leaf::(*commitment)?; @@ -129,6 +201,12 @@ impl PrivateTransferOperation { .map_err(|_| Error::::TooManyInputsOrOutputs)?; } + // ── 9. Accrue the relay fee ────────────────────────────────────────── + // The fee never leaves the pool as a transfer: it is credited to the + // relayer's pending balance and claimed later via `claim_shielded_fees`, + // which is what keeps the payout unlinkable from this transaction. + // Falls back to the block author when no relayer is registered for the + // caller's EVM address. if fee > >::Balance::zero() { let recipient_account = relayer_evm .and_then(|addr| T::Relayer::resolve_relayer(&addr)) @@ -140,11 +218,23 @@ impl PrivateTransferOperation { Pallet::::deposit_event(Event::NullifiersSpent { nullifiers: nullifiers.clone(), }); + // The blob belongs to the recipient output — commitments[0] by the + // wallet's positional convention (output[0] = recipient, output[1] = + // change). Nothing on chain enforces that ordering, so it is a contract + // with the wallet, not an invariant the pallet can check. + let ovk_commitment = commitments.first().copied(); + Pallet::::deposit_event(Event::CommitmentsInserted { commitments, encrypted_memos, leaf_indices, }); + if let Some(commitment) = ovk_commitment { + Pallet::::deposit_event(Event::OutgoingBlobPublished { + commitment, + blob: ovk_blob, + }); + } Ok(()) } @@ -165,7 +255,7 @@ mod tests { mock::{Test, acc, new_test_ext}, pallet::Event as PalletEvent, storage::{CommitmentRepository, MerkleRepository, NullifierRepository}, - types::{Commitment, EncryptedMemo, MAX_ENCRYPTED_MEMO_SIZE, Nullifier}, + types::{Commitment, EncryptedMemo, MAX_ENCRYPTED_MEMO_SIZE, Nullifier, OvkBlob}, }; use frame_support::{assert_err, assert_noop, assert_ok}; @@ -225,6 +315,16 @@ mod tests { v } + fn test_blob() -> OvkBlob { + OvkBlob([0x0Bu8; crate::types::OVK_BLOB_SIZE]) + } + + /// The 56 zeros a wallet must never emit — spelled out so a test that uses + /// it is visibly asking for the forbidden value, not reaching for a default. + fn zero_blob() -> OvkBlob { + OvkBlob([0u8; crate::types::OVK_BLOB_SIZE]) + } + // ── execute ─────────────────────────────────────────────────────────────── #[test] @@ -242,6 +342,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); }); } @@ -261,6 +362,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); }); } @@ -280,6 +382,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), crate::pallet::Error::::UnknownMerkleRoot ); @@ -304,6 +407,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), crate::pallet::Error::::NullifierAlreadyUsed ); @@ -328,6 +432,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), crate::pallet::Error::::NullifierAlreadyUsed ); @@ -351,6 +456,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), crate::pallet::Error::::MemoCommitmentMismatch ); @@ -376,6 +482,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), crate::pallet::Error::::InvalidMemoSize ); @@ -402,6 +509,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); assert!(PrivateTransferOperation::is_nullifier_used::(&n1)); @@ -427,6 +535,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); assert!(CommitmentRepository::exists::(&c)); @@ -451,6 +560,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); let events = frame_system::Pallet::::events(); @@ -493,6 +603,7 @@ mod tests { fee, None, 1, + test_blob(), )); // MockRelayer block_author = Some(1) @@ -516,6 +627,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); let pending = crate::mock::mock_pending_fees_get(acc(1), 0u32); @@ -572,6 +684,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); // Real nullifier must be marked used @@ -606,6 +719,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); // Second tx with a different real nullifier but same dummy — must succeed @@ -619,6 +733,7 @@ mod tests { 0u128, None, 1, + test_blob(), )); }); } @@ -645,6 +760,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), Error::::InvalidAmount ); @@ -670,6 +786,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), Error::::TooManyInputsOrOutputs ); @@ -712,6 +829,7 @@ mod tests { fee, None, 1, + test_blob(), )); assert_eq!( @@ -753,6 +871,7 @@ mod tests { 30u128, Some(sp_core::H160::from([0xAA; 20])), 1, + test_blob(), )); assert_eq!(crate::mock::mock_pending_fees_get(relayer_acct, 0u32), 30); @@ -767,6 +886,7 @@ mod tests { 20u128, Some(sp_core::H160::from([0xBB; 20])), 1, + test_blob(), )); assert_eq!(crate::mock::mock_pending_fees_get(acc(1), 0u32), 20); }); @@ -790,6 +910,7 @@ mod tests { 25u128, None, 1, + test_blob(), ), Error::::FeeRecipientUnavailable ); @@ -817,6 +938,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), Error::::AssetNotVerified ); @@ -840,6 +962,7 @@ mod tests { 0u128, None, 1, + test_blob(), ), Error::::InvalidAssetId ); @@ -848,6 +971,91 @@ mod tests { // ── weight scales with outputs ─────────────────────────────────────────── + // ── OVK blob ───────────────────────────────────────────────────────────── + + /// A valid transfer emits OutgoingBlobPublished bound to commitments[0] + /// with the exact blob bytes. + #[test] + fn execute_emits_outgoing_blob_published() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let commitments = commitments_of(&[0xF4, 0xF5]); + let expected_commitment = commitments[0]; + let blob = test_blob(); + + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xF6, 0xF7]), + commitments, + memos_of(2), + 0u32, + 0u128, + None, + 1, + blob.clone(), + )); + + let events = frame_system::Pallet::::events(); + let found = events.iter().any(|r| { + matches!( + &r.event, + crate::mock::RuntimeEvent::ShieldedPool(PalletEvent::OutgoingBlobPublished { + commitment, + blob: eb, + }) if *commitment == expected_commitment && eb == &blob + ) + }); + assert!(found, "OutgoingBlobPublished event not emitted"); + }); + } + + /// The pallet does not judge blob contents — not even the 56 zeros the + /// design forbids. + /// + /// It cannot: the blob is ciphertext, and rejecting a specific value would + /// mean the chain claims to know something about plaintext it cannot read. + /// Keeping zeros out is the WALLET's job (it sends random bytes when the + /// sender opts out), and `OvkBlob` has no `Default` so reaching them takes + /// deliberate effort. + #[test] + fn execute_accepts_all_zero_blob() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x21]), + commitments_of(&[0x22]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + zero_blob(), + )); + }); + } + + /// SCALE codec round-trip of the new type: 56 raw bytes, no length prefix. + #[test] + fn ovk_blob_codec_round_trip() { + use parity_scale_codec::{Decode, Encode}; + + let blob = test_blob(); + let encoded = blob.encode(); + assert_eq!(encoded.len(), 56, "fixed array must encode without prefix"); + let decoded = OvkBlob::decode(&mut &encoded[..]).unwrap(); + assert_eq!(decoded, blob); + + // Wrong length must not decode into the type. + assert!(OvkBlob::decode(&mut &encoded[..55]).is_err()); + } + + // ── weight scales with outputs ─────────────────────────────────────────── + /// A 2-output transfer inserts two leaves and must be weighted heavier than a /// 1-output one (guards against the flat weight that under-priced the second /// insert). @@ -865,4 +1073,673 @@ mod tests { "two outputs must cost more proof_size than one" ); } + + // ── adversarial battery ────────────────────────────────────────────────── + // + // Each of these is an attempt to BREAK an invariant, not a demonstration + // that it holds. They are written from the attacker's side: assume the ZK + // proof is satisfiable (the mock skips verification) and ask what the + // non-cryptographic checks still have to stop on their own. + + /// Double-spend inside ONE extrinsic, same nullifier twice. + /// + /// The set check cannot catch this: neither nullifier is in storage yet when + /// the loop runs, so only the explicit pairwise comparison stands between + /// this and spending one note twice in a single call. + #[test] + fn attack_same_nullifier_twice_in_one_extrinsic_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut nulls: BoundedVec> = BoundedVec::new(); + nulls.try_push(make_nullifier(0x77)).unwrap(); + nulls.try_push(make_nullifier(0x77)).unwrap(); // same note, twice + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nulls, + commitments_of(&[0xC1, 0xC2]), + memos_of(2), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::NullifierAlreadyUsed + ); + }); + } + + /// The dummy nullifier is exempt from the "already used" check by design. + /// Two dummies in one call must therefore NOT be readable as a duplicate + /// pair — but the all-dummy guard has to reject the call outright, or a + /// transfer with no real input mints two free leaves. + #[test] + fn attack_two_dummy_nullifiers_cannot_mint_free_leaves() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut nulls: BoundedVec> = BoundedVec::new(); + nulls.try_push(Nullifier::new([0u8; 32])).unwrap(); + nulls.try_push(Nullifier::new([0u8; 32])).unwrap(); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nulls, + commitments_of(&[0xD1, 0xD2]), + memos_of(2), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::InvalidAmount + ); + }); + } + + /// Replay of a nullifier already spent in an EARLIER block. + #[test] + fn attack_replaying_a_spent_nullifier_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x51]), + commitments_of(&[0x52]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + + // Same nullifier, different outputs — the note is already gone. + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x51]), + commitments_of(&[0x53]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::NullifierAlreadyUsed + ); + }); + } + + /// Non-canonical field elements: bytes above the BN254 modulus that reduce + /// to a DIFFERENT, already-spent value. Accepting them would give every + /// nullifier a second spelling and defeat the double-spend set entirely. + #[test] + fn attack_non_canonical_nullifier_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + // modulus + 1, little-endian — reduces to 1, which is canonical. + let mut over = [0u8; 32]; + over[0] = 0x02; + over[31] = 0xFF; + assert!( + !Nullifier::new(over).is_canonical(), + "fixture must actually be non-canonical or the test proves nothing" + ); + + let mut nulls: BoundedVec> = BoundedVec::new(); + nulls.try_push(Nullifier::new(over)).unwrap(); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nulls, + commitments_of(&[0xE1]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::InvalidPublicSignals + ); + }); + } + + /// Same, on the output side: a non-canonical commitment would land a leaf + /// whose second spelling could collide with a real one. + #[test] + fn attack_non_canonical_commitment_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut over = [0u8; 32]; + over[0] = 0x02; + over[31] = 0xFF; + assert!(!Commitment::new(over).is_canonical()); + + let mut comms: BoundedVec> = BoundedVec::new(); + comms.try_push(Commitment::new(over)).unwrap(); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x61]), + comms, + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::InvalidPublicSignals + ); + }); + } + + /// A forged Merkle root the attacker made up: it lets them prove membership + /// of a note that was never in the tree. + #[test] + fn attack_unknown_merkle_root_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + [0xEEu8; 32], // never added + nullifiers_of(&[0x71]), + commitments_of(&[0x72]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::UnknownMerkleRoot + ); + }); + } + + /// Array-length confusion: more commitments than nullifiers would insert an + /// output nothing paid for. + #[test] + fn attack_more_commitments_than_nullifiers_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x81]), // 1 input + commitments_of(&[0x82, 0x83]), // 2 outputs + memos_of(2), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::TooManyInputsOrOutputs + ); + }); + } + + /// Memo count out of step with the outputs: a missing memo would leave a + /// commitment nobody can ever open, and the zip() that stores them would + /// silently drop the extra output. + #[test] + fn attack_memo_count_mismatch_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x91, 0x92]), + commitments_of(&[0x93, 0x94]), + memos_of(1), // one memo for two outputs + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::MemoCommitmentMismatch + ); + }); + } + + /// A wrong-sized memo must not reach storage: the wallet's decrypt path + /// slices fixed offsets, so a short memo is a note nobody can open. + #[test] + fn attack_undersized_memo_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut memos: BoundedVec> = BoundedVec::new(); + memos.try_push(short_memo()).unwrap(); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xA9]), + commitments_of(&[0xAA]), + memos, + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::InvalidMemoSize + ); + }); + } + + /// The zero commitment is the tree's empty-leaf sentinel. Inserting it as a + /// real output would corrupt the Merkle structure. + #[test] + fn attack_zero_commitment_is_refused() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut comms: BoundedVec> = BoundedVec::new(); + comms.try_push(Commitment::new([0u8; 32])).unwrap(); + + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xB9]), + comms, + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + ), + Error::::InvalidPublicSignals + ); + }); + } + + /// A fee below the relay minimum must be refused BEFORE any state changes — + /// otherwise the pool subsidizes the spam it is meant to price out. + #[test] + fn attack_fee_below_minimum_is_refused_without_spending_the_nullifier() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + let min = ::Relayer::min_relay_fee(); + if min == 0 { + return; // mock has no minimum; nothing to prove here + } + + let n = make_nullifier(0xC9); + assert_err!( + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xC9]), + commitments_of(&[0xCA]), + memos_of(1), + 0u32, + min.saturating_sub(1), + None, + 1, + test_blob(), + ), + Error::::FeeTooLow + ); + // And the note must still be spendable — a rejected call that burned + // the nullifier would destroy funds. + assert!(!NullifierRepository::is_used::(&n)); + }); + } + + /// Two DIFFERENT transfers may legitimately carry the same blob bytes (a + /// wallet opting out publishes random bytes each time, but nothing stops a + /// repeat). The blob must never act as an identifier: it is opaque + /// ciphertext and must not gate admission or dedup. + #[test] + fn attack_reusing_a_blob_across_transfers_is_allowed_and_harmless() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xD9]), + commitments_of(&[0xDA]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + // Same blob, different note — must succeed: the blob carries no + // identity the chain is entitled to reason about. + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xDB]), + commitments_of(&[0xDC]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + }); + } + + /// Duplicate commitments inside ONE call, checked for real. + /// + /// The duplicate guard reads `CommitmentMemos`, which is only populated + /// AFTER each insert by `store_memo`. Within a single call the loop runs + /// insert→store_memo per output, so by the time the second (identical) + /// output is inserted the first one's memo IS stored and the guard fires. + /// If that ordering ever changes, one note would take two leaves in one + /// transaction — this pins the outcome, not the mechanism. + #[test] + fn attack_duplicate_commitments_in_one_call_cannot_take_two_leaves() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut comms: BoundedVec> = BoundedVec::new(); + comms.try_push(make_commitment(0xF1)).unwrap(); + comms.try_push(make_commitment(0xF1)).unwrap(); // same leaf twice + + let before = MerkleRepository::get_tree_size::(); + + // Run inside a storage transaction, the way a dispatchable executes: + // FRAME rolls the whole extrinsic back on error, so the partial leaf + // from the first (accepted) output must not survive. Calling + // `execute` bare would leave that write in place — an artefact of the + // test harness, not of the runtime. + let result = frame_support::storage::with_storage_layer(|| { + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0xF2, 0xF3]), + comms, + memos_of(2), + 0u32, + 0u128, + None, + 1, + test_blob(), + ) + }); + + assert!(result.is_err(), "a duplicated output must not be accepted"); + let after = MerkleRepository::get_tree_size::(); + assert_eq!( + before, after, + "the rejected call must leave no leaf behind once rolled back" + ); + }); + } + + // ── adversarial: OVK blob & sourcePk, from the CHAIN's side ────────────── + // + // Every sourcePk protection built this cycle lives in the wallet, and the + // chain cannot see it: the memo is ciphertext and the pallet has no key. + // So the question here is not "does the pallet validate sourcePk" (it + // cannot) but "what can an attacker who bypasses the SDK and submits raw + // extrinsics actually achieve". + + /// An attacker must not be able to bind a blob to SOMEONE ELSE'S note. + /// + /// The blob is bound to `commitments[0]` of the attacker's own call, and a + /// commitment can only take a leaf once, so referencing a victim's existing + /// commitment is refused outright. Without this, anyone could publish a blob + /// against another wallet's output and plant history in it. + #[test] + fn attack_cannot_bind_a_blob_to_someone_elses_commitment() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + // Victim's transfer lands first. + let victim_output = make_commitment(0x5A); + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x5B]), + commitments_of(&[0x5A]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + + // Attacker tries to publish THEIR blob against the victim's output. + let attacker_blob = OvkBlob([0xEEu8; crate::types::OVK_BLOB_SIZE]); + let result = frame_support::storage::with_storage_layer(|| { + PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x5C]), + commitments_of(&[0x5A]), // the victim's commitment + memos_of(1), + 0u32, + 0u128, + None, + 1, + attacker_blob, + ) + }); + assert!( + result.is_err(), + "reusing a live commitment must be refused, or a blob can be planted on it" + ); + + // And the victim's binding is untouched: exactly one blob event for + // that commitment, carrying the victim's bytes. + let blobs: alloc::vec::Vec<_> = frame_system::Pallet::::events() + .into_iter() + .filter_map(|r| match r.event { + crate::mock::RuntimeEvent::ShieldedPool( + PalletEvent::OutgoingBlobPublished { commitment, blob }, + ) if commitment == victim_output => Some(blob), + _ => None, + }) + .collect(); + assert_eq!( + blobs.len(), + 1, + "exactly one blob may ever bind to a commitment" + ); + assert_eq!( + blobs[0], + test_blob(), + "and it is the victim's, not the attacker's" + ); + }); + } + + /// The blob binds to `commitments[0]`, so a 2-output transfer must never + /// emit a second blob event or bind to the change output. + #[test] + fn attack_only_one_blob_event_per_transfer_bound_to_the_first_output() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x61, 0x62]), + commitments_of(&[0x63, 0x64]), + memos_of(2), + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + + let bound: alloc::vec::Vec<_> = frame_system::Pallet::::events() + .into_iter() + .filter_map(|r| match r.event { + crate::mock::RuntimeEvent::ShieldedPool( + PalletEvent::OutgoingBlobPublished { commitment, .. }, + ) => Some(commitment), + _ => None, + }) + .collect(); + assert_eq!(bound.len(), 1, "one transfer publishes exactly one blob"); + assert_eq!( + bound[0], + make_commitment(0x63), + "bound to output[0] (recipient), never to the change output" + ); + }); + } + + /// The chain must accept ANY 56 bytes as a blob — including all-zeros. + /// + /// It cannot tell a real blob from random (that is the whole point of the + /// opt-out design), so rejecting a value would be the chain pretending to + /// validate ciphertext it holds no key for. Zeros are a WALLET-side smell, + /// not a consensus rule: a pallet that rejected them would leak that the + /// distinction is observable, and would break any future blob format whose + /// encoding can legitimately be zero. + #[test] + fn attack_chain_stays_agnostic_about_blob_contents() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + for (label, blob) in [ + ("all zeros", zero_blob()), + ("all 0xFF", OvkBlob([0xFFu8; crate::types::OVK_BLOB_SIZE])), + ("real-looking", test_blob()), + ] { + let seed = label.len() as u8 + 0x70; + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[seed]), + commitments_of(&[seed + 1]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + blob, + )); + } + }); + } + + /// The memo is opaque to the chain, and must stay that way. + /// + /// `sourcePk` lives at plaintext bytes [84,116) INSIDE the ciphertext — the + /// pallet holds no key and must never gate on memo contents. This pins that: + /// two transfers whose memos differ only in those bytes are equally valid on + /// chain. A pallet that could tell them apart would mean the memo was not + /// actually encrypted. + #[test] + fn attack_memo_contents_never_gate_admission() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + // Two memos, same length, different bytes where sourcePk would sit. + let mut a = [0x01u8; MAX_ENCRYPTED_MEMO_SIZE as usize]; + let mut b = [0x01u8; MAX_ENCRYPTED_MEMO_SIZE as usize]; + for byte in a[84..116].iter_mut() { + *byte = 0x00; + } + for byte in b[84..116].iter_mut() { + *byte = 0xAB; + } + + for (i, bytes) in [a, b].into_iter().enumerate() { + let mut memos: BoundedVec> = BoundedVec::new(); + memos + .try_push(EncryptedMemo::from_bytes(&bytes).unwrap()) + .unwrap(); + let seed = 0x80 + i as u8 * 2; + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[seed]), + commitments_of(&[seed + 1]), + memos, + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + } + }); + } + + /// A transfer with NO change output still publishes a blob. + /// + /// The exact-amount spend (change = 0) is the case only OVK can recover — + /// there is no change note to infer the peer from. If the chain skipped the + /// event here, that history would be unrecoverable, and the absence would + /// itself mark which transfers were exact. + #[test] + fn attack_exact_amount_transfer_still_publishes_its_blob() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + // One input, one output: no change. + assert_ok!(PrivateTransferOperation::execute::( + proof(), + KNOWN_ROOT, + nullifiers_of(&[0x91]), + commitments_of(&[0x92]), + memos_of(1), + 0u32, + 0u128, + None, + 1, + test_blob(), + )); + + let count = frame_system::Pallet::::events() + .into_iter() + .filter(|r| { + matches!( + r.event, + crate::mock::RuntimeEvent::ShieldedPool( + PalletEvent::OutgoingBlobPublished { .. } + ) + ) + }) + .count(); + assert_eq!( + count, 1, + "an exact-amount transfer must still publish its blob" + ); + }); + } } diff --git a/frame/shielded-pool/src/operations/shield.rs b/frame/shielded-pool/src/operations/shield.rs index 3fb52bd5..1529a474 100644 --- a/frame/shielded-pool/src/operations/shield.rs +++ b/frame/shielded-pool/src/operations/shield.rs @@ -1,3 +1,28 @@ +//! `shield` — moving public tokens into the shielded pool. +//! +//! The one operation with NO proof: the depositor names the commitment their +//! note will have, and nothing constrains those bytes. Everything the other +//! calls get from the circuit has to be checked explicitly here, which makes +//! this the least guarded way into the Merkle tree. +//! +//! ## Order of steps +//! +//! Numbered below. The split matters: every validation (steps 1–3) runs BEFORE +//! the fund transfer (step 4), so a rejected shield never moves money. After +//! step 4 the operation must not fail — `?` on any later step would leave the +//! tokens in the pool account with no note to claim them. Storage is rolled +//! back on error, so the danger is not a partial write; it is that the +//! post-transfer steps are all infallible by construction. +//! +//! | # | Step | Fallible | +//! |---|-------------------------------|----------| +//! | 1 | asset is registered, verified | yes | +//! | 2 | amount and memo well-formed | yes | +//! | 3 | commitment usable and unused | yes | +//! | 4 | transfer funds into the pool | yes | +//! | 5 | insert leaf, store memo | tree-full only | +//! | 6 | emit `Shielded` | no | + use frame_support::{ pallet_prelude::*, traits::{Currency, ExistenceRequirement}, @@ -14,6 +39,8 @@ use crate::{ pub struct ShieldOperation; impl ShieldOperation { + /// Executes a shield. Steps are numbered to match the table in the module + /// header; the ordering around step 4 is a safety property, not style. pub fn execute( depositor: ::AccountId, asset_id: u32, @@ -21,21 +48,38 @@ impl ShieldOperation { commitment: Commitment, encrypted_memo: EncryptedMemo, ) -> DispatchResult { + // ── 1. Asset ───────────────────────────────────────────────────────── + // Unverified assets are refused: governance vets what may enter the pool. let asset = AssetRepository::get_asset::(asset_id).ok_or(Error::::InvalidAssetId)?; ensure!(asset.is_verified, Error::::AssetNotVerified); + + // ── 2. Amount and memo ─────────────────────────────────────────────── + // A zero-amount shield would insert a leaf and grow the tree for free. + // The memo is exact-sized, never merely bounded — the wallet slices it at + // fixed offsets, so a short one is a note nobody can open. ensure!(!amount.is_zero(), Error::::InvalidAmount); ensure!( encrypted_memo.0.len() == MAX_ENCRYPTED_MEMO_SIZE as usize, Error::::InvalidMemoSize ); + + // ── 3. Commitment ──────────────────────────────────────────────────── + // No proof constrains these bytes, so all three checks live here: + // - canonical: `n` and `n + p` reduce to the same field element but are + // DIFFERENT storage keys, so a non-canonical spelling would give one + // note two identities; + // - non-zero: zero is the tree's empty-leaf sentinel; + // - unused: one commitment may occupy at most one leaf. ensure!(commitment.is_canonical(), Error::::InvalidPublicSignals); ensure!(commitment.is_valid(), Error::::InvalidPublicSignals); - ensure!( !CommitmentRepository::exists::(&commitment), Error::::CommitmentAlreadyExists ); + // ── 4. Move the funds ──────────────────────────────────────────────── + // The point of no return: everything above rejects without touching + // money. `KeepAlive` refuses to reap the depositor's account. T::Currency::transfer( &depositor, &Pallet::::pool_account_id(), @@ -43,10 +87,16 @@ impl ShieldOperation { ExistenceRequirement::KeepAlive, )?; + // ── 5. Record the note ─────────────────────────────────────────────── + // The only way this fails is a full forest, which the `?` propagates and + // the runtime rolls back along with the transfer above. let leaf_index = MerkleTreeService::insert_leaf::(commitment)?; CommitmentRepository::store_memo::(commitment, encrypted_memo.clone()); PoolBalanceRepository::increase_balance::(asset_id, amount); + // ── 6. Announce it ─────────────────────────────────────────────────── + // `leaf_index` is what lets a scanner locate the note without walking the + // whole tree. Pallet::::deposit_event(Event::Shielded { depositor, amount, @@ -480,4 +530,50 @@ mod tests { ); assert!(one.ref_time() > zero.ref_time(), "weight must scale with n"); } + + /// A one-element batch must still be charged for the proof verification. + /// + /// The check above passes for ANY positive intercept, which is too weak. + /// `shield_batch(n)` is a line fitted over n ∈ [1,20], and where that line + /// puts its intercept is a fitting artifact rather than a measurement: the + /// fixed cost is dominated by ONE proof verification (~1s, the same for + /// every n), so a fit is free to park it in the slope instead. When it does, + /// short batches — the ones worth spamming — get under-priced, and the + /// intercept alone (41.8 ms here) is nowhere near a verification. + /// + /// So the assertion is on the total charged at n=1, not on the intercept, + /// anchored against `shield()` — which covers the same single verification + /// and single insert, so it holds wherever the fit moves the split. + /// + /// Execution time only, deliberately: `shield()` writes 34 storage entries + /// against `shield_batch(1)`'s 33, so their DB weights differ BY DESIGN and a + /// raw `ref_time()` comparison would fail on that gap instead of on the thing + /// under test. + #[test] + fn shield_batch_of_one_is_charged_for_a_proof_verification() { + use crate::weights::WeightInfo; + use frame_support::weights::constants::RocksDbWeight; + + // The DB term has to come off both sides first. It is ~3.7 Gwt against an + // execution time of ~1 Gwt, so a comparison on raw `ref_time()` is + // dominated by storage access and stays green even when the execution + // component collapses to nothing — which is precisely the regression + // this test exists to catch. + let db = |r: u64, w: u64| RocksDbWeight::get().read * r + RocksDbWeight::get().write * w; + // Read/write counts come from the benchmark header above each weight fn. + let batch_exec = <() as WeightInfo>::shield_batch(1).ref_time() - db(15 + 2, 27 + 6); + let single_exec = <() as WeightInfo>::shield().ref_time() - db(17, 34); + + // One verification either way, so they must land in the same ballpark. + // Half is a deliberately loose floor: it tolerates the spread between two + // separate benchmark runs while still failing hard if the fit has moved + // the fixed cost into the slope (which drops this by ~25×). + assert!( + batch_exec * 2 > single_exec, + "shield_batch(1) execution time ({batch_exec} ps) is under half of \ + shield() ({single_exec} ps) — both verify exactly one proof, so the \ + linear fit has pushed that fixed cost out of the intercept and into \ + the per-element slope, under-pricing short batches" + ); + } } diff --git a/frame/shielded-pool/src/operations/unshield.rs b/frame/shielded-pool/src/operations/unshield.rs index d7a443bf..a6db06ca 100644 --- a/frame/shielded-pool/src/operations/unshield.rs +++ b/frame/shielded-pool/src/operations/unshield.rs @@ -1,3 +1,38 @@ +//! `unshield` — moving shielded value back out to a public account. +//! +//! The only operation that takes value OUT of the pool, which makes it the one +//! whose failure modes lose real money rather than just privacy. Two shapes: +//! +//! - **total** — the whole note leaves, `change_commitment` is all zeros and +//! there is no change memo; +//! - **partial** — part leaves and the remainder returns as a new shielded note. +//! +//! That single flag drives most of the branching below. +//! +//! ## Order of steps +//! +//! Numbered below. Every validation (steps 1–6) runs BEFORE any state changes +//! (steps 7–10), and proof verification (step 6) runs last among the checks so a +//! transaction failing a cheap test never costs a pairing. +//! +//! | # | Step | Touches state | +//! |---|-------------------------------------|---------------| +//! | 1 | asset, amount, recipient sane | no | +//! | 2 | Merkle root known | no | +//! | 3 | nullifier canonical and unspent | no | +//! | 4 | change note consistent with the flag| no | +//! | 5 | pool solvent, fee meets the floor | no | +//! | 6 | verify the proof | no | +//! | 7 | pay the recipient | yes | +//! | 8 | accrue the relay fee, adjust balance| yes | +//! | 9 | insert the change note | yes | +//! |10 | burn the input, emit `Unshielded` | yes | +//! +//! Every check here is also enforced at pool admission +//! ([`crate::validate_unsigned::unshield`]), which may reject more but never +//! less: admission can be skipped by a malicious block author, so this is the +//! authority. + use crate::{ merkle::MerkleTreeService, pallet::{CommitmentMemos, Config, Error, Event, Pallet}, @@ -60,6 +95,10 @@ impl UnshieldOperation { relayer_evm: Option, circuit_version: u32, ) -> DispatchResult { + // ── 1. Asset, amount, recipient ────────────────────────────────────── + // The pool account is refused as a recipient: paying the pool from the + // pool would credit the tracked balance nothing while `decrease_balance` + // below still runs, silently unbacking the accounting. let asset = AssetRepository::get_asset::(asset_id).ok_or(Error::::InvalidAssetId)?; ensure!(asset.is_verified, Error::::AssetNotVerified); ensure!(!amount.is_zero(), Error::::InvalidAmount); @@ -67,17 +106,28 @@ impl UnshieldOperation { recipient != Pallet::::pool_account_id(), Error::::InvalidRecipient ); + + // ── 2. Merkle root ─────────────────────────────────────────────────── ensure!( MerkleRepository::is_known_root::(&merkle_root), Error::::UnknownMerkleRoot ); + + // ── 3. Nullifier ───────────────────────────────────────────────────── + // Canonical because `n` and `n + p` are different storage keys for one + // field element; unspent against the on-chain set. Unlike a transfer + // there is exactly one input and no dummy padding. ensure!(nullifier.is_canonical(), Error::::InvalidPublicSignals); ensure!( !NullifierRepository::is_used::(&nullifier), Error::::NullifierAlreadyUsed ); - // If a change note is present, ensure its commitment is not already in the tree. + // ── 4. Change note, consistent with the total/partial flag ─────────── + // A zero `change_commitment` means TOTAL. The two branches are mutually + // exclusive on purpose: a memo on a total unshield would describe a note + // that does not exist, and a partial one whose commitment is already in + // the tree would give one note two leaves. let has_change = change_commitment != [0u8; 32]; if has_change { let change_comm = Commitment::new(change_commitment); @@ -86,7 +136,8 @@ impl UnshieldOperation { !CommitmentRepository::exists::(&change_comm), Error::::CommitmentAlreadyExists ); - // For partial unshield, memo must be valid size (180 bytes). + // Exact size, never merely bounded — the wallet slices the memo at + // fixed offsets, so a short one is a change note nobody can open. if !change_encrypted_memo.is_empty() { ensure!( change_encrypted_memo.is_valid_size(), @@ -94,13 +145,17 @@ impl UnshieldOperation { ); } } else { - // For total unshield, memo must be empty. ensure!( change_encrypted_memo.is_empty(), Error::::InvalidMemoSize ); } + // ── 5. Pool solvency and fee floor ─────────────────────────────────── + // `amount + fee` both leave the pool's accounting, so both are required + // to be backed. The addition is CHECKED: a wrapping sum would produce a + // small total that passes the comparison and admit a spend the pool + // cannot cover. let total = amount.checked_add(&fee).ok_or(Error::::InvalidAmount)?; ensure!( PoolBalanceRepository::get_asset_balance::(asset_id) >= total, @@ -113,6 +168,11 @@ impl UnshieldOperation { let fee_u128: u128 = fee.saturated_into(); let amount_u128: u128 = amount.saturated_into(); + // ── 6. Verify the proof ────────────────────────────────────────────── + // Last and most expensive. `amount`, `recipient`, `fee` and + // `change_commitment` are all PUBLIC inputs, so the proof binds the + // payout to exactly this destination and these numbers — a relayer + // cannot redirect the funds or inflate the fee after the fact. #[cfg(not(feature = "skip-proof-verification"))] { let recipient_bytes = recipient_to_field::(&recipient)?; @@ -139,6 +199,11 @@ impl UnshieldOperation { let _ = fee_u128; } + // ── 7. Pay the recipient ───────────────────────────────────────────── + // First state change: everything above rejects without moving money. + // `AllowDeath` unlike `shield`'s `KeepAlive` — the SOURCE here is the + // pool account, which holds every shielded balance and is never at risk + // of being reaped by one payout. T::Currency::transfer( &Pallet::::pool_account_id(), &recipient, @@ -146,6 +211,10 @@ impl UnshieldOperation { ExistenceRequirement::AllowDeath, )?; + // ── 8. Accrue the fee, then adjust the tracked balance ─────────────── + // The fee is credited to the relayer's pending balance rather than + // transferred, and claimed later via `claim_shielded_fees` — which is + // what keeps the payout unlinkable from this transaction. if fee > >::Balance::zero() { let recipient_account = relayer_evm .and_then(|addr| T::Relayer::resolve_relayer(&addr)) @@ -156,11 +225,11 @@ impl UnshieldOperation { // Decrement only `amount`: the `fee` tokens stay physically in the pool as // backing for the pending relayer fee, so the tracked balance must retain - // them too. This is correct ONLY because the guard above requires + // them too. This is correct ONLY because the guard in step 5 requires // `>= amount + fee`; do not weaken it to `>= amount` or fees go unbacked. PoolBalanceRepository::decrease_balance::(asset_id, amount); - // Insert the change note commitment into the Merkle tree (partial unshield). + // ── 9. Insert the change note (partial unshield only) ──────────────── let change_leaf_index = if has_change { let change_comm = Commitment::new(change_commitment); let idx = MerkleTreeService::insert_leaf::(change_comm)?; @@ -175,6 +244,9 @@ impl UnshieldOperation { None }; + // ── 10. Burn the input and announce ────────────────────────────────── + // The event reports the change note only when there is one, so a scanner + // can tell a total unshield from a partial one without re-deriving it. let current_block = frame_system::Pallet::::block_number(); NullifierRepository::mark_as_used::(nullifier, current_block); diff --git a/frame/shielded-pool/src/types/mod.rs b/frame/shielded-pool/src/types/mod.rs index 553b8199..35a9e960 100644 --- a/frame/shielded-pool/src/types/mod.rs +++ b/frame/shielded-pool/src/types/mod.rs @@ -7,6 +7,7 @@ //! - [`note`] — the off-chain shielded note. //! - [`merkle`] — Merkle path plus the tree-depth constants. //! - [`memo`] — the fixed-size encrypted memo. +//! - [`ovk`] — the outgoing-viewing-key blob published per transfer. //! - [`asset`] — registered asset metadata. pub mod asset; @@ -14,12 +15,14 @@ pub mod ids; pub mod memo; pub mod merkle; pub mod note; +pub mod ovk; pub use asset::AssetMetadata; pub use ids::{AssetId, Commitment, Nullifier}; pub use memo::{EncryptedMemo, MAX_ENCRYPTED_MEMO_SIZE}; pub use merkle::{DEFAULT_TREE_DEPTH, DefaultMerklePath, MAX_TREE_DEPTH, MerklePath}; pub use note::Note; +pub use ovk::{OVK_BLOB_SIZE, OvkBlob}; /// A 32-byte hash used for Merkle roots, cryptographic hashes and identifiers. /// diff --git a/frame/shielded-pool/src/types/ovk.rs b/frame/shielded-pool/src/types/ovk.rs new file mode 100644 index 00000000..bb939602 --- /dev/null +++ b/frame/shielded-pool/src/types/ovk.rs @@ -0,0 +1,59 @@ +//! The outgoing-viewing-key blob published with each private transfer. +//! +//! A memo is sealed toward the RECIPIENT, so its sender cannot reopen it — a +//! sender who loses their vault loses the record of what they sent. This blob +//! is the fix: it wraps that memo's shared secret under the sender's outgoing +//! viewing key, so the sender can reach the same plaintext the recipient does. +//! +//! The chain treats it as opaque. It cannot be validated: the contents are +//! ciphertext, and nothing on chain holds the key to check them against. Only +//! the length is guaranteed, and the type is what guarantees it. + +use parity_scale_codec::{Decode, DecodeWithMemTracking, Encode, MaxEncodedLen}; +use scale_info::TypeInfo; +use sp_runtime::RuntimeDebug; + +/// Blob size: `nonce_suffix(8) + ciphertext(32) + MAC(16) = 56`. +pub const OVK_BLOB_SIZE: usize = 56; + +/// Outgoing-viewing-key blob: `nonce_suffix(8) || ciphertext(32) || MAC(16)`. +/// +/// ALWAYS present and ALWAYS 56 bytes. The fixed-size array is what enforces +/// that — a `BoundedVec` would leave the length to programmer discipline, and +/// the SCALE codec rejects anything else before the extrinsic is even decoded. +/// +/// A sender who opts out of recoverability sends 56 RANDOM bytes, not zeros. +/// Zeros would be greppable: opting out would mark the transaction forever, and +/// every user who did so would form a trivially identifiable set. Random bytes +/// are indistinguishable from a real blob, so opting out reveals nothing. +/// +/// There is deliberately no `Default`. It would produce exactly the 56 zeros +/// the design forbids, from a call that takes no arguments and reads as +/// harmless — build one with [`OvkBlob::from_bytes`] instead. +#[derive( + Clone, + PartialEq, + Eq, + Encode, + Decode, + DecodeWithMemTracking, + MaxEncodedLen, + TypeInfo, + RuntimeDebug +)] +pub struct OvkBlob(pub [u8; OVK_BLOB_SIZE]); + +impl OvkBlob { + /// Builds a blob from raw bytes, rejecting any length other than 56. + /// + /// This is the EVM route's length check: calldata carries a dynamic `bytes`, + /// so unlike the SCALE route nothing upstream has pinned the size yet. + pub fn from_bytes(bytes: &[u8]) -> Result { + let arr: [u8; OVK_BLOB_SIZE] = bytes.try_into().map_err(|_| "Invalid ovk blob size")?; + Ok(Self(arr)) + } + + pub fn as_bytes(&self) -> &[u8] { + &self.0 + } +} diff --git a/frame/shielded-pool/src/validate_unsigned/mod.rs b/frame/shielded-pool/src/validate_unsigned/mod.rs index 96417673..d25356c9 100644 --- a/frame/shielded-pool/src/validate_unsigned/mod.rs +++ b/frame/shielded-pool/src/validate_unsigned/mod.rs @@ -28,6 +28,16 @@ pub use unshield::validate_unshield; /// pass admission, propagate, and only then revert with `UnknownMerkleRoot`. pub(crate) const TX_LONGEVITY: u64 = 64; +/// ONE tag namespace for every operation that spends a note. +/// +/// A nullifier identifies a NOTE, not an operation, and the on-chain rule is +/// simply "each note is spent once" — whether by a transfer or an unshield. +/// While transfer and unshield used separate prefixes, the same note could back +/// one of each in the pool at the same time: both propagate and get revalidated +/// network-wide, only one can ever execute. Sharing the namespace makes pool +/// admission mirror the chain: one note, one entry. +pub(crate) const SPEND_TAG_PREFIX: &str = "ShieldedPoolSpend"; + #[cfg(test)] mod tests { use super::{TX_LONGEVITY, validate_private_transfer, validate_unshield}; @@ -407,11 +417,18 @@ mod tests { sp_core::H160::from([byte; 20]) } - /// Two unshield variants differing only in `relayer` produce different - /// `provides` tag sets, so a spoofed variant is a distinct pool entry and - /// cannot silently replace the honest one. + /// Two unshield variants differing only in `relayer` COLLIDE — they are the + /// same spend of the same note. + /// + /// This inverts the earlier expectation on purpose. Binding the relayer into + /// the tag made a spoofed copy a SEPARATE pool entry, so anyone could + /// rebroadcast an honest unshield pointed at their own account and have both + /// live in the pool: duplicate propagation and revalidation across the whole + /// network, for a copy that cost the attacker nothing. Keyed on the nullifier + /// alone the two are mutually exclusive, and taking the fee requires + /// out-bidding — which means actually paying it. #[test] - fn unshield_relayer_changes_provides_tag() { + fn unshield_relayer_swap_collides_with_the_original() { new_test_ext().execute_with(|| { MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); PoolBalanceRepository::set_asset_balance::(0, 1000u128); @@ -441,8 +458,14 @@ mod tests { validate_unshield::(&KNOWN_ROOT, &n, &0u32, &100u128, &10u128, &None, 1) .unwrap(); - assert_ne!(a.provides, b.provides, "different relayer → different tags"); - assert_ne!(a.provides, none.provides, "Some vs None → different tags"); + assert_eq!( + a.provides, b.provides, + "a relayer-swapped copy is the same spend and must collide" + ); + assert_eq!( + a.provides, none.provides, + "Some vs None relayer is still the same note being spent" + ); // Fee steers priority, not the relayer field. assert_eq!(a.priority, b.priority); }); @@ -481,9 +504,20 @@ mod tests { }); } - /// Same for private_transfer. + /// A relayer-swapped copy of a transfer COLLIDES with the original. + /// + /// This inverts the earlier expectation, deliberately. Binding the relayer + /// into the tag made a copy with a different fee recipient a *separate* pool + /// entry, so a third party could rebroadcast someone else's spend pointed at + /// their own account and have both sit in the pool at once — duplicate load, + /// and a race for the fee that cost the attacker nothing. + /// + /// Tagging per nullifier makes the two mutually exclusive: the higher fee + /// wins (first-seen at equal fee), so out-bidding is the only way to take + /// the fee, and out-bidding means actually paying it. The pool now mirrors + /// the on-chain rule — one note, one spend. #[test] - fn transfer_relayer_changes_provides_tag() { + fn transfer_relayer_swap_collides_with_the_original() { new_test_ext().execute_with(|| { MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); let ns = nullifiers_of(&[0x63]); @@ -494,7 +528,7 @@ mod tests { let b = validate_private_transfer::(&KNOWN_ROOT, &ns, &10u128, &Some(evm(0xBB)), 1) .unwrap(); - assert_ne!(a.provides, b.provides); + assert_eq!(a.provides, b.provides); }); } @@ -579,4 +613,265 @@ mod tests { ); }); } + + // ── adversarial: mempool tag manipulation ──────────────────────────────── + // + // The `provides` tag decides which pool entries are mutually exclusive. + // Getting it wrong is not a crash — it is censorship or fee theft: an + // attacker who can mint a colliding variant of someone else's transaction + // can displace it, and one who can mint NON-colliding variants of the same + // spend can flood the pool with entries that all spend one note. + + /// Two transactions spending the SAME note must be mutually exclusive in the + /// pool. If their tags differ, both sit in the pool and the second is dead + /// weight the node still gossips and validates. + #[test] + fn attack_same_nullifier_different_root_still_collides_in_the_pool() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + let other_root = [0x22u8; 32]; + MerkleRepository::add_historic_poseidon_root::(other_root); + + let nulls = nullifiers_of(&[0x42]); + let a = + validate_private_transfer::(&KNOWN_ROOT, &nulls, &0u128, &None, 1).unwrap(); + let b = + validate_private_transfer::(&other_root, &nulls, &0u128, &None, 1).unwrap(); + + assert_eq!( + a.provides, b.provides, + "same note spent twice must produce the same tag, whatever the root" + ); + }); + } + + /// Fee-hijack attempt: a third party rebroadcasts someone else's spend with + /// the relayer swapped to themselves. The two must be MUTUALLY EXCLUSIVE in + /// the pool (same nullifier tag) so both can never sit there at once — + /// otherwise the network carries a duplicate of every transfer. + #[test] + fn attack_swapping_the_relayer_cannot_add_a_second_pool_entry() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + let nulls = nullifiers_of(&[0x43]); + + let honest = + validate_private_transfer::(&KNOWN_ROOT, &nulls, &0u128, &None, 1).unwrap(); + let hijacked = validate_private_transfer::( + &KNOWN_ROOT, + &nulls, + &0u128, + &Some(sp_core::H160::repeat_byte(0xEE)), + 1, + ) + .unwrap(); + + assert_eq!( + honest.provides, hijacked.provides, + "a relayer-swapped copy must collide with the original, not coexist" + ); + }); + } + + /// Dummy nullifiers carry no identity. Two DIFFERENT real spends that each + /// pad with a dummy must not be forced to collide through the dummy. + #[test] + fn attack_dummy_padding_does_not_make_unrelated_spends_collide() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let mut a_nulls: BoundedVec> = BoundedVec::new(); + a_nulls.try_push(make_nullifier(0x51)).unwrap(); + a_nulls.try_push(Nullifier::new([0u8; 32])).unwrap(); + + let mut b_nulls: BoundedVec> = BoundedVec::new(); + b_nulls.try_push(make_nullifier(0x52)).unwrap(); + b_nulls.try_push(Nullifier::new([0u8; 32])).unwrap(); + + let a = + validate_private_transfer::(&KNOWN_ROOT, &a_nulls, &0u128, &None, 1).unwrap(); + let b = + validate_private_transfer::(&KNOWN_ROOT, &b_nulls, &0u128, &None, 1).unwrap(); + + assert_ne!( + a.provides, b.provides, + "unrelated spends must not collide just because both padded with a dummy" + ); + }); + } + + /// Reordering the two inputs of the SAME spend must not mint a second pool + /// entry — otherwise one note yields two admissible transactions. + #[test] + fn attack_reordering_inputs_does_not_mint_a_second_pool_entry() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let ab = nullifiers_of(&[0x61, 0x62]); + let ba = nullifiers_of(&[0x62, 0x61]); + + let a = validate_private_transfer::(&KNOWN_ROOT, &ab, &0u128, &None, 1).unwrap(); + let b = validate_private_transfer::(&KNOWN_ROOT, &ba, &0u128, &None, 1).unwrap(); + + let mut a_tags = a.provides.clone(); + let mut b_tags = b.provides.clone(); + a_tags.sort(); + b_tags.sort(); + assert_eq!( + a_tags, b_tags, + "the same pair of notes must produce the same tag set in any order" + ); + }); + } + + /// Priority is the fee. An attacker must not be able to outrank an honest + /// transaction without actually paying more. + #[test] + fn attack_priority_tracks_the_fee_and_cannot_be_forged() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + let nulls = nullifiers_of(&[0x71]); + + let cheap = + validate_private_transfer::(&KNOWN_ROOT, &nulls, &10u128, &None, 1).unwrap(); + let rich = validate_private_transfer::(&KNOWN_ROOT, &nulls, &1_000u128, &None, 1) + .unwrap(); + + assert!( + rich.priority > cheap.priority, + "a higher fee must buy higher priority, or fee bidding is broken" + ); + assert_eq!( + cheap.longevity, TX_LONGEVITY, + "longevity must not vary with fee" + ); + assert_eq!(rich.longevity, TX_LONGEVITY); + }); + } + + /// A spent note must be refused at ADMISSION, not merely at execution: + /// otherwise every node re-validates and gossips a transaction that can + /// never succeed. + #[test] + fn attack_spent_note_is_refused_at_pool_admission() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + let n = make_nullifier(0x81); + NullifierRepository::mark_as_used::(n, 1u64); + + let result = validate_private_transfer::( + &KNOWN_ROOT, + &nullifiers_of(&[0x81]), + &0u128, + &None, + 1, + ); + assert_eq!( + result.unwrap_err(), + sp_runtime::transaction_validity::TransactionValidityError::Invalid( + sp_runtime::transaction_validity::InvalidTransaction::Stale + ), + ); + }); + } + + /// THE REGRESSION THIS SUITE EXISTS FOR. + /// + /// Two transfers that share only ONE input note (A+B and A+C) must be + /// mutually exclusive: note A can back exactly one pool entry. When the tag + /// was a single blob over the whole nullifier set, these did not collide, + /// so one note could back unboundedly many admissible transactions — free + /// mempool amplification, since the fee is only charged on execution. + #[test] + fn attack_transfers_sharing_one_note_are_mutually_exclusive() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let ab = nullifiers_of(&[0x61, 0x62]); + let ac = nullifiers_of(&[0x61, 0x63]); + + let a = validate_private_transfer::(&KNOWN_ROOT, &ab, &0u128, &None, 1).unwrap(); + let b = validate_private_transfer::(&KNOWN_ROOT, &ac, &0u128, &None, 1).unwrap(); + + let shared = a.provides.iter().any(|t| b.provides.contains(t)); + assert!( + shared, + "spends sharing note A must share a tag, or A backs two pool entries" + ); + }); + } + + /// Each real nullifier contributes its OWN tag — the property every + /// exclusion guarantee above rests on. A single concatenated tag silently + /// breaks all of them, so pin the cardinality directly. + #[test] + fn attack_each_nullifier_contributes_an_independent_tag() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + + let one = validate_private_transfer::( + &KNOWN_ROOT, + &nullifiers_of(&[0x91]), + &0u128, + &None, + 1, + ) + .unwrap(); + assert_eq!(one.provides.len(), 1, "one real input → one tag"); + + let two = validate_private_transfer::( + &KNOWN_ROOT, + &nullifiers_of(&[0x92, 0x93]), + &0u128, + &None, + 1, + ) + .unwrap(); + assert_eq!( + two.provides.len(), + 2, + "two real inputs → two independent tags" + ); + + // A dummy-padded single input must still yield exactly one tag. + let mut padded: BoundedVec> = BoundedVec::new(); + padded.try_push(make_nullifier(0x94)).unwrap(); + padded.try_push(Nullifier::new([0u8; 32])).unwrap(); + let p = + validate_private_transfer::(&KNOWN_ROOT, &padded, &0u128, &None, 1).unwrap(); + assert_eq!(p.provides.len(), 1, "the dummy must not contribute a tag"); + }); + } + /// A transfer and an unshield spending the SAME note must be mutually + /// exclusive in the pool. + /// + /// They used to carry different tag prefixes, so one of each could sit in + /// the pool for a single note: both propagate and get revalidated by every + /// node, while at most one can execute. A nullifier names a NOTE, not an + /// operation, so both now share one tag namespace. + #[test] + fn attack_transfer_and_unshield_of_the_same_note_are_mutually_exclusive() { + new_test_ext().execute_with(|| { + MerkleRepository::add_historic_poseidon_root::(KNOWN_ROOT); + PoolBalanceRepository::set_asset_balance::(0, 100_000u128); + let n = make_nullifier(0x77); + + let transfer = validate_private_transfer::( + &KNOWN_ROOT, + &nullifiers_of(&[0x77]), + &10u128, + &None, + 1, + ) + .unwrap(); + let unshield = + validate_unshield::(&KNOWN_ROOT, &n, &0u32, &100u128, &10u128, &None, 1) + .unwrap(); + + assert_eq!( + transfer.provides, unshield.provides, + "one note must back one pool entry, whichever operation spends it" + ); + }); + } } diff --git a/frame/shielded-pool/src/validate_unsigned/transfer.rs b/frame/shielded-pool/src/validate_unsigned/transfer.rs index 400ae412..fc5fa895 100644 --- a/frame/shielded-pool/src/validate_unsigned/transfer.rs +++ b/frame/shielded-pool/src/validate_unsigned/transfer.rs @@ -1,8 +1,27 @@ //! Pool admission for `private_transfer`. //! -//! Checks run cheapest-first — a version lookup, a fee compare, then two point -//! reads — so flooding the pool with invalid transactions stays cheap to reject. -//! No ZK verification happens here; that is the extrinsic's job. +//! No ZK verification happens here; that is the extrinsic's job. Verifying a +//! proof at admission would let anyone burn a node's CPU for free, since +//! unsigned submissions cost nothing to make. +//! +//! ## Order of checks +//! +//! The steps below are numbered, and the order is the anti-spam property, not a +//! style choice: each step is more expensive than the last, so a junk +//! transaction is rejected as early — and as cheaply — as possible. +//! +//! | # | Check | Cost | +//! |---|------------------------|-----------------------------| +//! | 1 | circuit version | in-memory lookup | +//! | 2 | fee floor | one storage read + compare | +//! | 3 | Merkle root known | one storage read | +//! | 4 | nullifiers not spent | up to two storage reads | +//! | 5 | not all-dummy | in-memory scan | +//! | 6 | build the pool tags | no reads | +//! +//! Every check here is ALSO re-done in the dispatchable. That is deliberate: a +//! check performed only at admission could be skipped by a malicious block +//! author, so admission may reject more than execution — never less. use super::{ TX_LONGEVITY, @@ -16,7 +35,6 @@ use crate::{ use frame_support::pallet_prelude::*; use pallet_relayer::RelayerInterface as _; use pallet_zk_verifier::ZkVerifierPort as _; -use parity_scale_codec::Encode; use sp_runtime::{ SaturatedConversion, transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction}, @@ -26,61 +44,91 @@ use sp_runtime::{ /// zk-verifier's `CircuitId` constants). const CIRCUIT_TRANSFER: u32 = 1; +/// `_relayer` is intentionally unused: it is part of the call and reaches the +/// dispatchable, but it must NOT enter the pool tag. Binding it made a copy with +/// a swapped fee recipient a separate pool entry, so anyone could duplicate an +/// honest transaction at no cost. Kept in the signature so the caller in +/// `lib.rs` stays a faithful mirror of the call's fields. pub fn validate_private_transfer( merkle_root: &Hash, nullifiers: &BoundedVec>, fee: &BalanceOf, - relayer: &Option, + _relayer: &Option, circuit_version: u32, ) -> TransactionValidity { - // Anti-spam: reject an unsupported circuit version before pool admission. + // ── 1. Circuit version ─────────────────────────────────────────────────── + // Cheapest gate first: a transaction proving against a retired circuit can + // never execute, so it must not reach the pool at all. if !T::ZkVerifier::is_supported_version(CIRCUIT_TRANSFER, circuit_version) { return reject(codes::UNSUPPORTED_CIRCUIT_VERSION).into(); } - // Anti-spam: fee must meet minimum relay fee + // ── 2. Fee floor ───────────────────────────────────────────────────────── + // The pool's price of entry. Submissions are unsigned and gasless, so this + // is what stops an attacker from filling it for nothing. let min_fee: BalanceOf = T::Relayer::min_relay_fee().saturated_into(); if *fee < min_fee { return InvalidTransaction::Payment.into(); } - // Reject unknown Merkle roots + // ── 3. Merkle root ─────────────────────────────────────────────────────── + // An unknown root cannot verify, and the retention window is sized to + // outlive `TX_LONGEVITY` so a root accepted here stays valid until the + // transaction expires. if !MerkleRepository::is_known_root::(merkle_root) { return reject(codes::UNKNOWN_ROOT).into(); } - // Reject already-spent nullifiers (skip dummy nullifiers — value zero, forced by circuit) + // ── 4. Nullifiers not already spent ────────────────────────────────────── + // The dummy nullifier (all zeros) pads a single-input spend. It is never + // inserted into the set, so it can never be stale — skipping it is required, + // not an optimisation: treating it as spent would reject every one-input + // transfer after the first. for nullifier in nullifiers.iter() { if nullifier.0 == [0u8; 32] { - continue; // dummy input — never inserted in the set, cannot be stale + continue; } if NullifierSet::::contains_key(nullifier) { return InvalidTransaction::Stale.into(); } } - // Reject transactions where all nullifiers are dummy (both inputs value=0). - // This prevents free Merkle tree spam (2 commitments inserted at zero cost). + // ── 5. At least one real input ─────────────────────────────────────────── + // All-dummy means no note is being spent, so the transfer would insert two + // commitments at zero cost — free Merkle tree growth. if nullifiers.iter().all(|n| n.0 == [0u8; 32]) { return reject(codes::ALL_INPUTS_DUMMY).into(); } - // Exclude dummy nullifiers (zero) from provides — they carry no identity. - // Bind the fee recipient (`relayer`) into the tag so a variant differing only - // in `relayer` is a distinct pool entry and cannot silently replace the honest - // tx. The shared nullifier tag already makes same-nullifier variants mutually - // exclusive (first-seen wins at equal fee); this hardens that boundary. - let mut provides: alloc::vec::Vec> = nullifiers - .iter() - .filter(|n| n.0 != [0u8; 32]) - .map(|n| n.encode()) - .collect(); - provides.push(relayer.encode()); - - ValidTransaction::with_tag_prefix("ShieldedPoolTransfer") + // ── 6. Pool tags — ONE PER NULLIFIER, never one over the whole set ─────── + // + // `and_provides(x)` contributes exactly ONE tag: passing a `Vec>` + // encodes the entire vector into one blob. Doing that made the tag depend on + // the ORDER of the inputs and on the OTHER note in the pair, so: + // - reordering the two inputs minted a second admissible entry for the + // same spend, and + // - two transfers sharing only one note (A+B and A+C) did not collide at + // all, letting one note back an unbounded number of pool entries. + // Since the fee is only charged on execution, that was free mempool + // amplification: every variant propagates and is revalidated network-wide + // while at most one can ever execute. + // + // Calling `and_provides` once PER nullifier makes any two transactions that + // share a note mutually exclusive, in any order — which is what makes the + // pool mirror the on-chain nullifier set. + // + // Dummy nullifiers (zero) are excluded: they carry no identity, and tagging + // them would collide every padded single-input spend with every other. + // The `ovk_blob` deliberately stays OUT: binding unauthenticated ciphertext + // would let an attacker mint pool variants differing only in the blob. + let mut builder = ValidTransaction::with_tag_prefix(super::SPEND_TAG_PREFIX) .priority((*fee).saturated_into()) .longevity(TX_LONGEVITY) - .and_provides(provides) - .propagate(true) - .build() + .propagate(true); + + for nullifier in nullifiers.iter().filter(|n| n.0 != [0u8; 32]) { + builder = builder.and_provides(nullifier); + } + + builder.build() } diff --git a/frame/shielded-pool/src/validate_unsigned/unshield.rs b/frame/shielded-pool/src/validate_unsigned/unshield.rs index 2cda0cce..04516158 100644 --- a/frame/shielded-pool/src/validate_unsigned/unshield.rs +++ b/frame/shielded-pool/src/validate_unsigned/unshield.rs @@ -1,9 +1,32 @@ //! Pool admission for `unshield`. //! -//! Mirrors [`super::transfer`], plus a pool-solvency check. That check is -//! advisory only: the balance can move between admission and execution, so the -//! extrinsic re-verifies it. Rejecting early just avoids gossiping a spend the -//! pool cannot cover. +//! Mirrors [`super::transfer`] step for step, with one extra check: unshield is +//! the only call that moves value OUT of the pool, so it also verifies the pool +//! can cover it. +//! +//! ## Order of checks +//! +//! The steps below are numbered, and the order is the anti-spam property, not a +//! style choice: each step is more expensive than the last, so a junk +//! transaction is rejected as early — and as cheaply — as possible. Steps 1–4 +//! are identical to `transfer`; step 5 is unshield's own. +//! +//! | # | Check | Cost | +//! |---|----------------------|----------------------------| +//! | 1 | circuit version | in-memory lookup | +//! | 2 | fee floor | one storage read + compare | +//! | 3 | Merkle root known | one storage read | +//! | 4 | nullifier not spent | one storage read | +//! | 5 | pool can cover it | one storage read + add | +//! | 6 | build the pool tag | no reads | +//! +//! Step 5 is ADVISORY: the balance can move between admission and execution, so +//! the extrinsic re-verifies it. Rejecting here only avoids gossiping a spend +//! the pool visibly cannot cover. +//! +//! Every check here is ALSO re-done in the dispatchable. That is deliberate: a +//! check performed only at admission could be skipped by a malicious block +//! author, so admission may reject more than execution — never less. use super::{ TX_LONGEVITY, @@ -17,7 +40,6 @@ use crate::{ use frame_support::pallet_prelude::*; use pallet_relayer::RelayerInterface as _; use pallet_zk_verifier::ZkVerifierPort as _; -use parity_scale_codec::Encode; use sp_runtime::{ SaturatedConversion, transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction}, @@ -28,37 +50,55 @@ use sp_runtime::{ const CIRCUIT_UNSHIELD: u32 = 2; /// Validate an incoming `unshield` unsigned transaction. +/// +/// `_relayer` is intentionally unused — see the note in `transfer.rs`: the fee +/// recipient must not enter the pool tag, or a spoofed copy becomes a separate +/// pool entry instead of colliding with the original. pub fn validate_unshield( merkle_root: &Hash, nullifier: &Nullifier, asset_id: &u32, amount: &BalanceOf, fee: &BalanceOf, - relayer: &Option, + _relayer: &Option, circuit_version: u32, ) -> TransactionValidity { - // Anti-spam: reject an unsupported circuit version before pool admission. + // ── 1. Circuit version ─────────────────────────────────────────────────── + // Cheapest gate first: a transaction proving against a retired circuit can + // never execute, so it must not reach the pool at all. if !T::ZkVerifier::is_supported_version(CIRCUIT_UNSHIELD, circuit_version) { return reject(codes::UNSUPPORTED_CIRCUIT_VERSION).into(); } - // Anti-spam: fee must meet minimum relay fee + // ── 2. Fee floor ───────────────────────────────────────────────────────── + // The pool's price of entry. Submissions are unsigned and gasless, so this + // is what stops an attacker from filling it for nothing. let min_fee: BalanceOf = T::Relayer::min_relay_fee().saturated_into(); if *fee < min_fee { return InvalidTransaction::Payment.into(); } - // Reject unknown Merkle roots + // ── 3. Merkle root ─────────────────────────────────────────────────────── + // An unknown root cannot verify, and the retention window is sized to + // outlive `TX_LONGEVITY` so a root accepted here stays valid until the + // transaction expires. if !MerkleRepository::is_known_root::(merkle_root) { return reject(codes::UNKNOWN_ROOT).into(); } - // Reject already-spent nullifier + // ── 4. Nullifier not already spent ─────────────────────────────────────── + // Unlike `transfer`, unshield has exactly one input and no dummy padding — + // so there is no zero sentinel to skip here. if NullifierSet::::contains_key(nullifier) { return InvalidTransaction::Stale.into(); } - // Reject if pool balance is insufficient + // ── 5. Pool solvency (unshield only) ───────────────────────────────────── + // `amount + fee` both leave the pool, so both count against its balance. + // The addition is CHECKED: a wrapping sum would produce a small total that + // passes the comparison below, admitting a spend the pool cannot cover. + // Advisory — the balance can move before execution, so the extrinsic checks + // it again (see the module header). let total = amount .checked_add(fee) .ok_or(reject(codes::AMOUNT_OVERFLOW))?; @@ -66,14 +106,18 @@ pub fn validate_unshield( return reject(codes::INSUFFICIENT_POOL_BALANCE).into(); } - // Bind `relayer` into the tag alongside the nullifier: a variant differing only - // in the fee recipient is a distinct pool entry, so it cannot silently replace - // the honest tx. Same-nullifier variants stay mutually exclusive (first-seen - // wins at equal fee). - ValidTransaction::with_tag_prefix("ShieldedPoolUnshield") + // ── 6. Pool tag — the NULLIFIER ALONE: one note, one pool entry ────────── + // + // `relayer` used to be concatenated in, which made a copy differing only in + // the fee recipient a SEPARATE entry: anyone could rebroadcast someone + // else's unshield pointed at their own account and have both sit in the pool, + // racing for a fee the copy never paid for. Keyed on the nullifier the two + // are mutually exclusive, so taking the fee requires out-bidding — which + // means actually paying it. Mirrors `transfer.rs`. + ValidTransaction::with_tag_prefix(super::SPEND_TAG_PREFIX) .priority((*fee).saturated_into()) .longevity(TX_LONGEVITY) - .and_provides([nullifier.encode(), relayer.encode()]) + .and_provides(nullifier) .propagate(true) .build() } diff --git a/frame/shielded-pool/src/weights.rs b/frame/shielded-pool/src/weights.rs index cf135372..01004909 100644 --- a/frame/shielded-pool/src/weights.rs +++ b/frame/shielded-pool/src/weights.rs @@ -2,9 +2,9 @@ //! Autogenerated weights for pallet_shielded_pool //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 53.0.0 -//! DATE: 2026-08-07, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2026-08-11, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `ubuntu-32gb-nbg1-1`, CPU: `AMD EPYC-Genoa Processor` +//! HOSTNAME: `ubuntu-32gb-hel1-1`, CPU: `AMD EPYC-Genoa Processor` //! EXECUTION: , WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 // Executed Command: @@ -97,8 +97,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `1242` // Estimated: `3695` - // Minimum execution time: 1_021_472_000 picoseconds. - Weight::from_parts(1_027_963_000, 3695) + // Minimum execution time: 1_037_083_000 picoseconds. + Weight::from_parts(1_042_842_000, 3695) .saturating_add(T::DbWeight::get().reads(17_u64)) .saturating_add(T::DbWeight::get().writes(34_u64)) } @@ -147,10 +147,10 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `1242` // Estimated: `3631 + n * (2705 ±0)` - // Minimum execution time: 1_023_986_000 picoseconds. - Weight::from_parts(1_032_270_000, 3631) - // Standard Error: 2_054_651 - .saturating_add(Weight::from_parts(910_069_443, 0).saturating_mul(n.into())) + // Minimum execution time: 1_026_617_000 picoseconds. + Weight::from_parts(41_863_934, 3631) + // Standard Error: 752_907 + .saturating_add(Weight::from_parts(989_372_216, 0).saturating_mul(n.into())) .saturating_add(T::DbWeight::get().reads(15_u64)) .saturating_add(T::DbWeight::get().reads((2_u64).saturating_mul(n.into()))) .saturating_add(T::DbWeight::get().writes(27_u64)) @@ -206,10 +206,10 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `1260` // Estimated: `3631 + n * (2705 ±0)` - // Minimum execution time: 983_546_000 picoseconds. - Weight::from_parts(90_754_716, 3631) - // Standard Error: 2_663_348 - .saturating_add(Weight::from_parts(918_631_791, 0).saturating_mul(n.into())) + // Minimum execution time: 997_483_000 picoseconds. + Weight::from_parts(72_927_393, 3631) + // Standard Error: 1_960_935 + .saturating_add(Weight::from_parts(942_604_153, 0).saturating_mul(n.into())) .saturating_add(T::DbWeight::get().reads(17_u64)) .saturating_add(T::DbWeight::get().reads((3_u64).saturating_mul(n.into()))) .saturating_add(T::DbWeight::get().writes(29_u64)) @@ -250,8 +250,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `981` // Estimated: `6196` - // Minimum execution time: 116_957_000 picoseconds. - Weight::from_parts(119_921_000, 6196) + // Minimum execution time: 117_816_000 picoseconds. + Weight::from_parts(122_494_000, 6196) .saturating_add(T::DbWeight::get().reads(16_u64)) .saturating_add(T::DbWeight::get().writes(8_u64)) } @@ -271,8 +271,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `260` // Estimated: `3631` - // Minimum execution time: 19_179_000 picoseconds. - Weight::from_parts(20_231_000, 3631) + // Minimum execution time: 19_198_000 picoseconds. + Weight::from_parts(20_420_000, 3631) .saturating_add(T::DbWeight::get().reads(6_u64)) .saturating_add(T::DbWeight::get().writes(4_u64)) } @@ -290,7 +290,7 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `349` // Estimated: `3631` - // Minimum execution time: 18_217_000 picoseconds. + // Minimum execution time: 18_798_000 picoseconds. Weight::from_parts(19_510_000, 3631) .saturating_add(T::DbWeight::get().reads(5_u64)) .saturating_add(T::DbWeight::get().writes(3_u64)) @@ -309,8 +309,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `349` // Estimated: `3631` - // Minimum execution time: 18_318_000 picoseconds. - Weight::from_parts(19_068_000, 3631) + // Minimum execution time: 18_307_000 picoseconds. + Weight::from_parts(19_109_000, 3631) .saturating_add(T::DbWeight::get().reads(5_u64)) .saturating_add(T::DbWeight::get().writes(3_u64)) } @@ -354,8 +354,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `1200` // Estimated: `3695` - // Minimum execution time: 969_545_000 picoseconds. - Weight::from_parts(987_242_000, 3695) + // Minimum execution time: 979_506_000 picoseconds. + Weight::from_parts(987_508_000, 3695) .saturating_add(T::DbWeight::get().reads(15_u64)) .saturating_add(T::DbWeight::get().writes(33_u64)) } @@ -372,10 +372,10 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `247 + n * (48 ±0)` // Estimated: `1494 + n * (2540 ±0)` - // Minimum execution time: 211_000 picoseconds. - Weight::from_parts(250_000, 1494) - // Standard Error: 15_510 - .saturating_add(Weight::from_parts(12_680_277, 0).saturating_mul(n.into())) + // Minimum execution time: 230_000 picoseconds. + Weight::from_parts(230_000, 1494) + // Standard Error: 9_934 + .saturating_add(Weight::from_parts(12_813_771, 0).saturating_mul(n.into())) .saturating_add(T::DbWeight::get().reads(3_u64)) .saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(n.into()))) .saturating_add(T::DbWeight::get().writes(1_u64)) @@ -430,8 +430,8 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `1242` // Estimated: `3695` - // Minimum execution time: 1_021_472_000 picoseconds. - Weight::from_parts(1_027_963_000, 3695) + // Minimum execution time: 1_037_083_000 picoseconds. + Weight::from_parts(1_042_842_000, 3695) .saturating_add(RocksDbWeight::get().reads(17_u64)) .saturating_add(RocksDbWeight::get().writes(34_u64)) } @@ -480,10 +480,10 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `1242` // Estimated: `3631 + n * (2705 ±0)` - // Minimum execution time: 1_023_986_000 picoseconds. - Weight::from_parts(1_032_270_000, 3631) - // Standard Error: 2_054_651 - .saturating_add(Weight::from_parts(910_069_443, 0).saturating_mul(n.into())) + // Minimum execution time: 1_026_617_000 picoseconds. + Weight::from_parts(41_863_934, 3631) + // Standard Error: 752_907 + .saturating_add(Weight::from_parts(989_372_216, 0).saturating_mul(n.into())) .saturating_add(RocksDbWeight::get().reads(15_u64)) .saturating_add(RocksDbWeight::get().reads((2_u64).saturating_mul(n.into()))) .saturating_add(RocksDbWeight::get().writes(27_u64)) @@ -539,10 +539,10 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `1260` // Estimated: `3631 + n * (2705 ±0)` - // Minimum execution time: 983_546_000 picoseconds. - Weight::from_parts(90_754_716, 3631) - // Standard Error: 2_663_348 - .saturating_add(Weight::from_parts(918_631_791, 0).saturating_mul(n.into())) + // Minimum execution time: 997_483_000 picoseconds. + Weight::from_parts(72_927_393, 3631) + // Standard Error: 1_960_935 + .saturating_add(Weight::from_parts(942_604_153, 0).saturating_mul(n.into())) .saturating_add(RocksDbWeight::get().reads(17_u64)) .saturating_add(RocksDbWeight::get().reads((3_u64).saturating_mul(n.into()))) .saturating_add(RocksDbWeight::get().writes(29_u64)) @@ -583,8 +583,8 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `981` // Estimated: `6196` - // Minimum execution time: 116_957_000 picoseconds. - Weight::from_parts(119_921_000, 6196) + // Minimum execution time: 117_816_000 picoseconds. + Weight::from_parts(122_494_000, 6196) .saturating_add(RocksDbWeight::get().reads(16_u64)) .saturating_add(RocksDbWeight::get().writes(8_u64)) } @@ -604,8 +604,8 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `260` // Estimated: `3631` - // Minimum execution time: 19_179_000 picoseconds. - Weight::from_parts(20_231_000, 3631) + // Minimum execution time: 19_198_000 picoseconds. + Weight::from_parts(20_420_000, 3631) .saturating_add(RocksDbWeight::get().reads(6_u64)) .saturating_add(RocksDbWeight::get().writes(4_u64)) } @@ -623,7 +623,7 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `349` // Estimated: `3631` - // Minimum execution time: 18_217_000 picoseconds. + // Minimum execution time: 18_798_000 picoseconds. Weight::from_parts(19_510_000, 3631) .saturating_add(RocksDbWeight::get().reads(5_u64)) .saturating_add(RocksDbWeight::get().writes(3_u64)) @@ -642,8 +642,8 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `349` // Estimated: `3631` - // Minimum execution time: 18_318_000 picoseconds. - Weight::from_parts(19_068_000, 3631) + // Minimum execution time: 18_307_000 picoseconds. + Weight::from_parts(19_109_000, 3631) .saturating_add(RocksDbWeight::get().reads(5_u64)) .saturating_add(RocksDbWeight::get().writes(3_u64)) } @@ -687,8 +687,8 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `1200` // Estimated: `3695` - // Minimum execution time: 969_545_000 picoseconds. - Weight::from_parts(987_242_000, 3695) + // Minimum execution time: 979_506_000 picoseconds. + Weight::from_parts(987_508_000, 3695) .saturating_add(RocksDbWeight::get().reads(15_u64)) .saturating_add(RocksDbWeight::get().writes(33_u64)) } @@ -705,10 +705,10 @@ impl WeightInfo for () { // Proof Size summary in bytes: // Measured: `247 + n * (48 ±0)` // Estimated: `1494 + n * (2540 ±0)` - // Minimum execution time: 211_000 picoseconds. - Weight::from_parts(250_000, 1494) - // Standard Error: 15_510 - .saturating_add(Weight::from_parts(12_680_277, 0).saturating_mul(n.into())) + // Minimum execution time: 230_000 picoseconds. + Weight::from_parts(230_000, 1494) + // Standard Error: 9_934 + .saturating_add(Weight::from_parts(12_813_771, 0).saturating_mul(n.into())) .saturating_add(RocksDbWeight::get().reads(3_u64)) .saturating_add(RocksDbWeight::get().reads((1_u64).saturating_mul(n.into()))) .saturating_add(RocksDbWeight::get().writes(1_u64)) diff --git a/primitives/encrypted-memo/README.md b/primitives/encrypted-memo/README.md index bff2c1df..ed39b8cc 100644 --- a/primitives/encrypted-memo/README.md +++ b/primitives/encrypted-memo/README.md @@ -36,10 +36,11 @@ use orbinum_encrypted_memo::{MemoData, KeySet, encrypt_memo, decrypt_memo}; // Derive keys from master spending key let keys = KeySet::from_spending_key(spending_key); -// Create memo with counterparty (private transfer) +// With a counterparty key — only valid when it is ONE-TIME (see the field notes below) let memo = MemoData::new(1000, owner_pubkey, blinding, 0, counterparty_pk); -// Create memo without counterparty (shield / unshield) +// Without one: shield, unshield, and any transfer whose spent note had no +// one-time key to stamp let memo = MemoData::new_without_counterparty(1000, owner_pubkey, blinding, 0); // Encrypt (nonce must be unique per note) @@ -105,11 +106,11 @@ let vp = ValueProof::from_bytes(&serialized)?; ChaCha20Poly1305 AEAD with per-note key derivation: ```text -Plaintext (MemoData): value_lo(8) | value_hi(8) | owner_pk(32) | blinding(32) | asset_id(4) | counterparty_pk(32) = 116 bytes +Plaintext (MemoData): value_lo(8) | value_hi(8) | owner_pk(32) | blinding(32) | asset_id(4) | counterparty_pk(32) | circuit_version(4) = 120 bytes (value = value_lo + value_hi × 2^64, supports u128) encryption_key = SHA256(shared_secret || commitment || "orbinum-note-encryption-v1") -ciphertext = ChaCha20Poly1305(plaintext=116B, key=encryption_key, nonce=12B) +ciphertext = ChaCha20Poly1305(plaintext=120B, key=encryption_key, nonce=12B) encrypted_memo = nonce(12) | ciphertext(132) | MAC(16) | ephPk_packed(32) → 176 bytes total ``` @@ -165,13 +166,32 @@ This ensures change note commitments are **unlinkable** — they cannot be assoc | `owner_pk` | `[u8; 32]` | 32 bytes | Owner BabyJubJub public key (Ax, LE) | | `blinding` | `[u8; 32]` | 32 bytes | Blinding factor | | `asset_id` | `u32` LE | 4 bytes | Asset identifier | -| `counterparty_pk` | `[u8; 32]` | 32 bytes | Other party's Ax (LE); `[0u8;32]` for shield/unshield/change | +| `counterparty_pk` | `[u8; 32]` | 32 bytes | A **one-time** key of the other party (Ax, LE), or `[0u8;32]` — see below | +| `circuit_version` | `u32` LE | 4 bytes | ZK circuit version the note is spent under | -**Plaintext**: 116 bytes — **Encrypted wire format**: 176 bytes (`nonce(12) | ciphertext(132) | MAC(16) | ephPk_packed(32)`) +**Plaintext**: 120 bytes — **Encrypted wire format**: 180 bytes (`nonce(12) | ciphertext(132) | MAC(16) | ephPk_packed(32)`) **Value range**: u128 supporting ~340 billion tokens with 18 decimals per note -Use `MemoData::new_without_counterparty(value, owner_pk, blinding, asset_id)` for shield, unshield, and change notes. +### `counterparty_pk` — wire name vs. domain name + +This is the **frozen wire name**, kept identical here and in the TypeScript SDK's +serialisation layer so both implementations agree byte for byte. Above that boundary the +SDK calls the same field **`sourcePk`**, which describes it more honestly: + +- It is **not** an identity. What it carries is the `owner_pk` of the note that was + *spent*, and only when that key is **one-time** (it came from a stealth-addressed + transfer). Otherwise the field is zero. +- A note shielded to yourself is self-addressed with no stealth derivation, so its + `owner_pk` **is** your permanent key. Stamping that into a recipient's note would give + them a stable identifier for you, so on that path the field stays zero. + +Zero is therefore the value for shield and unshield outputs, **and** for a transfer whose +spent note had no one-time key to offer. Use +`MemoData::new_without_counterparty(value, owner_pk, blinding, asset_id)` for those. + +A transfer's **change** note is the one case that reliably carries a non-zero value: it +records the recipient's one-time stealth key, and it never leaves the sender's own wallet. ## Value Proof Public Signals diff --git a/template/runtime/RUNTIME_VERSIONS.md b/template/runtime/RUNTIME_VERSIONS.md index 67fd77cf..33734b81 100644 --- a/template/runtime/RUNTIME_VERSIONS.md +++ b/template/runtime/RUNTIME_VERSIONS.md @@ -20,6 +20,73 @@ to `spec_version` / `transaction_version` must add a row here in the same PR. The genesis reset (`69d1b837`) set `spec_version` back to 1 and `transaction_version` to 1 for the public testnet launch. +### spec 9 — tx 3 — 2026-08-10 + +Outgoing viewing keys (OVK): `private_transfer` gains an argument, so this is +the first upgrade since the reset where `transaction_version` moves. **Wallet +and runtime must ship together** — an extrinsic encoded for tx 2 no longer +decodes, and a caller on the old EVM selector is rejected as unsupported. + +**Features** + +- **shielded-pool 0.18.0 — OVK blob published per transfer** (OVK plan Fases + 4–7). A memo is sealed toward the RECIPIENT, so its sender cannot reopen it: + a sender who loses their vault loses every record of what they sent. + `private_transfer` takes a trailing `ovk_blob: OvkBlob` (fixed `[u8; 56]`) + wrapping that memo's shared secret under the sender's outgoing viewing key, + and emits `OutgoingBlobPublished { commitment, blob }` bound to + `commitments[0]` — its own event, since `CommitmentsInserted` is shared with + `shield`, `shield_batch` and `unshield`, none of which carry a blob. + + The chain treats the blob as opaque and checks only its length; it holds no + key and must not editorialize about ciphertext. So **any** 56 bytes are + accepted, zeros included. A sender opting out of recoverability publishes 56 + RANDOM bytes rather than zeros: zeros would be greppable, marking the opt-out + permanently and making everyone who chose it a trivially identifiable set. + Presence and size therefore leak nothing either way. + + `transaction_version` 2 → 3 (dispatch signature). No migration: the field is + additive and read only at dispatch, never from storage. +- **precompile 0.6.0 — `privateTransfer` ABI gains the trailing `bytes`** + (same batch). Selector `0x66ed2cd4` → `0x1ec439cf`; the ABI head grows from + 8 slots (256 B) to 9 (288 B). The decoder pins the blob at **exactly 56 + bytes** — the SCALE route gets that from the type, but calldata carries a + dynamic `bytes`, so the EVM route has to enforce it. Selectors are now + exported (`selectors::{SHIELD, PRIVATE_TRANSFER, UNSHIELD, + CLAIM_SHIELDED_FEES}`) so the relay whitelist is pinned against the decoder's + own constants in a test — the ME-8 class of drift, which fails silently + because a wrong selector is merely "unsupported". + +**Security** + +- **shielded-pool 0.18.0 — pool admission tags one entry per nullifier**, in a + namespace shared with `unshield` (`ShieldedPoolSpend`). `and_provides` + contributes exactly ONE tag, so passing it a `Vec` encoded the whole + nullifier set plus the relayer into a single blob. Three consequences, each + free for an attacker since the fee is only charged on execution: reordering + the two inputs minted a second admissible entry for the same spend; two + transfers sharing only ONE note (A+B and A+C) did not collide at all, so one + note could back unboundedly many entries; and transfer/unshield used + different prefixes, so the same note could back one of each at once. Every + variant propagates and is revalidated network-wide while at most one can + execute. + + `relayer` deliberately leaves the tag. Binding it made a copy with a swapped + fee recipient a *separate* entry, so anyone could rebroadcast another user's + spend pointed at their own account and have both sit in the pool; keyed on + the nullifier the two are mutually exclusive, so taking the fee requires + out-bidding, which means paying it. + + **Admission policy, not state transition** — consensus is unaffected, and + this is the reason `spec_version` moves for the OVK work rather than for + this. Nodes on the old logic keep accepting the duplicate variants, so the + mitigation only completes as the network updates. +- **relay RPC — `gas_price` saturates instead of panicking.** `U256::as_u128()` + panics above 2^128; the value comes from the runtime API rather than + calldata, so it is not attacker-reachable, but a panic there still takes down + the relay RPC. Now mirrors the hardening already applied to the + caller-controlled fee word. Node-side only, no consensus effect. + ### spec 8 — tx 2 — 2026-08-08 Bundles the whole audit-remediation batch plus the config/feature work that diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index e18ff9f5..4bc00b68 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -201,10 +201,10 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("orbinum"), impl_name: Cow::Borrowed("orbinum"), authoring_version: 1, - spec_version: 8, + spec_version: 9, impl_version: 1, apis: RUNTIME_API_VERSIONS, - transaction_version: 2, + transaction_version: 3, system_version: 1, }; @@ -924,9 +924,12 @@ impl_runtime_apis! { let allowed_selectors = { let stored = pallet_relayer::Pallet::::allowed_selectors(); if stored.is_empty() { + // Taken from the precompile itself rather than copied: a + // literal here that drifts from the decoder is ME-8, and it + // fails silently — a wrong selector is simply "unsupported". sp_std::vec![ - [0x47, 0xfc, 0x44, 0xa2], // unshield - [0x8c, 0x0f, 0x5d, 0x24], // privateTransfer + pallet_evm_precompile_shielded_pool::selectors::UNSHIELD, + pallet_evm_precompile_shielded_pool::selectors::PRIVATE_TRANSFER, ] } else { stored diff --git a/ts-tests/tests/test-relay-rpc.ts b/ts-tests/tests/test-relay-rpc.ts index afb92c73..6c8449c4 100644 --- a/ts-tests/tests/test-relay-rpc.ts +++ b/ts-tests/tests/test-relay-rpc.ts @@ -14,9 +14,17 @@ const MIN_RELAY_FEE = ethers.parseUnits("0.001", 18); /// EVM address derived from GENESIS_ACCOUNT_PRIVATE_KEY (lower‑case, with 0x) const RELAYER_ADDRESS = "0x6be02d1d3665660d22ff9624b7be0551ee1ac91b"; -/// Verified function selectors (keccak256 of ABI signature, first 4 bytes) -const SEL_UNSHIELD = "47fc44a2"; -const SEL_PRIVATE_TRANSFER = "8c0f5d24"; +/// Function selectors, derived below from the ABI signatures rather than +/// hardcoded. A stale copy here is exactly the ME-8 failure: the tests keep +/// passing because a wrong selector still produces "unsupported selector", so +/// the negative cases go green while the positive ones silently test nothing. +const SIG_UNSHIELD = + "unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256,bytes32,bytes,uint32)"; +const SIG_PRIVATE_TRANSFER = + "privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256,uint32,bytes)"; + +const SEL_UNSHIELD = ethers.id(SIG_UNSHIELD).slice(2, 10); +const SEL_PRIVATE_TRANSFER = ethers.id(SIG_PRIVATE_TRANSFER).slice(2, 10); // --------------------------------------------------------------------------- // Calldata builders @@ -25,15 +33,28 @@ const SEL_PRIVATE_TRANSFER = "8c0f5d24"; const abiCoder = ethers.AbiCoder.defaultAbiCoder(); /** - * Build ABI-encoded calldata for `unshield(bytes,bytes32,bytes32,uint32,uint256,bytes32,uint256)` - * with the given relay fee inserted as the 7th argument. + * Build ABI-encoded calldata for `unshield(...)` with the given relay fee. * * ABI head layout after prepending the selector: - * data[196..228] = slot 6 = uint256 fee ← the value relay.rs reads + * data[196..228] = slot 6 = uint256 fee ← the value the relay reads + * + * The head is 10 slots (320 bytes), so calldata clears the relay's 324-byte + * minimum for this op. */ function buildUnshieldCalldata(fee: bigint): string { const encoded = abiCoder.encode( - ["bytes", "bytes32", "bytes32", "uint32", "uint256", "bytes32", "uint256"], + [ + "bytes", + "bytes32", + "bytes32", + "uint32", + "uint256", + "bytes32", + "uint256", + "bytes32", + "bytes", + "uint32", + ], [ "0x" + "aa".repeat(32), // proof (32 dummy bytes) "0x" + "bb".repeat(32), // merkle root @@ -42,29 +63,46 @@ function buildUnshieldCalldata(fee: bigint): string { ethers.parseEther("1"), // amount "0x" + "00".repeat(32), // recipient (AccountId32 as bytes32) fee, // relay fee + "0x" + "00".repeat(32), // change commitment (zero = total unshield) + "0x", // change encrypted memo (empty = total unshield) + 1, // circuit version ] ); return "0x" + SEL_UNSHIELD + encoded.slice(2); } /** - * Build ABI-encoded calldata for - * `privateTransfer(bytes,bytes32,bytes32[],bytes32[],bytes[],uint32,uint256)` - * with the given relay fee as the 7th argument. + * Build ABI-encoded calldata for `privateTransfer(...)` with the given relay fee. * - * ABI head layout: data[196..228] = slot 6 = uint256 fee + * ABI head layout: data[196..228] = slot 6 = uint256 fee. The head is 9 slots + * (288 bytes), so calldata clears the relay's 292-byte minimum for this op. + * + * The trailing `bytes` is the 56-byte OVK blob; the precompile rejects any + * other length, so a placeholder here must still be exactly 56 bytes. */ function buildPrivateTransferCalldata(fee: bigint): string { const encoded = abiCoder.encode( - ["bytes", "bytes32", "bytes32[]", "bytes32[]", "bytes[]", "uint32", "uint256"], + [ + "bytes", + "bytes32", + "bytes32[]", + "bytes32[]", + "bytes[]", + "uint32", + "uint256", + "uint32", + "bytes", + ], [ "0x" + "aa".repeat(32), // proof "0x" + "bb".repeat(32), // merkle root ["0x" + "cc".repeat(32)], // nullifiers[] ["0x" + "dd".repeat(32)], // output commitments[] - ["0x" + "ee".repeat(104)], // encrypted memos[] + ["0x" + "ee".repeat(180)], // encrypted memos[] 0, // assetId fee, // relay fee + 1, // circuit version + "0x" + "0b".repeat(56), // ovk blob (exactly 56 bytes) ] ); return "0x" + SEL_PRIVATE_TRANSFER + encoded.slice(2);