From 903c82e39f309c508a39253f18d826260a006fd8 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 13:18:58 +0800 Subject: [PATCH 01/17] Security review: pin wormhole exit credits against double recording The reviewed finding (exit credits recorded twice via the event-scanning WormholeProofRecorderExtension) is not reproducible: exits are bare extrinsics, so the extension's post_dispatch never runs for them, and Unbalanced::increase_balance emits no Minted/Transfer event to scan. Add regression tests pinning both guards and document why the exit credit must stay on the event-free increase_balance path. Co-authored-by: Cursor --- pallets/wormhole/src/lib.rs | 7 +++ pallets/wormhole/src/tests.rs | 62 +++++++++++++++++++++++++++ runtime/src/transaction_extensions.rs | 57 ++++++++++++++++++++++++ 3 files changed, 126 insertions(+) diff --git a/pallets/wormhole/src/lib.rs b/pallets/wormhole/src/lib.rs index 39601d0f..ee3c65d0 100644 --- a/pallets/wormhole/src/lib.rs +++ b/pallets/wormhole/src/lib.rs @@ -890,6 +890,13 @@ pub mod pallet { for (exit_account, exit_balance) in &processed_accounts { // Skip failed credits (e.g. below ED); nullifier already marked, value // excluded from fee settlement / event / TotalWormholeExits. + // + // NOTE: this must stay `Unbalanced::increase_balance` (event-free). The runtime's + // `WormholeProofRecorderExtension` records a transfer proof for every + // `Balances::Minted` event it scans, and this exit already records its own proof + // via `record_transfer` below — switching to `mint_into` (which emits `Minted`) + // could double-record the credit. Pinned by the test + // `exit_credits_emit_no_scannable_transfer_events_and_count_once_into_pool`. match >::increase_balance( exit_account, *exit_balance, diff --git a/pallets/wormhole/src/tests.rs b/pallets/wormhole/src/tests.rs index e0424a9a..025ae666 100644 --- a/pallets/wormhole/src/tests.rs +++ b/pallets/wormhole/src/tests.rs @@ -811,6 +811,68 @@ mod private_batch_proof_tests { }); } + /// The runtime's `WormholeProofRecorderExtension` records transfer proofs by scanning + /// `Balances::Transfer`/`Minted` events after a transaction. The exit path must therefore + /// never emit either event: `process_exit_bundle` credits exits via + /// `Unbalanced::increase_balance` (event-free) and records its own proof internally via + /// `record_transfer`. If a refactor ever switched the exit credit to `mint_into` (which + /// emits `Minted`), each exit could be recorded twice — inflating `TransferCount` and + /// `PotentialWormholeBalance` and weakening the `TotalWormholeExits <= + /// PotentialWormholeBalance` soundness invariant. + #[test] + fn exit_credits_emit_no_scannable_transfer_events_and_count_once_into_pool() { + new_test_ext().execute_with(|| { + let proof = deserialize_test_proof(); + let inputs = parse_private_batch_public_inputs(&proof).expect("Should parse"); + + // Set up block state so the proof's cheap bundle checks pass. + let block_number = inputs.block_data.block_number as u64; + let block_hash_bytes: [u8; 32] = + inputs.block_data.block_hash.as_ref().try_into().unwrap(); + frame_system::BlockHash::::insert(block_number, H256::from(block_hash_bytes)); + System::set_block_number(block_number + 10); + + let seeded = 1_000_000 * UNIT; + PotentialWormholeBalance::::put(seeded); + + let expected_exit: u128 = inputs + .account_data + .iter() + .filter(|a| a.summed_output_amount > 0) + .map(|a| (a.summed_output_amount as u128) * crate::SCALE_DOWN_FACTOR) + .sum(); + assert!(expected_exit > 0, "test proof must credit at least one exit"); + + System::reset_events(); + assert_ok!(Wormhole::verify_private_batch( + RawOrigin::None.into(), + get_test_proof_bytes() + )); + + // No `Transfer`/`Minted` events: nothing for an event-based recorder to pick up. + for record in System::events() { + assert!( + !matches!( + record.event, + RuntimeEvent::Balances( + pallet_balances::Event::::Transfer { .. } | + pallet_balances::Event::::Minted { .. } + ) + ), + "exit processing must not emit scannable Transfer/Minted events: {:?}", + record.event + ); + } + + // The pallet's internal `record_transfer` credited the pool exactly once per exit. + assert_eq!( + PotentialWormholeBalance::::get(), + seeded + expected_exit, + "each exit credit must enter the potential pool exactly once" + ); + }); + } + /// Sets up the on-chain block state so the test proof's cheap bundle checks pass. fn setup_valid_block_state_for_test_proof() { let proof = deserialize_test_proof(); diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 8e3c1de7..2656b866 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -1617,6 +1617,63 @@ mod tests { }); } + // --- wormhole exit credits must not be double recorded by the event scan --- + // `process_exit_bundle` credits exits via `Unbalanced::increase_balance` (which emits no + // `Transfer`/`Minted` event) and then records the proof itself via `record_transfer`. The + // exit extrinsics are bare (unsigned), so this extension's `post_dispatch` never runs for + // them — but even if the event scan DID run over the exit's events, it must find nothing: + // otherwise every exit would be counted twice into `TransferCount` and + // `PotentialWormholeBalance`, weakening the soundness invariant. + #[test] + fn exit_credit_sequence_is_not_double_recorded_by_event_scan() { + use frame_support::traits::fungible::Unbalanced; + + new_test_ext().execute_with(|| { + System::set_block_number(1); + + let exit_account = intg_account(90); + let amount = 100 * UNIT; + pallet_wormhole::PotentialWormholeBalance::::put(POOL_BASE); + + let events_before = frame_system::Pallet::::event_count(); + + // Replicate exactly what `process_exit_bundle` does for a successful exit credit. + assert_ok!(>::increase_balance( + &exit_account, + amount, + frame_support::traits::tokens::Precision::Exact, + )); + pallet_wormhole::Pallet::::record_transfer( + AssetId::default(), + &crate::configs::MintingAccount::get(), + &exit_account, + amount, + ); + + // The pallet's own recording applied exactly once. + assert_eq!(Wormhole::transfer_count(&exit_account), 1); + assert_eq!(pool(), POOL_BASE + amount); + + // A hypothetical event scan over the exit's events must record nothing extra: + // `increase_balance` emits no `Transfer`/`Minted` event to pick up. + let recorded = + WormholeProofRecorderExtension::::record_proofs_from_events_since( + events_before, + ); + assert_eq!(recorded, 0, "the event scan must not re-record the exit credit"); + assert_eq!( + Wormhole::transfer_count(&exit_account), + 1, + "exit credit must be recorded exactly once" + ); + assert_eq!( + pool(), + POOL_BASE + amount, + "the potential pool must not be double credited for an exit" + ); + }); + } + // --- mined block rewards flow into the potential pool --- // Mining mints brand-new coins to the miner (ambiguous, never-signed) and the treasury (a // keyless governance account, excluded via `NonWormholeAccounts`). Only the miner's portion is From 86612f342183e623396d9760ae320af778200df9 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 13:49:44 +0800 Subject: [PATCH 02/17] Derive wormhole genesis proofs from balances Remove the independent endowed_addresses genesis vector and GenesisEndowmentsPending staging. At block 1, record transfer proofs from every account that exists with a balance, so exit capacity cannot disagree with actually issued genesis value. Co-authored-by: Cursor --- pallets/wormhole/src/lib.rs | 107 ++++++++------------------ pallets/wormhole/src/mock.rs | 20 ++--- pallets/wormhole/src/tests.rs | 64 +++++++++++++++ runtime/src/genesis_config_presets.rs | 40 +++++++--- 4 files changed, 135 insertions(+), 96 deletions(-) diff --git a/pallets/wormhole/src/lib.rs b/pallets/wormhole/src/lib.rs index ee3c65d0..b17e5bf1 100644 --- a/pallets/wormhole/src/lib.rs +++ b/pallets/wormhole/src/lib.rs @@ -207,7 +207,7 @@ pub mod pallet { pallet_prelude::*, traits::{ fungible::{Inspect as FungibleInspect, Mutate, Unbalanced}, - BuildGenesisConfig, Contains, Currency, + Contains, Currency, }, }; use frame_system::pallet_prelude::*; @@ -244,54 +244,6 @@ pub mod pallet { #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); - /// Genesis configuration for recording transfer proofs. - /// - /// This allows addresses to be endowed at genesis with funds that can be spent - /// using ZK proofs. The endowments are stored during genesis and processed in - /// `on_initialize` at block 1, which calls `record_transfer` for each address. - /// This records both the TransferProof in storage AND emits NativeTransferred events. - /// - /// We defer to block 1 because events emitted during genesis_build are not - /// persisted (Substrate limitation). By processing at block 1, indexers like - /// Subsquid can track these transfers. - /// - /// The chain does not distinguish between "wormhole addresses" and regular addresses - - /// any address can have transfer proofs recorded and spend via ZK proofs. - /// - /// Note: The actual balance must also be set via BalancesConfig separately. - #[pallet::genesis_config] - #[derive(frame_support::DefaultNoBound)] - pub struct GenesisConfig { - /// Addresses to record transfer proofs for at genesis: (address, amount). - /// A TransferProof will be recorded for each, enabling ZK spending. - /// Uses u128 for serde compatibility; converted to BalanceOf at build time. - pub endowed_addresses: Vec<(T::WormholeAccountId, u128)>, - } - - #[pallet::genesis_build] - impl BuildGenesisConfig for GenesisConfig { - fn build(&self) { - // Store endowments to be processed in on_initialize at block 1. - // We can't call record_transfer here because events emitted during - // genesis_build are not persisted (Substrate limitation). - // By deferring to block 1, both storage and events are handled correctly. - let pending: Vec<(T::WormholeAccountId, BalanceOf)> = self - .endowed_addresses - .iter() - .map(|(to, amount)| { - let balance: BalanceOf = (*amount).try_into().unwrap_or_else(|_| { - panic!("Genesis endowment amount {} exceeds Balance capacity", amount) - }); - (to.clone(), balance) - }) - .collect(); - - if !pending.is_empty() { - GenesisEndowmentsPending::::put(pending); - } - } - } - #[pallet::config] pub trait Config: frame_system::Config { /// Native balance type for transfer proofs. @@ -412,17 +364,6 @@ pub mod pallet { pub type TransferCount = StorageMap<_, Blake2_128Concat, T::WormholeAccountId, T::TransferCount, ValueQuery>; - /// Genesis endowments pending event emission. - /// Stores (to_address, amount) for each genesis endowment. - /// These are processed in on_initialize at block 1 to emit NativeTransferred events, - /// then cleared. This ensures indexers like Subsquid can track genesis transfers. - /// - /// Unbounded because it's only populated at genesis and cleared on block 1. - #[pallet::storage] - #[pallet::unbounded] - pub type GenesisEndowmentsPending = - StorageValue<_, Vec<(T::WormholeAccountId, BalanceOf)>, ValueQuery>; - /// Sum of balances held by "ambiguous" addresses (accounts that have never signed a /// dilithium transaction, i.e. `nonce == 0`). These addresses are indistinguishable from /// wormhole deposit addresses, so this is the maximum value that could legitimately be @@ -530,31 +471,51 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { - /// On block 1, process all genesis endowments by calling record_transfer. - /// This records transfer proofs and emits NativeTransferred events. - /// We defer this from genesis_build because events emitted during genesis - /// are not persisted (Substrate limitation). + /// On block 1, record a transfer proof for every account that exists with a + /// balance — i.e. exactly the genesis balances. + /// + /// The genesis state is the single source of truth: proofs are *derived* from the + /// balances actually issued (there is no separate endowment list that could + /// disagree with them), so an exitable leaf or `PotentialWormholeBalance` credit + /// that isn't backed by real issuance is unrepresentable. This runs before any + /// extrinsic has ever executed, so the account set observed here is precisely the + /// genesis set. + /// + /// This also seeds the soundness pool consistently: the reveal logic subtracts a + /// first-time signer's whole balance from the pool on the assumption that every + /// credit it received was counted in, which now holds for genesis balances too. + /// + /// We do this at block 1 rather than in a genesis build because events emitted + /// during genesis are not persisted (Substrate limitation); recording here emits + /// `NativeTransferred` events that indexers like Subsquid can track. fn on_initialize(n: BlockNumberFor) -> Weight { // Only process on block 1 if n != One::one() { return Weight::zero(); } - let pending = GenesisEndowmentsPending::::take(); - if pending.is_empty() { - return Weight::zero(); - } - let minting_account: T::WormholeAccountId = T::MintingAccount::get().into(); - let num_endowments = pending.len() as u64; + let mut accounts_seen = 0u64; + let mut recorded = 0u64; - for (to, amount) in pending { + for who in frame_system::Account::::iter_keys() { + accounts_seen = accounts_seen.saturating_add(1); + let amount = >::total_balance(&who); + if amount.is_zero() { + continue; + } + let to: T::WormholeAccountId = who.into(); // Record transfer proof and emit event Self::record_transfer(T::AssetId::default(), &minting_account, &to, amount); + recorded = recorded.saturating_add(1); } - // Weight: 1 read (take pending) + N * (2 reads + 2 writes + 1 event) per endowment - T::DbWeight::get().reads_writes(1 + num_endowments * 2, num_endowments * 2) + // Weight: 1 read per iterated account + N * (2 reads + 2 writes + 1 event) + // per recorded proof + T::DbWeight::get().reads_writes( + accounts_seen.saturating_add(recorded.saturating_mul(2)), + recorded.saturating_mul(2), + ) } } diff --git a/pallets/wormhole/src/mock.rs b/pallets/wormhole/src/mock.rs index c8940f4c..161b437e 100644 --- a/pallets/wormhole/src/mock.rs +++ b/pallets/wormhole/src/mock.rs @@ -148,13 +148,12 @@ pub fn new_test_ext() -> sp_state_machine::TestExternalities { t.into() } -/// Build test externalities with genesis endowments. -/// Each endowment is (address, amount) and will have both balance and TransferProof recorded -/// (after block 1 initialization), enabling the address to spend via ZK proofs. +/// Build test externalities with genesis balance endowments. /// -/// Note: This sets up the genesis state, but TransferProofs are recorded in on_initialize -/// at block 1. Tests should call `System::set_block_number(1)` and then trigger -/// `Wormhole::on_initialize(1)` to process the endowments. +/// TransferProofs are *derived* from these balances in `on_initialize` at block 1 (the +/// wormhole pallet records a proof for every account existing with a balance), enabling +/// each address to spend via ZK proofs. Tests should call `System::set_block_number(1)` +/// and then trigger `Wormhole::on_initialize(1)` to process them. pub fn new_test_ext_with_endowments( endowments: Vec<(AccountId, Balance)>, ) -> sp_state_machine::TestExternalities { @@ -162,13 +161,8 @@ pub fn new_test_ext_with_endowments( let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); - // Set up balances for the endowed accounts - pallet_balances::GenesisConfig:: { balances: endowments.to_vec(), dev_accounts: None } - .assimilate_storage(&mut t) - .unwrap(); - - // Set up endowments to be processed at block 1 - pallet_wormhole::GenesisConfig:: { endowed_addresses: endowments } + // Set up balances for the endowed accounts; wormhole proofs derive from these. + pallet_balances::GenesisConfig:: { balances: endowments, dev_accounts: None } .assimilate_storage(&mut t) .unwrap(); diff --git a/pallets/wormhole/src/tests.rs b/pallets/wormhole/src/tests.rs index 025ae666..1ff559a5 100644 --- a/pallets/wormhole/src/tests.rs +++ b/pallets/wormhole/src/tests.rs @@ -468,6 +468,70 @@ mod wormhole_tests { }); } + // ========================================================================= + // Genesis proofs are derived from real balances (single source of truth) + // ========================================================================= + // + // There is no separate wormhole endowment list at genesis: `on_initialize(1)` derives + // a transfer proof from every account that exists with a balance. An exitable leaf or + // `PotentialWormholeBalance` credit that isn't backed by actually-issued value is + // therefore unrepresentable — the leaf amount IS the genesis balance. + + #[test] + fn genesis_proofs_derive_from_balances_and_seed_pool() { + use frame_support::traits::Hooks; + + let addr1 = account_id(100); + let addr2 = account_id(101); + let amount1 = 100 * UNIT; + let amount2 = 250 * UNIT; + + new_test_ext_with_endowments(vec![(addr1.clone(), amount1), (addr2.clone(), amount2)]) + .execute_with(|| { + System::set_block_number(1); + Wormhole::on_initialize(1); + + // One leaf per funded genesis account, amount = the real balance. + assert_eq!(Wormhole::transfer_count(&addr1), 1); + assert_eq!(Wormhole::transfer_count(&addr2), 1); + + // The soundness pool is seeded with exactly the ambiguous genesis + // balances — consistent with what a later reveal would subtract. + assert_eq!( + crate::PotentialWormholeBalance::::get(), + amount1 + amount2, + "pool must equal the total of ambiguous genesis balances" + ); + }); + } + + #[test] + fn genesis_proofs_exclude_non_wormhole_accounts_from_pool() { + use frame_support::traits::Hooks; + + // An excluded (`NonWormholeAccounts`) genesis account still gets an inert leaf, + // but must not count into the soundness pool. + let excluded = excluded_account(); + let regular = account_id(100); + + new_test_ext_with_endowments(vec![ + (excluded.clone(), 100 * UNIT), + (regular.clone(), 40 * UNIT), + ]) + .execute_with(|| { + System::set_block_number(1); + Wormhole::on_initialize(1); + + assert_eq!(Wormhole::transfer_count(&excluded), 1); + assert_eq!(Wormhole::transfer_count(®ular), 1); + assert_eq!( + crate::PotentialWormholeBalance::::get(), + 40 * UNIT, + "excluded accounts must not seed the pool" + ); + }); + } + // ========================================================================= // Soundness counter tracking // ========================================================================= diff --git a/runtime/src/genesis_config_presets.rs b/runtime/src/genesis_config_presets.rs index 2bc94376..c9676ad1 100644 --- a/runtime/src/genesis_config_presets.rs +++ b/runtime/src/genesis_config_presets.rs @@ -148,10 +148,11 @@ fn planck_tech_collective_seed() -> Vec { /// Returns the genesis config populated with given parameters. Treasury is per-profile. /// -/// All endowed addresses automatically get transfer proofs recorded, enabling them to -/// spend their funds via ZK proofs. The chain doesn't distinguish between "wormhole -/// addresses" and regular addresses - any address can spend via ZK proofs if they -/// know the corresponding secret. +/// All endowed addresses automatically get transfer proofs recorded at block 1 (the +/// wormhole pallet derives them from the genesis balances — there is no separate +/// endowment list), enabling them to spend their funds via ZK proofs. The chain doesn't +/// distinguish between "wormhole addresses" and regular addresses - any address can +/// spend via ZK proofs if they know the corresponding secret. fn genesis_template( endowed_accounts: Vec, treasury: TreasuryGenesis, @@ -170,16 +171,11 @@ fn genesis_template( // mining rewards. It is intentionally NOT added to `balances`. let config = RuntimeGenesisConfig { - balances: BalancesConfig { balances: balances.clone(), dev_accounts: None }, + balances: BalancesConfig { balances, dev_accounts: None }, treasury_pallet: pallet_treasury::GenesisConfig:: { treasury_account: Some(treasury.account), treasury_portion: Some(treasury.portion), }, - wormhole: pallet_wormhole::GenesisConfig:: { - // Record transfer proofs for ALL endowed addresses, enabling ZK spending. - // Events are emitted in on_initialize at block 1 for indexer compatibility. - endowed_addresses: balances, - }, ..Default::default() }; @@ -425,3 +421,27 @@ pub fn preset_names() -> Vec { PresetId::from(PLANCK_RUNTIME_PRESET), ] } + +#[cfg(test)] +mod tests { + use super::*; + use sp_runtime::BuildStorage; + + /// Every shipped preset must actually build genesis storage, i.e. pass every pallet's + /// genesis-build invariants. (Wormhole transfer proofs need no preset entry at all: + /// they are derived from these genesis balances at block 1, so they cannot disagree + /// with the value actually issued.) + #[test] + fn all_presets_build_genesis_storage() { + for id in preset_names() { + let bytes = get_preset(&id).expect("listed preset must resolve"); + let (config_bytes, _members) = prepare_genesis_build_input(bytes) + .unwrap_or_else(|e| panic!("preset {:?}: invalid genesis JSON: {e}", id)); + let config: crate::RuntimeGenesisConfig = serde_json::from_slice(&config_bytes) + .unwrap_or_else(|e| panic!("preset {:?} must deserialize: {e}", id)); + config + .build_storage() + .unwrap_or_else(|e| panic!("preset {:?} must build genesis storage: {e:?}", id)); + } + } +} From ca9da896f7e1a24466686913ded86139b2d20be5 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:00:19 +0800 Subject: [PATCH 03/17] Security review: pin aggregator rebate address binding for public batches The aggregator rebate is deliberately permissionless (whoever aggregates names its own payout address), which is safe because the address is a proof public input: redirecting it invalidates the proof at the pre_dispatch block-inclusion gate. Add a regression test pinning that property. Co-authored-by: Cursor --- pallets/wormhole/src/tests.rs | 62 +++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/pallets/wormhole/src/tests.rs b/pallets/wormhole/src/tests.rs index 1ff559a5..63529ee0 100644 --- a/pallets/wormhole/src/tests.rs +++ b/pallets/wormhole/src/tests.rs @@ -2181,6 +2181,68 @@ mod public_batch_proof_tests { }); } + /// The aggregator rebate is deliberately permissionless: whoever performs the public-batch + /// aggregation names its own payout address as a proof public input. The property that + /// makes this safe is that the address is *bound* by the proof — a third party cannot take + /// someone else's public batch and redirect the rebate to itself, because mutating the + /// aggregator-address public inputs invalidates the proof, and `pre_dispatch` (the + /// block-inclusion gate) runs full ZK verification. + #[test] + fn pre_dispatch_rejects_public_batch_with_redirected_aggregator_address() { + use frame_support::pallet_prelude::ValidateUnsigned; + use qp_plonky2_verifier::field::types::Field; + + new_test_ext().execute_with(|| { + let inputs = parse_test_inputs(); + setup_matching_block_state(&inputs); + + // The genuine proof passes the block-inclusion gate. + let original = get_test_proof_bytes(); + let call = crate::Call::::verify_public_batch { proof_bytes: original.clone() }; + assert!( + ::pre_dispatch(&call).is_ok(), + "the untampered fixture must pass pre_dispatch" + ); + + // An attacker rewrites the aggregator-address public inputs (the first 4 felts + // of the public-batch PI layout) to point at an account they control. + let mut tampered_proof = deserialize_test_proof(); + for felt in tampered_proof.public_inputs.iter_mut().take(4) { + *felt = F::from_canonical_u32(0x42); + } + let tampered_bytes = tampered_proof.to_bytes(); + assert_ne!(tampered_bytes, original, "mutation must change the encoded proof"); + + // The redirected address round-trips through parsing (i.e. the tampering is + // well-formed at the PI level) ... + let tampered_deser = ProofWithPublicInputs::::from_bytes( + tampered_bytes.clone(), + &crate::get_public_batch_verifier().unwrap().circuit_data.common, + ) + .expect("tampered PIs still deserialize"); + let tampered_inputs = parse_public_batch_public_inputs( + &tampered_deser, + crate::circuit_config::NUM_PRIVATE_BATCH_PROOFS, + crate::circuit_config::NUM_LEAF_PROOFS, + ) + .expect("tampered PIs still parse"); + assert_ne!( + tampered_inputs.aggregator_address.as_ref(), + &AGGREGATOR_ADDRESS, + "the payout address was redirected" + ); + + // ... but the proof no longer verifies, so the block-inclusion gate rejects it: + // the rebate cannot be stolen off an existing proof. + let tampered_call = + crate::Call::::verify_public_batch { proof_bytes: tampered_bytes }; + assert!( + ::pre_dispatch(&tampered_call).is_err(), + "pre_dispatch must reject a proof whose aggregator address was redirected" + ); + }); + } + /// Regenerate the public-batch test fixture when circuit parameters change. /// /// Run with: cargo test -p pallet-wormhole --release --lib -- From b570b39173326adc4ab5af0117842e91f90d1d18 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:03:48 +0800 Subject: [PATCH 04/17] Document the circuit tree-depth limit and planned update path The on-chain zk-tree can grow to depth 32 but the wormhole circuits accept Merkle paths only up to depth 16 (~4.3B leaves). Document that this is a deliberate proving-cost trade-off, the timeline to exhaustion (years to centuries depending on transfer volume), and the circuit update + runtime upgrade planned when the limit approaches. Also fixes the MAX_TREE_DEPTH comment that wrongly claimed circuits support depth 32. Co-authored-by: Cursor --- docs/zk-trie-architecture.md | 38 +++++++++++++++++++++++++++++++++++- pallets/zk-tree/src/lib.rs | 27 +++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/docs/zk-trie-architecture.md b/docs/zk-trie-architecture.md index 980c9ace..ddb070bb 100644 --- a/docs/zk-trie-architecture.md +++ b/docs/zk-trie-architecture.md @@ -63,10 +63,46 @@ Depth 3 (capacity: 64 leaves) | 2 | 16 | Grows automatically | | 3 | 64 | | | ... | ... | | -| 32 | ~1.8 × 10^19 | Maximum supported depth | +| 16 | ~4.3 × 10^9 | Max depth the **circuits** accept (see below) | +| 32 | ~1.8 × 10^19 | Max depth the on-chain tree may grow to | The tree grows dynamically -- when the 5th leaf arrives, depth increases from 1 to 2. The old root becomes child[0] of a new root node. +### Circuit depth limit (known, accepted limitation) + +The on-chain tree may grow up to depth 32 (`MAX_TREE_DEPTH` in `pallets/zk-tree`), but the +wormhole circuits only accept Merkle paths up to depth 16 (`MAX_DEPTH` in +`qp-zk-circuits-common/src/zk_merkle.rs`). The circuit pads every proof's witness to the +full `MAX_DEPTH` levels, so **every leaf proof pays the proving cost of a depth-16 path +regardless of the tree's actual depth** -- that is why the circuit constant is kept as +small as safely possible instead of matching the on-chain cap. + +**What happens at the limit:** once leaf 4^16 + 1 (~4.3 billion) is inserted, the tree +grows to depth 17, all Merkle proofs gain a 17th sibling level, and the prover and +on-chain verifier reject them. Existing funds are never lost and nullifier state is +untouched -- wormhole proof *generation* simply halts until the circuit is updated. + +**The plan is to do a circuit update when (long before) that happens.** Rough timeline +to exhaustion at 12-second blocks: + +| Sustained leaf rate | Time to 4.3 B leaves | +|---|---| +| 1 leaf/block (mining-reward floor) | ~1,600 years | +| 10 transfers/sec chain-wide | ~13 years | +| ~50 transfers/sec (permanently full blocks) | ~2.5 years | + +`LeafCount` is public storage, so the approach is observable years ahead; each +1 of +circuit depth quadruples capacity (e.g. 16 → 20 buys ~256× the runway). + +**What the update involves:** bump `MAX_DEPTH` in `qp-zk-circuits-common`, release the +circuit crates, rebuild -- `pallets/wormhole/build.rs` regenerates and embeds the new +verifier binaries automatically -- regenerate the proof test fixtures +(`regenerate_*_fixture` tests), re-benchmark weights, and ship a normal runtime upgrade. +The code change is a one-line constant; the end-to-end effort is on the order of days of +engineering inside a standard release cycle. Proofs built against the old circuit become +invalid at the upgrade (wallets/provers must update in step), but spent nullifiers +persist, so nothing can double-spend across the transition. + ### Hashing Strategy | Layer | Encoding | Felts | Injective? | diff --git a/pallets/zk-tree/src/lib.rs b/pallets/zk-tree/src/lib.rs index c709f306..7169cfea 100644 --- a/pallets/zk-tree/src/lib.rs +++ b/pallets/zk-tree/src/lib.rs @@ -37,8 +37,31 @@ pub mod tree; #[cfg(test)] mod tests; -/// Maximum depth supported by ZK circuits. -/// A tree of depth 32 can hold 4^32 leaves (more than enough). +/// Maximum depth the on-chain tree may grow to (weight-metering / growth cap). +/// A tree of depth 32 can hold 4^32 leaves. +/// +/// NOTE (known, accepted limitation): this is intentionally *larger* than the depth the +/// wormhole circuits accept. The circuits fix `MAX_DEPTH = 16` (`qp-zk-circuits-common`, +/// `zk_merkle.rs`) because every leaf proof pays the proving cost of a full +/// `MAX_DEPTH`-level Merkle path regardless of the tree's current depth — keeping it at +/// 16 keeps proving fast for everyone. If the tree ever grows past depth 16 +/// (4^16 ≈ 4.3 billion leaves), Merkle proofs gain a 17th sibling level and the prover +/// and verifier reject them, so wormhole proof generation halts until a circuit update +/// raises `MAX_DEPTH` and a runtime upgrade embeds the regenerated verifiers. +/// +/// This is a deliberate "fix it when we get close" trade-off, not an oversight: +/// - Timeline: at one leaf per block (the mining-reward floor, 12s blocks) depth 16 +/// lasts ~1,600 years; at a sustained 10 transfers/sec chain-wide it lasts ~13 years; +/// even permanently saturated blocks (~50 tps) give ~2.5 years. Each +1 of circuit +/// depth quadruples capacity. +/// - Observability: `LeafCount` is public storage, so exhaustion is visible years in +/// advance; alert well before 4^16 leaves. +/// - The update itself: bump `MAX_DEPTH` in `qp-zk-circuits-common`, release the +/// circuit crates, let `pallets/wormhole/build.rs` regenerate the embedded verifier +/// binaries, regenerate proof fixtures, re-benchmark, and ship a runtime upgrade — +/// days of engineering inside a normal release cycle. Old proofs are invalidated by +/// the circuit change; nullifier state is unaffected, so nothing can double-spend +/// across the upgrade. pub const MAX_TREE_DEPTH: u8 = 32; /// Worst-case `(reads, writes)` storage-operation counts for one [`Pallet::insert_leaf`] From 80802efadc737df91082b6e5ae0339dc8d5540fe Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:15:38 +0800 Subject: [PATCH 05/17] Security review: charge ZK-tree Poseidon hashing in leaf-recording weights The wormhole settlement weights, the transfer-proof extension's per_transfer_weight, and reversible-transfers' execute_transfer charged the leaf insert's DB ops but not its depth-dependent Poseidon hashing (one hash per tree level), under-declaring execution work per recorded transfer. Add a live-depth insert_leaf_hash_ref_time helper to pallet-zk-tree (mirroring insert_leaf_db_ops) and charge it on all three routes, pinned by depth-sensitivity tests. Co-authored-by: Cursor --- pallets/reversible-transfers/src/weights.rs | 35 ++++- pallets/wormhole/src/weights.rs | 150 +++++++++++++++++--- pallets/zk-tree/src/lib.rs | 9 ++ runtime/src/transaction_extensions.rs | 33 ++++- 4 files changed, 199 insertions(+), 28 deletions(-) diff --git a/pallets/reversible-transfers/src/weights.rs b/pallets/reversible-transfers/src/weights.rs index 1d6f3cd4..a0992be4 100644 --- a/pallets/reversible-transfers/src/weights.rs +++ b/pallets/reversible-transfers/src/weights.rs @@ -66,10 +66,15 @@ const EXECUTE_TRANSFER_BASE_WRITES: u64 = 5; /// `execute_transfer`'s weight: the benchmarked base (compute + non-tree storage) /// plus the depth-dependent ZK-tree leaf insert performed by the wormhole proof /// recorder. `insert_leaf` walks the tree leaf-to-root, so DB ops and PoV scale -/// with `tree_ops` via [`pallet_zk_tree::TREE_KEY_POV`]. -fn execute_transfer_weight(db: RuntimeDbWeight, (tree_reads, tree_writes): (u64, u64)) -> Weight { +/// with `tree_ops` via [`pallet_zk_tree::TREE_KEY_POV`], and the path update also +/// computes one Poseidon hash per level (`tree_hash_time`). +fn execute_transfer_weight( + db: RuntimeDbWeight, + (tree_reads, tree_writes): (u64, u64), + tree_hash_time: u64, +) -> Weight { // Minimum execution time: 105_000_000 picoseconds. - Weight::from_parts(110_000_000, 8619) + Weight::from_parts(110_000_000_u64.saturating_add(tree_hash_time), 8619) .saturating_add(Weight::from_parts( 0, tree_reads.saturating_mul(pallet_zk_tree::TREE_KEY_POV), @@ -178,6 +183,7 @@ impl WeightInfo for SubstrateW execute_transfer_weight( T::DbWeight::get(), pallet_zk_tree::Pallet::::insert_leaf_db_ops(), + pallet_zk_tree::Pallet::::insert_leaf_hash_ref_time(), ) } /// Storage: `ReversibleTransfers::HighSecurityAccounts` (r:1 w:0) @@ -311,6 +317,7 @@ impl WeightInfo for () { execute_transfer_weight( RocksDbWeight::get(), pallet_zk_tree::insert_leaf_db_ops_at_depth(pallet_zk_tree::MAX_TREE_DEPTH), + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(pallet_zk_tree::MAX_TREE_DEPTH), ) } /// Storage: `ReversibleTransfers::HighSecurityAccounts` (r:1 w:0) @@ -383,4 +390,26 @@ mod tests { ); }); } + + /// The leaf insert also computes one Poseidon hash per tree level; that compute + /// must be charged in `ref_time` on top of the DB ops. The mock's `DbWeight` is + /// zero, so any depth-driven `ref_time` growth must come from the hashing term. + #[test] + fn execute_transfer_ref_time_includes_tree_hash_compute() { + crate::tests::mock::new_test_ext().execute_with(|| { + type W = SubstrateWeight; + pallet_zk_tree::Depth::::put(1); + let shallow = W::execute_transfer(); + pallet_zk_tree::Depth::::put(pallet_zk_tree::MAX_TREE_DEPTH); + let deep = W::execute_transfer(); + assert!( + deep.ref_time() > + shallow.ref_time(), + "execute_transfer ref_time must grow with tree depth (Poseidon hashing per level); \ + shallow: {:?}, deep: {:?}", + shallow, + deep, + ); + }); + } } diff --git a/pallets/wormhole/src/weights.rs b/pallets/wormhole/src/weights.rs index 609051d7..7f815b66 100644 --- a/pallets/wormhole/src/weights.rs +++ b/pallets/wormhole/src/weights.rs @@ -163,27 +163,38 @@ impl WeightInfo for SubstrateW .saturating_add(T::DbWeight::get().reads(1_u64.saturating_add(nullifier_reads))) } /// Inclusion path: ZK verify + pre-validation twice (`pre_dispatch` and dispatch - /// body), each charged in full (compute + DB + PoV), plus exit-processing storage. + /// body), each charged in full (compute + DB + PoV), plus exit-processing storage + /// and the per-exit ZK-tree Poseidon hashing (one hash per tree level per insert). fn verify_private_batch() -> Weight { let tree_ops = pallet_zk_tree::Pallet::::insert_leaf_db_ops(); let (reads, writes, proof_size) = storage_tail(private_batch_max_exits(), false, tree_ops); - Weight::from_parts(PRIVATE_BATCH_ZK_VERIFY_REF_TIME_PS, proof_size) - .saturating_add(T::DbWeight::get().reads(reads)) - .saturating_add(T::DbWeight::get().writes(writes)) - .saturating_add(Self::pre_validate_proof()) - .saturating_add(Self::pre_validate_proof()) + let hash_time = private_batch_max_exits() + .saturating_mul(pallet_zk_tree::Pallet::::insert_leaf_hash_ref_time()); + Weight::from_parts( + PRIVATE_BATCH_ZK_VERIFY_REF_TIME_PS.saturating_add(hash_time), + proof_size, + ) + .saturating_add(T::DbWeight::get().reads(reads)) + .saturating_add(T::DbWeight::get().writes(writes)) + .saturating_add(Self::pre_validate_proof()) + .saturating_add(Self::pre_validate_proof()) } /// Same double-prevalidation shape as [`Self::verify_private_batch`], scaled /// across all inner segments plus the aggregator rebate. fn verify_public_batch() -> Weight { let tree_ops = pallet_zk_tree::Pallet::::insert_leaf_db_ops(); let (reads, writes, proof_size) = storage_tail(public_batch_max_exits(), true, tree_ops); - Weight::from_parts(PUBLIC_BATCH_ZK_VERIFY_REF_TIME_PS, proof_size) - .saturating_add(T::DbWeight::get().reads(reads)) - .saturating_add(T::DbWeight::get().writes(writes)) - .saturating_add(Self::pre_validate_public_batch_proof()) - .saturating_add(Self::pre_validate_public_batch_proof()) + let hash_time = public_batch_max_exits() + .saturating_mul(pallet_zk_tree::Pallet::::insert_leaf_hash_ref_time()); + Weight::from_parts( + PUBLIC_BATCH_ZK_VERIFY_REF_TIME_PS.saturating_add(hash_time), + proof_size, + ) + .saturating_add(T::DbWeight::get().reads(reads)) + .saturating_add(T::DbWeight::get().writes(writes)) + .saturating_add(Self::pre_validate_public_batch_proof()) + .saturating_add(Self::pre_validate_public_batch_proof()) } } @@ -204,27 +215,39 @@ impl WeightInfo for () { Weight::from_parts(PUBLIC_BATCH_PRE_VALIDATE_REF_TIME_PS, proof_size) .saturating_add(RocksDbWeight::get().reads(1_u64.saturating_add(nullifier_reads))) } - /// See `SubstrateWeight::verify_private_batch`. Tree component priced at - /// `MAX_TREE_DEPTH` (no runtime type to read live depth). + /// See `SubstrateWeight::verify_private_batch`. Tree component (DB ops and + /// Poseidon hashing) priced at `MAX_TREE_DEPTH` (no runtime type to read live depth). fn verify_private_batch() -> Weight { let tree_ops = pallet_zk_tree::insert_leaf_db_ops_at_depth(pallet_zk_tree::MAX_TREE_DEPTH); let (reads, writes, proof_size) = storage_tail(private_batch_max_exits(), false, tree_ops); - Weight::from_parts(PRIVATE_BATCH_ZK_VERIFY_REF_TIME_PS, proof_size) - .saturating_add(RocksDbWeight::get().reads(reads)) - .saturating_add(RocksDbWeight::get().writes(writes)) - .saturating_add(Self::pre_validate_proof()) - .saturating_add(Self::pre_validate_proof()) + let hash_time = private_batch_max_exits().saturating_mul( + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(pallet_zk_tree::MAX_TREE_DEPTH), + ); + Weight::from_parts( + PRIVATE_BATCH_ZK_VERIFY_REF_TIME_PS.saturating_add(hash_time), + proof_size, + ) + .saturating_add(RocksDbWeight::get().reads(reads)) + .saturating_add(RocksDbWeight::get().writes(writes)) + .saturating_add(Self::pre_validate_proof()) + .saturating_add(Self::pre_validate_proof()) } /// See `SubstrateWeight::verify_public_batch`. fn verify_public_batch() -> Weight { let tree_ops = pallet_zk_tree::insert_leaf_db_ops_at_depth(pallet_zk_tree::MAX_TREE_DEPTH); let (reads, writes, proof_size) = storage_tail(public_batch_max_exits(), true, tree_ops); - Weight::from_parts(PUBLIC_BATCH_ZK_VERIFY_REF_TIME_PS, proof_size) - .saturating_add(RocksDbWeight::get().reads(reads)) - .saturating_add(RocksDbWeight::get().writes(writes)) - .saturating_add(Self::pre_validate_public_batch_proof()) - .saturating_add(Self::pre_validate_public_batch_proof()) + let hash_time = public_batch_max_exits().saturating_mul( + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(pallet_zk_tree::MAX_TREE_DEPTH), + ); + Weight::from_parts( + PUBLIC_BATCH_ZK_VERIFY_REF_TIME_PS.saturating_add(hash_time), + proof_size, + ) + .saturating_add(RocksDbWeight::get().reads(reads)) + .saturating_add(RocksDbWeight::get().writes(writes)) + .saturating_add(Self::pre_validate_public_batch_proof()) + .saturating_add(Self::pre_validate_public_batch_proof()) } } @@ -368,6 +391,87 @@ mod tests { ); } + /// Every processed exit inserts a ZK-tree leaf, whose path update computes one + /// Poseidon hash per tree level. That compute must be charged in `ref_time` on + /// top of the DB ops — the mock's `DbWeight` is zero, so any depth-driven + /// `ref_time` growth must come from the hashing term. + #[test] + fn verify_weights_charge_per_exit_hash_compute() { + crate::mock::new_test_ext().execute_with(|| { + type W = SubstrateWeight; + + pallet_zk_tree::Depth::::put(1); + let shallow = W::verify_private_batch(); + pallet_zk_tree::Depth::::put(20); + let deep = W::verify_private_batch(); + assert!( + deep.ref_time() > shallow.ref_time(), + "per-exit Poseidon hashing must make verify ref_time grow with tree depth" + ); + + // Exact floor: ZK verify + both pre-validations + one leaf insert's + // hashing per exit, all at the live depth. + let hash_private = private_batch_max_exits() + .saturating_mul(pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(20)); + assert!( + deep.ref_time() >= + PRIVATE_BATCH_ZK_VERIFY_REF_TIME_PS + + 2 * W::pre_validate_proof().ref_time() + hash_private, + "private verify must charge per-exit hash compute" + ); + + let deep_public = W::verify_public_batch(); + let hash_public = public_batch_max_exits() + .saturating_mul(pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(20)); + assert!( + deep_public.ref_time() >= + PUBLIC_BATCH_ZK_VERIFY_REF_TIME_PS + + 2 * W::pre_validate_public_batch_proof().ref_time() + + hash_public, + "public verify must charge per-exit hash compute" + ); + }); + } + + /// The depth-blind `()` impl must charge the same per-exit hash compute, + /// priced at `MAX_TREE_DEPTH`. + #[test] + fn unit_impl_verify_weights_charge_per_exit_hash_compute() { + let hash_per_insert = pallet_zk_tree::insert_leaf_hash_ref_time_at_depth( + pallet_zk_tree::MAX_TREE_DEPTH, + ); + let tree_ops = + pallet_zk_tree::insert_leaf_db_ops_at_depth(pallet_zk_tree::MAX_TREE_DEPTH); + + let (reads, writes, _) = storage_tail(private_batch_max_exits(), false, tree_ops); + let private_db_time = RocksDbWeight::get() + .reads(reads) + .saturating_add(RocksDbWeight::get().writes(writes)) + .ref_time(); + assert!( + <() as WeightInfo>::verify_private_batch().ref_time() >= + PRIVATE_BATCH_ZK_VERIFY_REF_TIME_PS + + private_db_time + + 2 * <() as WeightInfo>::pre_validate_proof().ref_time() + + private_batch_max_exits().saturating_mul(hash_per_insert), + "() private verify must charge per-exit hash compute on top of DB ops" + ); + + let (reads, writes, _) = storage_tail(public_batch_max_exits(), true, tree_ops); + let public_db_time = RocksDbWeight::get() + .reads(reads) + .saturating_add(RocksDbWeight::get().writes(writes)) + .ref_time(); + assert!( + <() as WeightInfo>::verify_public_batch().ref_time() >= + PUBLIC_BATCH_ZK_VERIFY_REF_TIME_PS + + public_db_time + + 2 * <() as WeightInfo>::pre_validate_public_batch_proof().ref_time() + + public_batch_max_exits().saturating_mul(hash_per_insert), + "() public verify must charge per-exit hash compute on top of DB ops" + ); + } + /// Floor so regenerated weights can't under-price the all-valid worst case. #[test] fn pre_validation_compute_covers_production_path() { diff --git a/pallets/zk-tree/src/lib.rs b/pallets/zk-tree/src/lib.rs index 7169cfea..8c819908 100644 --- a/pallets/zk-tree/src/lib.rs +++ b/pallets/zk-tree/src/lib.rs @@ -246,6 +246,15 @@ pub mod pallet { pub fn insert_leaf_db_ops() -> (u64, u64) { crate::insert_leaf_db_ops_at_depth(Depth::::get()) } + + /// Worst-case Poseidon-hashing `ref_time` for one `insert_leaf` at the tree's + /// *current* depth. See [`insert_leaf_hash_ref_time_at_depth`]. Anything that + /// prices a leaf insert must charge this *in addition to* + /// [`Self::insert_leaf_db_ops`]: the DB ops cover storage I/O only, while the + /// path update also computes one Poseidon hash per tree level. + pub fn insert_leaf_hash_ref_time() -> u64 { + crate::insert_leaf_hash_ref_time_at_depth(Depth::::get()) + } } impl Pallet diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 2656b866..ec8b0b61 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -117,12 +117,15 @@ impl WormholeProofRecorderExtension /// writes: TransferCount (1) + the conditional `PotentialWormholeBalance` /// deposit add (1) => 2 writes. /// plus the ZK-tree leaf insert, whose path update walks the tree leaf-to-root and - /// therefore costs reads/writes proportional to the *current* tree depth (read from - /// storage here, so the charge tracks the tree as it deepens over the chain's life). + /// therefore costs reads/writes *and* one Poseidon hash per level, both proportional + /// to the *current* tree depth (read from storage here, so the charge tracks the + /// tree as it deepens over the chain's life). fn per_transfer_weight() -> Weight { let (tree_reads, tree_writes) = pallet_zk_tree::Pallet::::insert_leaf_db_ops(); + let hash_time = pallet_zk_tree::Pallet::::insert_leaf_hash_ref_time(); T::DbWeight::get() .reads_writes(5u64.saturating_add(tree_reads), 2u64.saturating_add(tree_writes)) + .saturating_add(Weight::from_parts(hash_time, 0)) } fn count_transfers(call: &RuntimeCall) -> u64 { @@ -835,6 +838,32 @@ mod tests { }); } + #[test] + fn per_transfer_weight_includes_tree_hash_compute() { + new_test_ext().execute_with(|| { + // Recording a transfer inserts a ZK-tree leaf; the path update computes one + // Poseidon hash per tree level. That compute must be charged on top of the + // DB ops, otherwise every recorded transfer under-declares execution work + // by an amount that grows with the tree depth. + pallet_zk_tree::Depth::::put(20); + let weight = WormholeProofRecorderExtension::::per_transfer_weight(); + + let (tree_reads, tree_writes) = pallet_zk_tree::insert_leaf_db_ops_at_depth(20); + let db_time = ::DbWeight::get() + .reads_writes( + 5u64.saturating_add(tree_reads), + 2u64.saturating_add(tree_writes), + ) + .ref_time(); + assert!( + weight.ref_time() >= + db_time + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(20), + "per-transfer weight must charge the leaf insert's Poseidon hashing \ + on top of its DB ops" + ); + }); + } + #[test] fn per_transfer_weight_scales_with_tree_depth() { new_test_ext().execute_with(|| { From 14d7fbe04eb6a7f529ce859b5cff15da7ddaeec7 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:25:14 +0800 Subject: [PATCH 06/17] Security review: drop zero-amount credits from wormhole proof recording A zero-value transfer_keep_alive (or zero-value scheduled transfer) emitted a Transfer event that the proof recorder turned into a useless ZK-tree leaf, advancing transfer counts and growing the tree with no value movement. Guard the recorder chokepoint (report the credit as deliberately dropped) and reject zero-amount schedules at the reversible-transfers entry points. Co-authored-by: Cursor --- pallets/reversible-transfers/src/lib.rs | 7 ++++ .../src/tests/test_reversible_transfers.rs | 32 +++++++++++++++ pallets/wormhole/src/lib.rs | 10 +++++ pallets/wormhole/src/tests.rs | 41 +++++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/pallets/reversible-transfers/src/lib.rs b/pallets/reversible-transfers/src/lib.rs index 9007468d..18a410d5 100644 --- a/pallets/reversible-transfers/src/lib.rs +++ b/pallets/reversible-transfers/src/lib.rs @@ -329,6 +329,9 @@ pub mod pallet { TooManyGuardianAccounts, /// Asset transfers are not supported. AssetsNotSupported, + /// Zero-amount transfers cannot be scheduled: there is nothing to hold, + /// execute, or reverse. + ZeroAmount, } #[pallet::call] @@ -753,6 +756,10 @@ pub mod pallet { ) -> DispatchResult { let recipient = T::Lookup::lookup(to.clone())?; ensure!(asset_id.is_none(), Error::::AssetsNotSupported); + // A zero-amount schedule is a pure no-op with side effects: it consumes a + // pending-transfer slot and scheduler agenda space, and its execution would + // dispatch a zero-value transfer. Reject it outright. + ensure!(!amount.is_zero(), Error::::ZeroAmount); // Build the transfer call for tx_id computation (not stored) let transfer_call: RuntimeCallOf = diff --git a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs index 93fef9d0..dbc37b9a 100644 --- a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs +++ b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs @@ -213,6 +213,38 @@ fn set_reversibility_fails_delay_too_short() { }); } +/// A zero-amount schedule is a pure no-op with side effects: it consumes a +/// pending-transfer slot and scheduler agenda space, and its execution would +/// dispatch a zero-value transfer. Both signed scheduling entry points must +/// reject it before any state is written. +#[test] +fn schedule_transfer_rejects_zero_amount() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + + // High-security entry point (alice is high-security from genesis). + assert_err!( + ReversibleTransfers::schedule_transfer(RuntimeOrigin::signed(alice()), bob(), 0), + Error::::ZeroAmount + ); + + // One-time entry point (charlie is a regular account). + assert_err!( + ReversibleTransfers::schedule_transfer_with_delay( + RuntimeOrigin::signed(charlie()), + bob(), + 0, + BlockNumberOrTimestamp::BlockNumber(10), + ), + Error::::ZeroAmount + ); + + // Nothing was scheduled or stored on either path. + assert!(PendingTransfersBySender::::get(&alice()).is_empty()); + assert!(PendingTransfersBySender::::get(&charlie()).is_empty()); + }); +} + #[test] fn schedule_transfer_works() { new_test_ext().execute_with(|| { diff --git a/pallets/wormhole/src/lib.rs b/pallets/wormhole/src/lib.rs index b17e5bf1..26d1843f 100644 --- a/pallets/wormhole/src/lib.rs +++ b/pallets/wormhole/src/lib.rs @@ -1419,6 +1419,16 @@ pub mod pallet { to: ::WormholeAccountId, amount: BalanceOf, ) -> bool { + // A zero-amount credit moves no value, so a leaf for it is pure state growth: + // it would advance the recipient's transfer count, enlarge the ZK tree, and + // emit a transfer event for nothing. Zero-value `Balances::Transfer` events + // are reachable from permissionless surfaces (plain `transfer_keep_alive(0)`, + // zero-value scheduled transfers, ...), so drop the credit here — the single + // chokepoint every event-scan / call-site recorder goes through — and report + // it as not recorded so weight reconciliation does not count a leaf insert. + if amount.is_zero() { + return false; + } // The wormhole tags native leaves with `asset_id == 0`, but `pallet_assets` uses // id 0 for an unrelated, independently-mintable token. Genuine native reaches us as // `None` (from `Balances` events); a `pallet_assets` asset-0 credit reaches us as diff --git a/pallets/wormhole/src/tests.rs b/pallets/wormhole/src/tests.rs index 63529ee0..e0061aac 100644 --- a/pallets/wormhole/src/tests.rs +++ b/pallets/wormhole/src/tests.rs @@ -97,6 +97,47 @@ mod wormhole_tests { }); } + /// A zero-amount credit moves no value, but recording it would still append a + /// ZK-tree leaf, advance the recipient's transfer count, and emit an event. + /// Zero-value `Balances::Transfer` events are reachable from permissionless + /// surfaces (`transfer_keep_alive(dest, 0)`, zero-value scheduled transfers), + /// so the recorder must drop zero-amount credits and report them as not + /// recorded (so weight reconciliation doesn't count a leaf insert). + #[test] + fn zero_amount_credit_is_not_recorded() { + use qp_wormhole::TransferProofRecorder; + + new_test_ext().execute_with(|| { + System::set_block_number(1); + let from = account_id(1); + let to = account_id(9001); + assert!(Wormhole::is_ambiguous_account(&to)); + + assert!( + !>::record_transfer_proof( + None, + from.clone(), + to.clone(), + 0, + ), + "a zero-amount credit must report as not recorded" + ); + assert_eq!(ZkTree::leaf_count(), 0, "no ZK-tree leaf for a zero-amount credit"); + assert_eq!( + Wormhole::transfer_count(&to), + 0, + "the recipient's transfer count must not advance" + ); + assert_eq!(Wormhole::potential_wormhole_balance(), 0); + + // Sanity: the same credit with a nonzero amount is recorded. + assert!(>::record_transfer_proof( + None, from, to, 1, + )); + assert_eq!(ZkTree::leaf_count(), 1); + }); + } + #[test] fn record_transfer_increments_count() { new_test_ext().execute_with(|| { From 60b2d4a1cd793575bbe620b5133c005f8b998cbc Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:26:27 +0800 Subject: [PATCH 07/17] Document the genesis-builder trust model (no input-size limits by design) Genesis JSON is supplied by the node operator building their own chain spec; it is not an untrusted boundary, so size limits there would protect no one. Note this on prepare_genesis_build_input where the next reviewer will look. Co-authored-by: Cursor --- runtime/src/genesis_config_presets.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/runtime/src/genesis_config_presets.rs b/runtime/src/genesis_config_presets.rs index c9676ad1..1c782ef7 100644 --- a/runtime/src/genesis_config_presets.rs +++ b/runtime/src/genesis_config_presets.rs @@ -307,6 +307,17 @@ fn planck_treasury_account() -> AccountId { /// Parses genesis JSON, removes [`TECH_COLLECTIVE_SEED_MEMBERS_KEY`] if present, and returns /// serialized config for [`frame_support::genesis_builder_helper::build_state`] plus the optional /// member list. +/// +/// # Trust model (deliberately no size limits) +/// +/// This runs inside the `GenesisBuilder` runtime API, which is only invoked by the node +/// operator's own tooling (chain-spec building / genesis initialization) with the chain +/// spec that operator chose to launch. It is not reachable by network peers or on a +/// running chain. Whoever supplies this JSON already controls *everything* about the +/// chain being built — balances, keys, code — so input-size bounds here would not +/// protect anyone: an oversized or hostile genesis can only stall the chain of the +/// operator who supplied it. This matches upstream Substrate, whose `build_state` +/// helper deserializes the full unbounded config the same way. pub fn prepare_genesis_build_input( config: Vec, ) -> Result<(Vec, Option>), String> { From 84ee98f8574bfb3c806be31d17f333d1307550b1 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:30:15 +0800 Subject: [PATCH 08/17] Document genesis-build failure semantics (panics are the FRAME channel) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BuildGenesisConfig::build returns () — panicking assertions are the only failure channel for invalid genesis data, inherited verbatim from upstream Substrate, and they abort the operator's own chain-spec build. Extend the trust-model note so the next reviewer finds this answered. Co-authored-by: Cursor --- runtime/src/genesis_config_presets.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/runtime/src/genesis_config_presets.rs b/runtime/src/genesis_config_presets.rs index 1c782ef7..5fbf7246 100644 --- a/runtime/src/genesis_config_presets.rs +++ b/runtime/src/genesis_config_presets.rs @@ -318,6 +318,15 @@ fn planck_treasury_account() -> AccountId { /// protect anyone: an oversized or hostile genesis can only stall the chain of the /// operator who supplied it. This matches upstream Substrate, whose `build_state` /// helper deserializes the full unbounded config the same way. +/// +/// The same reasoning covers failure semantics: semantically invalid genesis data +/// (duplicate balance entries, sub-ED endowments, ...) *panics* inside the pallets' +/// `BuildGenesisConfig::build` rather than returning `Err`. That is FRAME's design — +/// `build` returns `()` and has no error channel; only JSON deserialization (which runs +/// before the trait) can return `Err`. The panics are inherited verbatim from upstream +/// Substrate and are the intended fail-fast: they abort the operator's own chain-spec +/// build with the assertion message, and the failed build's candidate storage is +/// discarded, so nothing half-built can persist. pub fn prepare_genesis_build_input( config: Vec, ) -> Result<(Vec, Option>), String> { From 0e7f4b6f8f2404531c25e11708628bcdcf9786ed Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:51:40 +0800 Subject: [PATCH 09/17] Security review: record hold-transfers (guardian seizure/recovery) in wormhole The proof-recorder extension only matched Balances::Transfer and Balances::Minted, but reversible-transfers releases seized/recovered held funds with transfer_on_hold, which emits Balances::TransferOnHold. The guardian therefore received spendable free balance with no ZK-tree leaf. Match TransferOnHold in the event scan; weight is already reconciled post-dispatch for statically uncountable paths. Co-authored-by: Cursor --- runtime/src/transaction_extensions.rs | 60 ++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index ec8b0b61..68539bb5 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -85,13 +85,15 @@ impl /// Transaction extension that records transfer proofs in the wormhole pallet /// /// This extension uses an EVENT-BASED approach to detect transfers: -/// - After successful execution, scans for Transfer/Transferred/Issued events +/// - After successful execution, scans for `Transfer`, `Minted` and `TransferOnHold` events /// - Records proofs for any transfers that were sent TO a wormhole account /// - Automatically catches ALL transfers regardless of how they're initiated: /// - Direct transfers (transfer, transfer_keep_alive, transfer_all, etc.) /// - Batch transfers (utility.batch, batch_all, force_batch) /// - Multisig transfers (multisig.execute) /// - Recovery transfers (recovery.as_recovered) +/// - Held-fund seizures/recoveries (reversible_transfers.cancel / recover_funds, +/// which move value with `transfer_on_hold` instead of a free-balance transfer) /// - Scheduled transfers (scheduler) /// - Future mechanisms automatically covered /// @@ -204,6 +206,18 @@ impl WormholeProofRecorderExtension let minting_account = crate::configs::MintingAccount::get(); Some((None, minting_account, who, amount)) }, + // Held-balance transfers. The reversible-transfers pallet releases + // seized/recovered funds to the guardian with `transfer_on_hold` + // (`Restriction::Free`), so the destination receives ordinary free + // balance — a genuine credit that needs a leaf exactly like a + // `Transfer`, it just emits a different event. (`TransferAndHold` + // is deliberately not matched: nothing in the runtime emits it.) + RuntimeEvent::Balances(pallet_balances::Event::TransferOnHold { + source, + dest, + amount, + .. + }) => Some((None, source, dest, amount)), _ => None, // Ignore all other events } }) @@ -1076,6 +1090,50 @@ mod tests { }); } + #[test] + fn event_based_proof_recording_guardian_seizure_via_transfer_on_hold() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + + let amount = EXISTENTIAL_DEPOSIT * 10; + let guardian = alice(); + let count_before = Wormhole::transfer_count(&guardian); + + // charlie is high-security (guardian = alice, from genesis); scheduling a + // transfer places the funds on hold. + assert_ok!(ReversibleTransfers::schedule_transfer( + RuntimeOrigin::signed(charlie()), + MultiAddress::Id(bob()), + amount, + )); + let tx_id = + pallet_reversible_transfers::PendingTransfersBySender::::get(charlie()) + [0]; + + // The guardian cancels: the held funds (minus the volume fee) are seized to + // the guardian via `transfer_on_hold`, which emits `Balances::TransferOnHold` + // — not a free-balance `Transfer`. The credit is real spendable value landing + // on the guardian's free balance, so the recorder must create a leaf for it + // exactly as it would for a plain transfer. + let events_before = frame_system::Pallet::::event_count(); + assert_ok!(ReversibleTransfers::cancel( + RuntimeOrigin::signed(guardian.clone()), + tx_id + )); + + let recorded = + WormholeProofRecorderExtension::::record_proofs_from_events_since( + events_before, + ); + + assert_eq!( + recorded, 1, + "hold-transfer seizure must be recorded as a transfer proof" + ); + assert_eq!(Wormhole::transfer_count(&guardian), count_before + 1); + }); + } + #[test] fn event_based_proof_recording_no_proof_for_non_transfer() { new_test_ext().execute_with(|| { From 1cbc1b4b993f57417058d392a1101818530a3cb2 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 14:57:12 +0800 Subject: [PATCH 10/17] Security review: record reserve repatriations (recovery-deposit seizure) in wormhole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same class as the TransferOnHold fix: pallet_recovery::close_recovery moves the rescuer's reserved deposit to the rescued account with repatriate_reserved, which emits Balances::ReserveRepatriated — an event the proof-recorder extension ignored, so the credit got no ZK-tree leaf. Match ReserveRepatriated in the event scan. Sweep of other balance-crediting APIs found no further gaps (referenda slashes burn to (), fee flows are excluded by design, TransferAndHold is unused). Co-authored-by: Cursor --- runtime/src/transaction_extensions.rs | 64 ++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 68539bb5..644e3487 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -85,7 +85,8 @@ impl /// Transaction extension that records transfer proofs in the wormhole pallet /// /// This extension uses an EVENT-BASED approach to detect transfers: -/// - After successful execution, scans for `Transfer`, `Minted` and `TransferOnHold` events +/// - After successful execution, scans for `Transfer`, `Minted`, `TransferOnHold` and +/// `ReserveRepatriated` events /// - Records proofs for any transfers that were sent TO a wormhole account /// - Automatically catches ALL transfers regardless of how they're initiated: /// - Direct transfers (transfer, transfer_keep_alive, transfer_all, etc.) @@ -94,6 +95,8 @@ impl /// - Recovery transfers (recovery.as_recovered) /// - Held-fund seizures/recoveries (reversible_transfers.cancel / recover_funds, /// which move value with `transfer_on_hold` instead of a free-balance transfer) +/// - Recovery-deposit seizures (recovery.close_recovery, which moves the rescuer's +/// deposit with `repatriate_reserved`) /// - Scheduled transfers (scheduler) /// - Future mechanisms automatically covered /// @@ -218,6 +221,18 @@ impl WormholeProofRecorderExtension amount, .. }) => Some((None, source, dest, amount)), + // Reserved-balance repatriations. `pallet_recovery::close_recovery` + // seizes the rescuer's recovery deposit into the rescued account with + // `repatriate_reserved`, which emits this instead of a `Transfer`. The + // event is only emitted for cross-account moves (self-repatriations + // return early), and the credit belongs to `to` whether it lands free + // or reserved, so record it unconditionally. + RuntimeEvent::Balances(pallet_balances::Event::ReserveRepatriated { + from, + to, + amount, + .. + }) => Some((None, from, to, amount)), _ => None, // Ignore all other events } }) @@ -1134,6 +1149,53 @@ mod tests { }); } + #[test] + fn event_based_proof_recording_recovery_deposit_repatriation() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + + // alice makes her account recoverable; bob (say, maliciously) initiates a + // recovery, reserving the recovery deposit on his own account. The recovery + // deposits are UNIT-denominated, so fund both well past the genesis balances. + Balances::make_free_balance_be(&alice(), 100 * crate::UNIT); + Balances::make_free_balance_be(&bob(), 100 * crate::UNIT); + assert_ok!(Recovery::create_recovery( + RuntimeOrigin::signed(alice()), + vec![charlie()], + 1, + 0, + )); + assert_ok!(Recovery::initiate_recovery( + RuntimeOrigin::signed(bob()), + MultiAddress::Id(alice()), + )); + + let count_before = Wormhole::transfer_count(&alice()); + let events_before = frame_system::Pallet::::event_count(); + + // Closing the recovery seizes the rescuer's reserved deposit into alice's + // free balance via `repatriate_reserved`, which emits + // `Balances::ReserveRepatriated` — not a free-balance `Transfer`. The + // credit is real spendable value landing on alice, so the recorder must + // create a leaf for it. + assert_ok!(Recovery::close_recovery( + RuntimeOrigin::signed(alice()), + MultiAddress::Id(bob()), + )); + + let recorded = + WormholeProofRecorderExtension::::record_proofs_from_events_since( + events_before, + ); + + assert_eq!( + recorded, 1, + "reserve repatriation must be recorded as a transfer proof" + ); + assert_eq!(Wormhole::transfer_count(&alice()), count_before + 1); + }); + } + #[test] fn event_based_proof_recording_no_proof_for_non_transfer() { new_test_ext().execute_with(|| { From ab976ca9176578680bfc46acb9d39192e3ede6ab Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 15:02:24 +0800 Subject: [PATCH 11/17] Document the proof-recorder coverage boundary (hooks vs transactions) The extension doc claimed scheduler-dispatched transfers were automatically covered, but extensions never run for hook-context dispatch. Spell out the actual contract: signed-extrinsic events are scanned; hook-context credits record explicitly (reversible-transfers execution, mining rewards/treasury); and governance enactment via the scheduler is a known, accepted gap because only Root can reach it and Root can already forge leaves via set_storage. Co-authored-by: Cursor --- runtime/src/transaction_extensions.rs | 29 +++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 644e3487..d088c6f3 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -88,7 +88,8 @@ impl /// - After successful execution, scans for `Transfer`, `Minted`, `TransferOnHold` and /// `ReserveRepatriated` events /// - Records proofs for any transfers that were sent TO a wormhole account -/// - Automatically catches ALL transfers regardless of how they're initiated: +/// - Automatically catches ALL transfers dispatched inside a transaction, regardless of +/// how they're initiated: /// - Direct transfers (transfer, transfer_keep_alive, transfer_all, etc.) /// - Batch transfers (utility.batch, batch_all, force_batch) /// - Multisig transfers (multisig.execute) @@ -97,10 +98,30 @@ impl /// which move value with `transfer_on_hold` instead of a free-balance transfer) /// - Recovery-deposit seizures (recovery.close_recovery, which moves the rescuer's /// deposit with `repatriate_reserved`) -/// - Scheduled transfers (scheduler) -/// - Future mechanisms automatically covered +/// - Future call-based mechanisms automatically covered, since wrapper calls emit +/// their inner events within the same extrinsic's event range /// -/// This addresses audit item EQ-QNT-WORMHOLE-F-05 comprehensively. +/// COVERAGE BOUNDARY: transaction extensions only run for transactions, so this scan +/// never sees events emitted from hooks (`on_initialize` / `on_finalize`). Every +/// hook-context credit therefore needs — and has — an explicit +/// `TransferProofRecorder::record_transfer_proof` call instead: +/// - reversible-transfers' scheduled execution records its transfer in +/// `do_execute_transfer`; +/// - mining rewards and the treasury share record theirs in `on_finalize` +/// (`pallet_mining_rewards`), using eventless `increase_balance` credits. +/// +/// The one remaining hook-context path is a governance-enacted call: referenda enactment +/// dispatches the approved call via the scheduler in `on_initialize` (e.g. a Root +/// `force_transfer`), so its events are not scanned and no leaf is recorded. This is a +/// known, accepted gap rather than an oversight: the scheduler's `ScheduleOrigin` is +/// Root, the tech-referenda track only accepts Root proposal origins, and sudo is +/// removed — so only Root can reach it, and Root can already forge or delete leaves +/// outright (`set_storage`, runtime upgrades), so there is no invariant left to defend +/// against it. The miss is conservative (the credit exists but gains no ZK-spendable +/// leaf; no unbacked exit capacity is created) and repairable (governance can re-issue +/// the credit as an ordinary signed transfer if a leaf is wanted). +/// +/// This addresses audit item EQ-QNT-WORMHOLE-F-05. #[derive(Encode, Decode, Clone, Eq, PartialEq, Default, TypeInfo, Debug, DecodeWithMemTracking)] #[scale_info(skip_type_params(T))] pub struct WormholeProofRecorderExtension(PhantomData); From cd045424abb9b0da5366b26f66d4099987c01e49 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 16:19:16 +0800 Subject: [PATCH 12/17] Security review: require canonical hashes in the zk-tree proof RPC resolve_proof_block accepted any backend-resolvable hash, so side-fork blocks (and fork blocks above best, where the one-sided window check saturates to 0) produced proof material that settlement always rejects against frame_system::BlockHash. Reject heights above best and require the requested hash to match the canonical hash at its height. The test mock now distinguishes imported blocks from the canonical index so fork-block cases are expressible. Co-authored-by: Cursor --- node/src/zktree_rpc.rs | 110 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 103 insertions(+), 7 deletions(-) diff --git a/node/src/zktree_rpc.rs b/node/src/zktree_rpc.rs index a786e225..1b725d05 100644 --- a/node/src/zktree_rpc.rs +++ b/node/src/zktree_rpc.rs @@ -70,6 +70,12 @@ pub trait ZkTreeApi { /// the proving block's hash is still in `frame_system::BlockHash`, a sliding /// window of `BlockHashCount` blocks. Blocks outside that window are rejected. /// +/// The requested hash must also be the *canonical* hash at its height. The backend +/// resolves numbers for any imported block (side forks included), but settlement +/// verifies the claimed hash against `frame_system::BlockHash`, so a proof built on +/// fork state is unusable by construction — reject it here instead of spending +/// state-execution resources producing it. +/// /// The window is `quantus_runtime::configs::BlockHashCount` from the runtime /// crate linked into this node binary — a compile-time constant, not a live /// chain/metadata lookup. After a forkless upgrade that changes @@ -104,6 +110,43 @@ where )), }; + // The backend resolves a number for ANY block it has imported, including + // side-fork blocks — resolvability is not canonicality. On-chain settlement + // compares the proof's claimed hash against `frame_system::BlockHash` (the + // canonical chain), so proof material derived from fork state can never + // settle. Reject anything that is not the canonical hash at its height; + // heights above best (where `best - number` saturates to 0) are rejected + // first for a precise error. + if number > info.best_number { + return Err(jsonrpsee::types::error::ErrorObject::owned( + 9007, + format!( + "Block {hash:?} (#{number}) is above the current best block \ + (#{best}); it is not on the canonical chain", + best = info.best_number, + ), + None::<()>, + )); + } + + let canonical = client.hash(number).map_err(|e| { + jsonrpsee::types::error::ErrorObject::owned( + 9006, + format!("Failed to resolve canonical hash at #{number}: {e}"), + None::<()>, + ) + })?; + if canonical != Some(hash) { + return Err(jsonrpsee::types::error::ErrorObject::owned( + 9008, + format!( + "Block {hash:?} (#{number}) is not on the canonical chain; proofs \ + against fork state cannot be verified on-chain" + ), + None::<()>, + )); + } + // Compile-time constant from the linked runtime crate — see fn docs. let window = >::get(); if info.best_number.saturating_sub(number) > window { @@ -207,18 +250,25 @@ mod tests { H256::from_low_u64_be(u64::from(number) + 1) } - /// Minimal chain view: a best block and a set of known (hash -> number) blocks. + /// Minimal chain view: a best block, the known (hash -> number) blocks the + /// backend has imported (canonical *and* side-fork), and the canonical + /// (number -> hash) index. struct MockChain { best_number: u32, - blocks: HashMap, + /// Every imported block, like the backend's hash->number index. Includes + /// side-fork blocks, which is exactly why resolvability != canonicality. + known: HashMap, + /// The canonical chain's number->hash index. + canonical: HashMap, /// Hashes for which `number()` simulates a backend/DB failure. failing: HashSet, } impl MockChain { fn with_blocks(best_number: u32, numbers: &[u32]) -> Self { - let blocks = numbers.iter().map(|n| (hash_for(*n), *n)).collect(); - Self { best_number, blocks, failing: HashSet::new() } + let known = numbers.iter().map(|n| (hash_for(*n), *n)).collect(); + let canonical = numbers.iter().map(|n| (*n, hash_for(*n))).collect(); + Self { best_number, known, canonical, failing: HashSet::new() } } fn with_number_failure(best_number: u32, failing_hash: H256) -> Self { @@ -226,6 +276,14 @@ mod tests { chain.failing.insert(failing_hash); chain } + + /// Add a block the backend knows about (imported) that is NOT on the + /// canonical chain, at the given height. Returns its hash. + fn add_fork_block(&mut self, number: u32) -> H256 { + let fork_hash = H256::from_low_u64_be(0xF0_0000 + u64::from(number)); + self.known.insert(fork_hash, number); + fork_hash + } } impl HeaderBackend for MockChain { @@ -247,7 +305,7 @@ mod tests { } fn status(&self, hash: H256) -> BlockchainResult { - Ok(if self.blocks.contains_key(&hash) { + Ok(if self.known.contains_key(&hash) { BlockStatus::InChain } else { BlockStatus::Unknown @@ -258,11 +316,11 @@ mod tests { if self.failing.contains(&hash) { return Err(sp_blockchain::Error::Backend("simulated db failure".into())); } - Ok(self.blocks.get(&hash).copied()) + Ok(self.known.get(&hash).copied()) } fn hash(&self, number: NumberFor) -> BlockchainResult> { - Ok(self.blocks.iter().find(|(_, n)| **n == number).map(|(h, _)| *h)) + Ok(self.canonical.get(&number).copied()) } } @@ -305,6 +363,44 @@ mod tests { assert!(resolve_proof_block(&chain, Some(hash_for(ancient))).is_err()); } + /// The backend resolves a number for ANY imported block, including side-fork + /// blocks — resolvability is not canonicality. A proof generated against fork + /// state can never settle (the wormhole pallet compares the claimed hash to + /// `frame_system::BlockHash`, the canonical chain), so the RPC must reject + /// noncanonical hashes instead of burning state-execution resources on them. + #[test] + fn rejects_noncanonical_hashes_within_the_window() { + let best = 10 * window(); + let fork_height = best - 5; + let mut chain = MockChain::with_blocks(best, &[best, fork_height]); + let fork_hash = chain.add_fork_block(fork_height); + + let err = resolve_proof_block(&chain, Some(fork_hash)) + .expect_err("side-fork hash must be rejected even inside the proof window"); + assert_eq!(err.code(), 9008); + + // The canonical block at the same height is still accepted. + assert_eq!( + resolve_proof_block(&chain, Some(hash_for(fork_height))).unwrap(), + hash_for(fork_height) + ); + } + + /// A backend-known block ABOVE the current best (e.g. from a longer side + /// fork that was imported but not chosen) makes `best_number - number` + /// saturate to 0, which the one-sided window check happily accepts. Heights + /// above best have no canonical hash and can never settle. + #[test] + fn rejects_blocks_above_the_best_number() { + let best = 10 * window(); + let mut chain = MockChain::with_blocks(best, &[best]); + let ahead_hash = chain.add_fork_block(best + 5); + + let err = resolve_proof_block(&chain, Some(ahead_hash)) + .expect_err("block above best must be rejected"); + assert_eq!(err.code(), 9007); + } + #[test] fn rejects_unknown_block_hashes() { let best = 10 * window(); From bc66b510bfaaa71ee5e2143acbaa6ab0e4eceb78 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 16:26:35 +0800 Subject: [PATCH 13/17] Security review: bound and canonicalize settlement proof bytes pre_validate_{private,public}_batch_proof copied and parsed unbounded attacker bytes on the fee-free unsigned path, and plonky2's from_bytes silently ignores trailing bytes, giving one proof unboundedly many byte representations that each re-cost copy+parse at pool admission. Gate length at MAX_PROOF_BYTES (512 KiB vs ~151/224 KB real proofs) before any copy, and require the bytes to round-trip through to_bytes so each proof has exactly one accepted encoding. Release-mode recalibration shows the existing pre-validation weight constants still cover the added serialize pass with large margin. Co-authored-by: Cursor --- pallets/wormhole/src/lib.rs | 45 ++++++++++++++++++++++++++++ pallets/wormhole/src/tests.rs | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/pallets/wormhole/src/lib.rs b/pallets/wormhole/src/lib.rs index 26d1843f..908df4a1 100644 --- a/pallets/wormhole/src/lib.rs +++ b/pallets/wormhole/src/lib.rs @@ -27,6 +27,22 @@ const PRIVATE_BATCH_PI_HEADER_FELTS: usize = 8; /// exit traffic. pub const UNSIGNED_EXIT_PRIORITY: u64 = 1; +/// Hard upper bound on the serialized size of a settlement proof (the `proof_bytes` +/// argument of `verify_private_batch` / `verify_public_batch`), enforced before the +/// blob is copied or parsed. +/// +/// Settlement extrinsics are unsigned and fee-free, and pre-validation runs for every +/// gossiped pool candidate, so without this gate the only bound on the bytes an +/// attacker can make every node copy (`to_vec`) and feed through the plonky2 parser +/// is the block-length limit — megabytes above any real proof. Proof sizes are fixed +/// by the compiled circuit dimensions: the current fixtures serialize to ~151 KB +/// (private batch) and ~224 KB (public batch), so 512 KiB leaves ample headroom for +/// circuit-knob growth (proof size scales only mildly with batch counts) while +/// keeping worst-case admission work near real-proof cost. If a circuit upgrade ever +/// pushes a real proof past this cap, `pre_validation_rejects_oversized_proof_bytes` +/// and every fixture-based settlement test will fail loudly at the same time. +pub const MAX_PROOF_BYTES: usize = 512 * 1024; + /// Expected public-input count of the private-batch circuit compiled into this runtime. fn private_batch_expected_public_inputs() -> usize { PRIVATE_BATCH_PI_HEADER_FELTS + circuit_config::NUM_LEAF_PROOFS * PUBLIC_INPUTS_FELTS_LEN @@ -457,6 +473,13 @@ pub mod pallet { BlockNotFound, VerifierNotAvailable, ProofDeserializationFailed, + /// The submitted proof blob exceeds [`crate::MAX_PROOF_BYTES`]. Rejected before + /// any copy or parsing so oversized unsigned spam costs only a length check. + ProofTooLarge, + /// The proof bytes are not the canonical serialization of the decoded proof + /// (e.g. a valid proof with trailing bytes, which the plonky2 parser would + /// silently ignore). Every proof has exactly one accepted byte encoding. + NonCanonicalProofEncoding, ProofVerificationFailed, InvalidProofPublicInputs, /// The volume fee rate in the proof doesn't match the configured rate @@ -1151,6 +1174,11 @@ pub mod pallet { ), Error, > { + // Length gate FIRST: `proof_bytes` is attacker-controlled, unsigned and + // fee-free, and everything below copies (`to_vec`) and parses the whole + // blob. Without this bound the only limit is the block-length cap, + // megabytes above any real proof. + ensure!(proof_bytes.len() <= crate::MAX_PROOF_BYTES, Error::::ProofTooLarge); let verifier = crate::get_private_batch_verifier() .map_err(|_| Error::::VerifierNotAvailable)?; let proof = ProofWithPublicInputs::::from_bytes( @@ -1158,6 +1186,16 @@ pub mod pallet { &verifier.circuit_data.common, ) .map_err(|_| Error::::ProofDeserializationFailed)?; + // Exact-framing check: `from_bytes` reads the proof off the front of the + // buffer and silently ignores trailing bytes, so without this a valid + // proof would have unboundedly many accepted byte representations — each + // a distinct tx hash whose copy+parse the pool re-pays at admission. + // Round-tripping pins one canonical encoding per proof (and also rejects + // non-canonical field encodings). + ensure!( + proof.to_bytes().as_slice() == proof_bytes, + Error::::NonCanonicalProofEncoding + ); let inputs = parse_private_batch_public_inputs(&proof) .map_err(|_| Error::::InvalidProofPublicInputs)?; let bundle: ExitBundle = inputs.into(); @@ -1195,6 +1233,9 @@ pub mod pallet { ), Error, > { + // Same gates as `pre_validate_private_batch_proof`: length bound before + // any copy/parse, then exact canonical framing after deserialization. + ensure!(proof_bytes.len() <= crate::MAX_PROOF_BYTES, Error::::ProofTooLarge); let verifier = crate::get_public_batch_verifier().map_err(|_| Error::::VerifierNotAvailable)?; let proof = ProofWithPublicInputs::::from_bytes( @@ -1202,6 +1243,10 @@ pub mod pallet { &verifier.circuit_data.common, ) .map_err(|_| Error::::ProofDeserializationFailed)?; + ensure!( + proof.to_bytes().as_slice() == proof_bytes, + Error::::NonCanonicalProofEncoding + ); let inputs = parse_public_batch_public_inputs( &proof, crate::circuit_config::NUM_PRIVATE_BATCH_PROOFS, diff --git a/pallets/wormhole/src/tests.rs b/pallets/wormhole/src/tests.rs index e0061aac..98353625 100644 --- a/pallets/wormhole/src/tests.rs +++ b/pallets/wormhole/src/tests.rs @@ -989,6 +989,43 @@ mod private_batch_proof_tests { PotentialWormholeBalance::::put(1_000_000 * UNIT); } + /// `ProofWithPublicInputs::from_bytes` reads the proof off the front of the buffer + /// and silently ignores trailing bytes, so without an exact-framing check one valid + /// proof has unboundedly many byte representations — each a distinct transaction + /// hash whose full copy + parse every node re-pays at pool admission, fee-free. + /// Pre-validation must accept exactly one canonical encoding per proof. + #[test] + fn pre_validation_rejects_padded_proof_bytes() { + new_test_ext().execute_with(|| { + setup_valid_block_state_for_test_proof(); + + // The canonical encoding passes pre-validation. + assert!(Wormhole::pre_validate_private_batch_proof(&get_test_proof_bytes()).is_ok()); + + // The same proof with trailing junk must be rejected. + let mut padded = get_test_proof_bytes(); + padded.extend_from_slice(&[0u8; 32]); + assert!(matches!( + Wormhole::pre_validate_private_batch_proof(&padded), + Err(Error::::NonCanonicalProofEncoding) + )); + }); + } + + /// Oversized blobs must be cut off by a length gate BEFORE the byte copy and the + /// parser run — `ProofDeserializationFailed` after the fact means the work was + /// already done. + #[test] + fn pre_validation_rejects_oversized_proof_bytes() { + new_test_ext().execute_with(|| { + let oversized = vec![0u8; crate::MAX_PROOF_BYTES + 1]; + assert!(matches!( + Wormhole::pre_validate_private_batch_proof(&oversized), + Err(Error::::ProofTooLarge) + )); + }); + } + /// The block-inclusion gate (`pre_dispatch`) must reject a proof that cannot be /// verified. Before this was fixed, `pre_dispatch` was a no-op that returned `Ok(())` /// for any `verify_*` call, so junk rode into blocks as failed `Pays::No` extrinsics; @@ -2068,6 +2105,25 @@ mod public_batch_proof_tests { PotentialWormholeBalance::::put(1_000_000 * UNIT); } + /// Public-batch twin of the private-batch exact-framing test: trailing bytes after + /// a valid proof are silently ignored by the plonky2 parser, so they must be + /// rejected by the canonical-encoding check. + #[test] + fn pre_validation_rejects_padded_proof_bytes() { + new_test_ext().execute_with(|| { + setup_matching_block_state(&parse_test_inputs()); + + assert!(Wormhole::pre_validate_public_batch_proof(&get_test_proof_bytes()).is_ok()); + + let mut padded = get_test_proof_bytes(); + padded.extend_from_slice(&[0u8; 32]); + assert!(matches!( + Wormhole::pre_validate_public_batch_proof(&padded), + Err(Error::::NonCanonicalProofEncoding) + )); + }); + } + #[test] fn test_parse_public_batch_public_inputs_succeeds() { let inputs = parse_test_inputs(); From 50c5f0b185ce4236ed2855b3640329217c7b078c Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 16:38:09 +0800 Subject: [PATCH 14/17] Security review: charge high-security policy reads in wrapper dispatch weights is_call_allowed hides an is_high_security classification read; as_derivative and as_recovered enforced the policy without charging the read, and Multisig::propose paid it twice (weight selection + is_call_allowed) while its weights document one read. Split the policy predicate into is_call_allowed_given so propose reuses its fetched classification, and add the read to the two wrapper weight declarations (and as_derivative's actual-weight mirror). Recovery's mock gets a non-zero DbWeight so the new weight assertions are meaningful. Co-authored-by: Cursor --- pallets/multisig/src/lib.rs | 7 +++++-- pallets/recovery/src/lib.rs | 8 +++++++- pallets/recovery/src/mock.rs | 3 +++ pallets/recovery/src/tests.rs | 25 +++++++++++++++++++++++ pallets/utility/src/lib.rs | 9 +++++++-- pallets/utility/src/tests.rs | 31 +++++++++++++++++++++++++++++ primitives/high-security/src/lib.rs | 17 +++++++++++++++- 7 files changed, 94 insertions(+), 6 deletions(-) diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index 7c6b4ba4..b2ab59bf 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -624,8 +624,11 @@ pub mod pallet { // ===== PHASE 4: High-security whitelist check (if applicable) ===== // (additional read: HighSecurityAccounts) let is_high_security = T::HighSecurity::is_high_security(&multisig_address); - // Use the shared `is_call_allowed` policy so `propose` and `execute` stay consistent. - if !T::HighSecurity::is_call_allowed(&multisig_address, &decoded_call) { + // Apply the shared call policy (the same predicate `execute` consults via + // `is_call_allowed`) using the classification already fetched above for + // weight selection, so the `HighSecurityAccounts` lookup is not repeated — + // the propose weights charge exactly one classification read. + if !T::HighSecurity::is_call_allowed_given(is_high_security, &decoded_call) { // Don't refund after decode - same reasoning as above. return Self::err_burn_full(Error::::CallNotAllowedForHighSecurityMultisig); } diff --git a/pallets/recovery/src/lib.rs b/pallets/recovery/src/lib.rs index 5944c47c..9a24f13f 100644 --- a/pallets/recovery/src/lib.rs +++ b/pallets/recovery/src/lib.rs @@ -433,7 +433,13 @@ pub mod pallet { #[pallet::weight({ let dispatch_info = call.get_dispatch_info(); ( - T::WeightInfo::as_recovered().saturating_add(dispatch_info.call_weight), + T::WeightInfo::as_recovered() + // High-security policy check on the recovered account + // (`is_call_allowed` → one classification read in the runtime + // inspector); the benchmarked base runs with the no-op inspector + // and does not include it. + .saturating_add(T::DbWeight::get().reads(1)) + .saturating_add(dispatch_info.call_weight), dispatch_info.class, )})] pub fn as_recovered( diff --git a/pallets/recovery/src/mock.rs b/pallets/recovery/src/mock.rs index 90509579..b7bd68ec 100644 --- a/pallets/recovery/src/mock.rs +++ b/pallets/recovery/src/mock.rs @@ -37,6 +37,9 @@ construct_runtime!( impl frame_system::Config for Test { type Block = Block; type AccountData = pallet_balances::AccountData; + // A non-zero database weight so tests can observe the db-op components of + // the weights the dispatchables charge (the prelude default is zero). + type DbWeight = frame::deps::frame_support::weights::constants::RocksDbWeight; } parameter_types! { diff --git a/pallets/recovery/src/tests.rs b/pallets/recovery/src/tests.rs index 994bfa2b..15132670 100644 --- a/pallets/recovery/src/tests.rs +++ b/pallets/recovery/src/tests.rs @@ -32,6 +32,31 @@ fn basic_setup_works() { }); } +/// `as_recovered` consults the high-security policy on the recovered account +/// (`T::HighSecurity::is_call_allowed`) before dispatching, which in the runtime +/// costs one `HighSecurityAccounts` storage read. The benchmarked base ran with +/// the no-op inspector, so the declared weight must add that read explicitly. +#[test] +fn as_recovered_weight_charges_high_security_policy_read() { + new_test_ext().execute_with(|| { + let inner = RuntimeCall::System(frame_system::Call::remark { remark: vec![] }); + let inner_weight = inner.get_dispatch_info().call_weight; + let call = + RuntimeCall::Recovery(crate::Call::as_recovered { account: 5, call: Box::new(inner) }); + + let db = ::DbWeight::get(); + let without_policy_read = + ::WeightInfo::as_recovered().saturating_add(inner_weight); + + assert!( + call.get_dispatch_info() + .call_weight + .all_gte(without_policy_read.saturating_add(db.reads(1))), + "declared as_recovered weight must include the high-security policy read" + ); + }); +} + /// A Root-installed proxy must hold the same frame_system consumer reference as a /// `claim_recovery`-created one: the reference keeps the rescuer account alive while the /// proxy exists, and it backs the unconditional `dec_consumers` in `cancel_recovered`, diff --git a/pallets/utility/src/lib.rs b/pallets/utility/src/lib.rs index 740953bf..922ef814 100644 --- a/pallets/utility/src/lib.rs +++ b/pallets/utility/src/lib.rs @@ -281,6 +281,10 @@ pub mod pallet { T::WeightInfo::as_derivative() // AccountData for inner call origin accountdata. .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + // High-security policy check on the pseudonym (`is_call_allowed` → + // one classification read in the runtime inspector); the benchmarked + // base runs with the no-op inspector and does not include it. + .saturating_add(T::DbWeight::get().reads(1)) // First-use derivative reveal: `KnownDerivatives` read + insert and the // wormhole pool write. .saturating_add(T::DbWeight::get().reads_writes(1, 2)) @@ -319,10 +323,11 @@ pub mod pallet { let info = call.get_dispatch_info(); let result = call.dispatch(origin); // Always take into account the base weight of this call, plus the - // `KnownDerivatives` membership read performed on every invocation. + // `KnownDerivatives` membership read and the high-security policy read + // on the pseudonym, both performed on every invocation. let mut weight = T::WeightInfo::as_derivative() .saturating_add(T::DbWeight::get().reads_writes(1, 1)) - .saturating_add(T::DbWeight::get().reads(1)); + .saturating_add(T::DbWeight::get().reads(2)); // The `KnownDerivatives` insert and the wormhole pool write only happen on a // pseudonym's first use; refund them to repeat users via the actual weight. if revealed { diff --git a/pallets/utility/src/tests.rs b/pallets/utility/src/tests.rs index fd5a236c..9e9ef25d 100644 --- a/pallets/utility/src/tests.rs +++ b/pallets/utility/src/tests.rs @@ -458,6 +458,37 @@ fn as_derivative_handles_weight_refund() { }); } +/// `as_derivative` consults the high-security policy on the pseudonym +/// (`T::HighSecurity::is_call_allowed`) before dispatching, which in the runtime +/// costs one `HighSecurityAccounts` storage read. The declared weight must +/// charge that read on top of the benchmarked base (which runs with a no-op +/// inspector), the AccountData ops, the reveal ops, and the inner call. +#[test] +fn as_derivative_weight_charges_high_security_policy_read() { + new_test_ext().execute_with(|| { + let inner = RuntimeCall::System(frame_system::Call::remark { remark: vec![] }); + let inner_weight = inner.get_dispatch_info().call_weight; + let call = + RuntimeCall::Utility(UtilityCall::as_derivative { index: 0, call: Box::new(inner) }); + + let db = ::DbWeight::get(); + // Everything the declared weight covered before the policy read was + // accounted: benchmarked base + AccountData r/w + first-use reveal ops + // + the inner call. + let without_policy_read = ::WeightInfo::as_derivative() + .saturating_add(db.reads_writes(1, 1)) + .saturating_add(db.reads_writes(1, 2)) + .saturating_add(inner_weight); + + assert!( + call.get_dispatch_info() + .call_weight + .all_gte(without_policy_read.saturating_add(db.reads(1))), + "declared as_derivative weight must include the high-security policy read" + ); + }); +} + #[test] fn as_derivative_filters() { new_test_ext().execute_with(|| { diff --git a/primitives/high-security/src/lib.rs b/primitives/high-security/src/lib.rs index d4103aec..c6d32209 100644 --- a/primitives/high-security/src/lib.rs +++ b/primitives/high-security/src/lib.rs @@ -122,13 +122,28 @@ pub trait HighSecurityInspector { /// `Some(guardian_account)` if the account has a guardian, `None` otherwise fn guardian(who: &AccountId) -> Option; + /// Evaluate the call policy for an account whose high-security classification has + /// already been determined. + /// + /// This is the single policy predicate behind [`Self::is_call_allowed`], split out + /// so a caller that already paid the `is_high_security` lookup for another purpose + /// (e.g. weight selection in `pallet_multisig::propose`) can apply the policy + /// without repeating the classification storage read. + fn is_call_allowed_given(is_high_security: bool, call: &RuntimeCall) -> bool { + !is_high_security || Self::is_whitelisted(call) + } + /// Whether `call` may be dispatched with `who` as the effective signed origin. /// /// Non-High-Security accounts may dispatch anything; High-Security accounts are /// restricted to whitelisted calls. Origin-rewriting wrappers (multisig execution, /// `as_recovered`, `as_derivative`) must consult this before dispatching as `who`. + /// + /// NOTE: this performs one `is_high_security` classification lookup — a storage + /// read in the runtime implementation — so every dispatchable that calls it must + /// charge that read in its declared weight. fn is_call_allowed(who: &AccountId, call: &RuntimeCall) -> bool { - !Self::is_high_security(who) || Self::is_whitelisted(call) + Self::is_call_allowed_given(Self::is_high_security(who), call) } // NOTE: No benchmarking-specific methods in the trait! From c35ec302acddd87b52272a4f8c9b5b5da6229599 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 16:48:59 +0800 Subject: [PATCH 15/17] Security review: meter the post-dispatch event scan against block weight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit record_proofs_from_events_since stream-decodes every event record present at scan time (Iterator::skip discards but still decodes the pre-snapshot prefix), yet weight() only charges per counted transfer and the post-hoc registration only fired for recording shortfalls — batched remark_with_event traffic produced unmetered linear decode work. Register event_scan_weight (one Events read + a conservative 1µs/record decode ceiling) alongside the existing shortfall in post_dispatch. Co-authored-by: Cursor --- runtime/src/transaction_extensions.rs | 98 ++++++++++++++++++++++++--- 1 file changed, 89 insertions(+), 9 deletions(-) diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index d088c6f3..72c79eca 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -154,6 +154,28 @@ impl WormholeProofRecorderExtension .saturating_add(Weight::from_parts(hash_time, 0)) } + /// Worst-case `ref_time` (picoseconds) to stream-decode one `EventRecord` in + /// [`Self::record_proofs_from_events_since`]. A record is a small SCALE blob + /// (phase + event enum + topics, typically well under ~300 bytes) decoded from an + /// already-fetched storage value — roughly 100–300ns of pure decode on reference + /// hardware; 1µs is a conservative ceiling. + const EVENT_SCAN_DECODE_REF_TIME_PS: u64 = 1_000_000; + + /// Weight of the post-dispatch event scan when `events` records are present at + /// scan time. `Events::stream_iter` fetches the storage value (one read) and the + /// scan then decodes EVERY record present — `Iterator::skip` discards but still + /// decodes the pre-snapshot prefix — so the cost is per record *present*, not per + /// record matched or recorded. + fn event_scan_weight(events: u32) -> Weight { + if events == 0 { + return Weight::zero(); + } + T::DbWeight::get().reads(1).saturating_add(Weight::from_parts( + Self::EVENT_SCAN_DECODE_REF_TIME_PS.saturating_mul(u64::from(events)), + 0, + )) + } + fn count_transfers(call: &RuntimeCall) -> u64 { // NOTE: this must stay in sync with the events matched by `record_proofs_from_events_since` // — we only weight calls whose emitted events we actually record. In particular @@ -371,18 +393,33 @@ impl TransactionEx // Use the event count snapshot from prepare() to avoid duplicate recording. if result.is_ok() { let (event_count_before, charged_transfers) = pre; + // Captured BEFORE recording deposits new events: this is exactly the number + // of records the scan below decodes. + let events_at_scan = frame_system::Pallet::::event_count(); let recorded = Self::record_proofs_from_events_since(event_count_before); - // Wrappers that dispatch inner calls stored on-chain (`Multisig::execute`, - // `ReversibleTransfers::recover_funds`, ...) can emit transfer events the static - // `count_transfers` matcher cannot see, so the proof-recording work above may exceed - // the weight reserved by `weight()`. Register the shortfall against the block so - // block-weight based DoS protection stays sound even when the static count drifts. + // Two pieces of caller-influenced work here are invisible to the static + // `weight()` and are therefore registered against the block post-hoc (this + // keeps block-capacity accounting sound; it is not fee-charged): + // + // 1. The event scan itself: any call can emit events the scan must decode + // (e.g. batched `remark_with_event`), and the decode cost is per record + // present at scan time — see `event_scan_weight`. + // + // 2. Recording shortfall: wrappers that dispatch inner calls stored on-chain + // (`Multisig::execute`, `ReversibleTransfers::recover_funds`, ...) can emit + // transfer events the static `count_transfers` matcher cannot see, so the + // proof-recording work above may exceed the weight reserved by `weight()`. + let mut extra = Self::event_scan_weight(events_at_scan); if recorded > charged_transfers { - frame_system::Pallet::::register_extra_weight_unchecked( + extra = extra.saturating_add( Self::per_transfer_weight() .saturating_mul(recorded.saturating_sub(charged_transfers)), - info.class, + ); + } + if extra != Weight::zero() { + frame_system::Pallet::::register_extra_weight_unchecked( + extra, info.class, ); } } @@ -945,19 +982,62 @@ mod tests { let weight_before = frame_system::Pallet::::block_weight().total(); + let scanned = core::cell::Cell::new(0u32); run_lifecycle(&alice(), opaque_call, || { assert_ok!(Balances::transfer_keep_alive( RuntimeOrigin::signed(alice()), MultiAddress::Id(bob()), EXISTENTIAL_DEPOSIT * 50, )); + scanned.set(frame_system::Pallet::::event_count()); + }); + + let weight_after = frame_system::Pallet::::block_weight().total(); + assert_eq!( + weight_after.saturating_sub(weight_before), + WormholeProofRecorderExtension::::per_transfer_weight().saturating_add( + WormholeProofRecorderExtension::::event_scan_weight(scanned.get()) + ), + "the uncounted recorded transfer must be registered as extra block weight, \ + on top of the always-registered event-scan weight" + ); + }); + } + + /// The post-dispatch scan streams `System::Events` through a decoding iterator — + /// and `skip()` still decodes the records it discards — so every event record + /// present at scan time costs decode work even when nothing is recorded. A signed + /// caller can emit arbitrarily many events with zero-transfer calls (e.g. batched + /// `remark_with_event`), so that work must be registered against the block. + #[test] + fn wormhole_proof_recorder_registers_event_scan_weight() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + + let call = RuntimeCall::System(frame_system::Call::remark { remark: vec![1] }); + assert_eq!(WormholeProofRecorderExtension::::count_transfers(&call), 0); + + let weight_before = frame_system::Pallet::::block_weight().total(); + + // Capture the event count at the end of the dispatch closure: that is + // exactly the number of records the post-dispatch scan decodes. + let scanned = core::cell::Cell::new(0u32); + run_lifecycle(&alice(), call, || { + for i in 0..7u8 { + assert_ok!(System::remark_with_event( + RuntimeOrigin::signed(alice()), + vec![i], + )); + } + scanned.set(frame_system::Pallet::::event_count()); }); + assert!(scanned.get() >= 7, "the remarks must have emitted events"); let weight_after = frame_system::Pallet::::block_weight().total(); assert_eq!( weight_after.saturating_sub(weight_before), - WormholeProofRecorderExtension::::per_transfer_weight(), - "the uncounted recorded transfer must be registered as extra block weight" + WormholeProofRecorderExtension::::event_scan_weight(scanned.get()), + "the per-event decode work of the scan must be registered as block weight" ); }); } From 9dffec2615d5e45eedadfd1d1188e29cd87271c6 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 16:58:16 +0800 Subject: [PATCH 16/17] Security review: stop depositing RuntimeEnvironmentUpdated (QPoW digest budget) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The QPoW header commits a fixed 110-byte digest window that the pre-runtime item and seal fill exactly, and import rejects anything larger. Upstream frame-system's RuntimeEnvironmentUpdated deposit on set_code/set_heap_pages therefore made every environment-changing block unimportable network-wide — runtime upgrades could not be finalized through normal block production. Nothing in the node stack consumes the item (clients detect upgrades from the :code state key, not the digest), so remove both deposits in the fork, document the no-runtime-digest-items invariant on deposit_log and DIGEST_LOGS_SIZE, and flip the pallet tests to assert the digest stays empty. Co-authored-by: Cursor --- pallets/frame-system/src/lib.rs | 30 ++++++++++++++++++++++++++++-- pallets/frame-system/src/tests.rs | 29 ++++++++++++++++------------- primitives/header/src/lib.rs | 7 +++++++ 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/pallets/frame-system/src/lib.rs b/pallets/frame-system/src/lib.rs index ed5be494..96c74dc7 100644 --- a/pallets/frame-system/src/lib.rs +++ b/pallets/frame-system/src/lib.rs @@ -735,7 +735,12 @@ pub mod pallet { // 65536 pages (4 GiB) is the wasm32 linear-memory hard maximum. ensure!((64..=65536).contains(&pages), Error::::InvalidHeapPages); storage::unhashed::put_raw(well_known_keys::HEAP_PAGES, &pages.encode()); - Self::deposit_log(generic::DigestItem::RuntimeEnvironmentUpdated); + // NOTE: upstream deposits `DigestItem::RuntimeEnvironmentUpdated` here. This + // fork must not: the QPoW header commits a fixed digest window that the + // pre-runtime item and seal fill exactly, so ANY runtime-deposited digest + // item makes the sealed header unimportable network-wide (see `deposit_log`). + // Nothing in the node stack consumes the item — clients detect environment + // changes from the `:heappages`/`:code` state keys, not the digest. Ok(().into()) } @@ -1622,7 +1627,14 @@ impl Pallet { /// the storage (for instance in case of parachains). pub fn update_code_in_storage(code: &[u8]) { storage::unhashed::put_raw(well_known_keys::CODE, code); - Self::deposit_log(generic::DigestItem::RuntimeEnvironmentUpdated); + // NOTE: upstream deposits `DigestItem::RuntimeEnvironmentUpdated` here. This + // fork must not: the QPoW header commits a fixed digest window that the + // pre-runtime item and seal fill exactly, so ANY runtime-deposited digest + // item makes the sealed header unimportable network-wide (see `deposit_log`). + // A runtime upgrade would then be un-includable through normal block + // production. Clients detect the new code from the `:code` state key (the + // executor's module cache is keyed by code hash); the `CodeUpdated` event + // below remains for observability. Self::deposit_event(Event::CodeUpdated); } @@ -2102,6 +2114,20 @@ impl Pallet { } /// Deposits a log and ensures it matches the block's log data. + /// + /// # WARNING: the QPoW digest window has no spare capacity + /// + /// `qp_header::Header::hash()` commits the digest through a fixed + /// `DIGEST_LOGS_SIZE` window that the client-injected pre-runtime item plus the + /// PoW seal fill **exactly**, and block import rejects any sealed header whose + /// encoded digest exceeds it (truncating would let distinct headers share a + /// hash). A digest item deposited from runtime code therefore does not fail the + /// call — it makes the finished block **unimportable by the entire network**, + /// silently, after mining. This is why the fork's `set_code` / + /// `set_heap_pages` paths do not deposit `RuntimeEnvironmentUpdated` the way + /// upstream does. Do not deposit digest items from runtime logic unless the + /// header format and the wormhole circuit's digest field are resized in the + /// same release. pub fn deposit_log(item: generic::DigestItem) { >::append(item); } diff --git a/pallets/frame-system/src/tests.rs b/pallets/frame-system/src/tests.rs index 35a92df8..e9545a2f 100644 --- a/pallets/frame-system/src/tests.rs +++ b/pallets/frame-system/src/tests.rs @@ -653,7 +653,8 @@ fn set_code_checks_works() { ext.execute_with(|| { let res = System::set_code(RawOrigin::Root.into(), vec![1, 2, 3, 4]); - assert_runtime_updated_digest(if res.is_ok() { 1 } else { 0 }); + // Success or failure, no digest item may be deposited (QPoW window). + assert_no_deposited_digest_items(); assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res); }); } @@ -755,15 +756,16 @@ fn validate_unsigned_apply_authorized_upgrade_honors_check_version() { } } -fn assert_runtime_updated_digest(num: usize) { +/// The QPoW header commits a fixed digest window that the pre-runtime item and +/// seal fill exactly, so a runtime-deposited digest item (like upstream's +/// `RuntimeEnvironmentUpdated`) makes the sealed block unimportable +/// network-wide. Environment-changing calls must deposit NO digest items. +fn assert_no_deposited_digest_items() { assert_eq!( - System::digest() - .logs - .into_iter() - .filter(|item| *item == generic::DigestItem::RuntimeEnvironmentUpdated) - .count(), - num, - "Incorrect number of Runtime Updated digest items", + System::digest().logs, + alloc::vec::Vec::new(), + "runtime code must not deposit digest items: the QPoW digest window has \ + no spare capacity and the sealed block would be rejected at import", ); } @@ -810,12 +812,12 @@ fn extrinsics_root_is_calculated_correctly() { } #[test] -fn runtime_updated_digest_emitted_when_heap_pages_changed() { +fn no_digest_item_deposited_when_heap_pages_changed() { new_test_ext().execute_with(|| { System::reset_events(); System::initialize(&1, &[0u8; 32].into(), &Default::default()); System::set_heap_pages(RawOrigin::Root.into(), 64).unwrap(); - assert_runtime_updated_digest(1); + assert_no_deposited_digest_items(); }); } @@ -834,10 +836,11 @@ fn set_heap_pages_validates_range() { ); } - // Both bounds of the allowed range are accepted and still emit the digest item. + // Both bounds of the allowed range are accepted, without depositing any + // digest item (the QPoW digest window has no spare capacity). assert_ok!(System::set_heap_pages(RawOrigin::Root.into(), 64)); assert_ok!(System::set_heap_pages(RawOrigin::Root.into(), 65536)); - assert_runtime_updated_digest(2); + assert_no_deposited_digest_items(); }); } diff --git a/primitives/header/src/lib.rs b/primitives/header/src/lib.rs index 0525b62b..16d3c0d3 100644 --- a/primitives/header/src/lib.rs +++ b/primitives/header/src/lib.rs @@ -43,6 +43,13 @@ use serde::{Deserialize, Serialize}; /// import rather than silently truncated; see the digest length check in /// `sc-consensus-qpow`. Truncation would let two distinct headers share a block /// hash on the bytes past this window. +/// +/// Because the window has no slack, the runtime must never deposit digest items +/// of its own: even a 1-byte item (e.g. upstream frame-system's +/// `RuntimeEnvironmentUpdated` on `set_code`) pushes the sealed digest to 111 +/// bytes and makes the block unimportable network-wide. The vendored +/// frame-system's deposits were removed for exactly this reason — see the +/// warning on `frame_system::Pallet::deposit_log`. pub const DIGEST_LOGS_SIZE: usize = 110; /// Extension trait for headers that support ZK tree root. From d28616a85f4bb420205d70f39f77628ee75a9dce Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 7 Aug 2026 17:03:41 +0800 Subject: [PATCH 17/17] fmt --- pallets/wormhole/src/tests.rs | 8 ++-- pallets/zk-tree/src/lib.rs | 22 +++++----- runtime/src/transaction_extensions.rs | 63 ++++++++++----------------- 3 files changed, 37 insertions(+), 56 deletions(-) diff --git a/pallets/wormhole/src/tests.rs b/pallets/wormhole/src/tests.rs index 98353625..dd4f3fc4 100644 --- a/pallets/wormhole/src/tests.rs +++ b/pallets/wormhole/src/tests.rs @@ -131,9 +131,11 @@ mod wormhole_tests { assert_eq!(Wormhole::potential_wormhole_balance(), 0); // Sanity: the same credit with a nonzero amount is recorded. - assert!(>::record_transfer_proof( - None, from, to, 1, - )); + assert!( + >::record_transfer_proof( + None, from, to, 1, + ) + ); assert_eq!(ZkTree::leaf_count(), 1); }); } diff --git a/pallets/zk-tree/src/lib.rs b/pallets/zk-tree/src/lib.rs index 8c819908..efcee98f 100644 --- a/pallets/zk-tree/src/lib.rs +++ b/pallets/zk-tree/src/lib.rs @@ -50,18 +50,16 @@ mod tests; /// raises `MAX_DEPTH` and a runtime upgrade embeds the regenerated verifiers. /// /// This is a deliberate "fix it when we get close" trade-off, not an oversight: -/// - Timeline: at one leaf per block (the mining-reward floor, 12s blocks) depth 16 -/// lasts ~1,600 years; at a sustained 10 transfers/sec chain-wide it lasts ~13 years; -/// even permanently saturated blocks (~50 tps) give ~2.5 years. Each +1 of circuit -/// depth quadruples capacity. -/// - Observability: `LeafCount` is public storage, so exhaustion is visible years in -/// advance; alert well before 4^16 leaves. -/// - The update itself: bump `MAX_DEPTH` in `qp-zk-circuits-common`, release the -/// circuit crates, let `pallets/wormhole/build.rs` regenerate the embedded verifier -/// binaries, regenerate proof fixtures, re-benchmark, and ship a runtime upgrade — -/// days of engineering inside a normal release cycle. Old proofs are invalidated by -/// the circuit change; nullifier state is unaffected, so nothing can double-spend -/// across the upgrade. +/// - Timeline: at one leaf per block (the mining-reward floor, 12s blocks) depth 16 lasts ~1,600 +/// years; at a sustained 10 transfers/sec chain-wide it lasts ~13 years; even permanently +/// saturated blocks (~50 tps) give ~2.5 years. Each +1 of circuit depth quadruples capacity. +/// - Observability: `LeafCount` is public storage, so exhaustion is visible years in advance; alert +/// well before 4^16 leaves. +/// - The update itself: bump `MAX_DEPTH` in `qp-zk-circuits-common`, release the circuit crates, +/// let `pallets/wormhole/build.rs` regenerate the embedded verifier binaries, regenerate proof +/// fixtures, re-benchmark, and ship a runtime upgrade — days of engineering inside a normal +/// release cycle. Old proofs are invalidated by the circuit change; nullifier state is +/// unaffected, so nothing can double-spend across the upgrade. pub const MAX_TREE_DEPTH: u8 = 32; /// Worst-case `(reads, writes)` storage-operation counts for one [`Pallet::insert_leaf`] diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 72c79eca..c15a8db4 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -88,25 +88,24 @@ impl /// - After successful execution, scans for `Transfer`, `Minted`, `TransferOnHold` and /// `ReserveRepatriated` events /// - Records proofs for any transfers that were sent TO a wormhole account -/// - Automatically catches ALL transfers dispatched inside a transaction, regardless of -/// how they're initiated: +/// - Automatically catches ALL transfers dispatched inside a transaction, regardless of how they're +/// initiated: /// - Direct transfers (transfer, transfer_keep_alive, transfer_all, etc.) /// - Batch transfers (utility.batch, batch_all, force_batch) /// - Multisig transfers (multisig.execute) /// - Recovery transfers (recovery.as_recovered) -/// - Held-fund seizures/recoveries (reversible_transfers.cancel / recover_funds, -/// which move value with `transfer_on_hold` instead of a free-balance transfer) -/// - Recovery-deposit seizures (recovery.close_recovery, which moves the rescuer's -/// deposit with `repatriate_reserved`) -/// - Future call-based mechanisms automatically covered, since wrapper calls emit -/// their inner events within the same extrinsic's event range +/// - Held-fund seizures/recoveries (reversible_transfers.cancel / recover_funds, which move value +/// with `transfer_on_hold` instead of a free-balance transfer) +/// - Recovery-deposit seizures (recovery.close_recovery, which moves the rescuer's deposit with +/// `repatriate_reserved`) +/// - Future call-based mechanisms automatically covered, since wrapper calls emit their inner +/// events within the same extrinsic's event range /// /// COVERAGE BOUNDARY: transaction extensions only run for transactions, so this scan /// never sees events emitted from hooks (`on_initialize` / `on_finalize`). Every /// hook-context credit therefore needs — and has — an explicit /// `TransferProofRecorder::record_transfer_proof` call instead: -/// - reversible-transfers' scheduled execution records its transfer in -/// `do_execute_transfer`; +/// - reversible-transfers' scheduled execution records its transfer in `do_execute_transfer`; /// - mining rewards and the treasury share record theirs in `on_finalize` /// (`pallet_mining_rewards`), using eventless `increase_balance` credits. /// @@ -402,14 +401,14 @@ impl TransactionEx // `weight()` and are therefore registered against the block post-hoc (this // keeps block-capacity accounting sound; it is not fee-charged): // - // 1. The event scan itself: any call can emit events the scan must decode - // (e.g. batched `remark_with_event`), and the decode cost is per record - // present at scan time — see `event_scan_weight`. + // 1. The event scan itself: any call can emit events the scan must decode (e.g. batched + // `remark_with_event`), and the decode cost is per record present at scan time — see + // `event_scan_weight`. // // 2. Recording shortfall: wrappers that dispatch inner calls stored on-chain - // (`Multisig::execute`, `ReversibleTransfers::recover_funds`, ...) can emit - // transfer events the static `count_transfers` matcher cannot see, so the - // proof-recording work above may exceed the weight reserved by `weight()`. + // (`Multisig::execute`, `ReversibleTransfers::recover_funds`, ...) can emit transfer + // events the static `count_transfers` matcher cannot see, so the proof-recording + // work above may exceed the weight reserved by `weight()`. let mut extra = Self::event_scan_weight(events_at_scan); if recorded > charged_transfers { extra = extra.saturating_add( @@ -418,9 +417,7 @@ impl TransactionEx ); } if extra != Weight::zero() { - frame_system::Pallet::::register_extra_weight_unchecked( - extra, info.class, - ); + frame_system::Pallet::::register_extra_weight_unchecked(extra, info.class); } } @@ -937,10 +934,7 @@ mod tests { let (tree_reads, tree_writes) = pallet_zk_tree::insert_leaf_db_ops_at_depth(20); let db_time = ::DbWeight::get() - .reads_writes( - 5u64.saturating_add(tree_reads), - 2u64.saturating_add(tree_writes), - ) + .reads_writes(5u64.saturating_add(tree_reads), 2u64.saturating_add(tree_writes)) .ref_time(); assert!( weight.ref_time() >= @@ -1024,10 +1018,7 @@ mod tests { let scanned = core::cell::Cell::new(0u32); run_lifecycle(&alice(), call, || { for i in 0..7u8 { - assert_ok!(System::remark_with_event( - RuntimeOrigin::signed(alice()), - vec![i], - )); + assert_ok!(System::remark_with_event(RuntimeOrigin::signed(alice()), vec![i],)); } scanned.set(frame_system::Pallet::::event_count()); }); @@ -1223,8 +1214,7 @@ mod tests { amount, )); let tx_id = - pallet_reversible_transfers::PendingTransfersBySender::::get(charlie()) - [0]; + pallet_reversible_transfers::PendingTransfersBySender::::get(charlie())[0]; // The guardian cancels: the held funds (minus the volume fee) are seized to // the guardian via `transfer_on_hold`, which emits `Balances::TransferOnHold` @@ -1232,20 +1222,14 @@ mod tests { // on the guardian's free balance, so the recorder must create a leaf for it // exactly as it would for a plain transfer. let events_before = frame_system::Pallet::::event_count(); - assert_ok!(ReversibleTransfers::cancel( - RuntimeOrigin::signed(guardian.clone()), - tx_id - )); + assert_ok!(ReversibleTransfers::cancel(RuntimeOrigin::signed(guardian.clone()), tx_id)); let recorded = WormholeProofRecorderExtension::::record_proofs_from_events_since( events_before, ); - assert_eq!( - recorded, 1, - "hold-transfer seizure must be recorded as a transfer proof" - ); + assert_eq!(recorded, 1, "hold-transfer seizure must be recorded as a transfer proof"); assert_eq!(Wormhole::transfer_count(&guardian), count_before + 1); }); } @@ -1289,10 +1273,7 @@ mod tests { events_before, ); - assert_eq!( - recorded, 1, - "reserve repatriation must be recorded as a transfer proof" - ); + assert_eq!(recorded, 1, "reserve repatriation must be recorded as a transfer proof"); assert_eq!(Wormhole::transfer_count(&alice()), count_before + 1); }); }