diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index 1f137c3c..62c0a0c1 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -438,16 +438,22 @@ pub mod pallet { ensure!(threshold > 0, Error::::ThresholdZero); ensure!(signers.len() >= 2, Error::::NotEnoughSigners); + // SECURITY: Bound raw input size BEFORE normalization to prevent DoS. + // An attacker could submit a huge duplicate-heavy vector; clone/sort/dedup + // is O(n log n) on the raw length, but benchmarks only cover MaxSigners. + // Reject oversized inputs before doing any expensive work. + // + // BREAKING CHANGE: This rejects inputs with len > MaxSigners even if they would + // deduplicate to ≤ MaxSigners. Previously such inputs were accepted. This is the + // correct security tradeoff: users must pre-deduplicate their signer lists. + ensure!(signers.len() <= T::MaxSigners::get() as usize, Error::::TooManySigners); + // Normalize signers: sort and deduplicate (single authoritative place) let normalized_signers = Self::normalize_signers(&signers); // Validate against normalized count (after dedup) - must have at least 2 unique signers ensure!(normalized_signers.len() >= 2, Error::::NotEnoughSigners); ensure!(threshold <= normalized_signers.len() as u32, Error::::ThresholdTooHigh); - ensure!( - normalized_signers.len() <= T::MaxSigners::get() as usize, - Error::::TooManySigners - ); // Generate deterministic multisig address from normalized signers let multisig_address = @@ -516,10 +522,23 @@ pub mod pallet { /// **For threshold=1:** The proposal is created with `Approved` status immediately /// and can be executed via `execute()` without additional approvals. /// - /// **Weight:** Charged upfront for worst-case (high-security path with decode). - /// Refunded to actual cost on success based on whether HS path was taken. + /// **Weight:** Charged upfront includes bookkeeping + MaxInnerCallWeight to cover + /// the cost of decoding arbitrary RuntimeCall structures and calling get_dispatch_info(). + /// On success, refunds based on actual inner call weight. On rejection after decode + /// (e.g., CallWeightExceedsLimit, CallNotAllowedForHighSecurityMultisig), the full + /// reserved weight is burned to prevent griefing with complex calls that get rejected. #[pallet::call_index(1)] - #[pallet::weight(::WeightInfo::propose_high_security(call.len() as u32))] + #[pallet::weight({ + // Bookkeeping weight + MaxInnerCallWeight to cover decode + get_dispatch_info cost. + // Structured RuntimeCall decode is O(inner_call_count), not O(bytes), so we must + // reserve weight for the worst-case inner call to prevent block time overruns. + // + // Note: MaxInnerCallWeight is a proxy for decode cost, not exact. Decode cost ≠ + // dispatch weight, but they're correlated (complex calls = more variants to decode). + // This is intentionally conservative: over-reservation is refunded on success. + ::WeightInfo::propose_high_security(call.len() as u32) + .saturating_add(T::MaxInnerCallWeight::get()) + })] #[allow(clippy::useless_conversion)] pub fn propose( origin: OriginFor, @@ -583,30 +602,28 @@ pub mod pallet { // ===== PHASE 3: Decode call (validates call is well-formed for ALL proposals) ===== // This catches malformed calls at propose time rather than execute time, // providing consistent error behavior for both HS and non-HS multisigs. - let decoded_call = - ::RuntimeCall::decode(&mut &call[..]).map_err(|_| { - DispatchErrorWithPostInfo { - post_info: PostDispatchInfo { - actual_weight: Some(T::DbWeight::get().reads(1)), - pays_fee: Pays::Yes, - }, - error: Error::::InvalidCall.into(), - } - })?; + // NOTE: Decode cost is O(inner_call_count) for nested calls, not O(bytes). + // On decode failure, we burn the full reserved weight to prevent griefing. + let decoded_call = ::RuntimeCall::decode(&mut &call[..]) + .map_err(|_| Self::err_burn_full_raw(Error::::InvalidCall))?; // ===== PHASE 3b: Check inner call weight against limit ===== // This ensures execute() can safely reserve weight at pre-dispatch time. let call_weight = decoded_call.get_dispatch_info().call_weight; let max_inner_weight = T::MaxInnerCallWeight::get(); if call_weight.any_gt(max_inner_weight) { - return Self::err_with_weight(Error::::CallWeightExceedsLimit, 1); + // Don't refund after decode - the expensive work (decode + get_dispatch_info) + // has already been done. Returning None burns the full reserved weight, + // preventing griefing with complex calls that get rejected. + return Self::err_burn_full(Error::::CallWeightExceedsLimit); } // ===== PHASE 4: High-security whitelist check (if applicable) ===== // (additional read: HighSecurityAccounts) let is_high_security = T::HighSecurity::is_high_security(&multisig_address); if is_high_security && !T::HighSecurity::is_whitelisted(&decoded_call) { - return Self::err_with_weight(Error::::CallNotAllowedForHighSecurityMultisig, 2); + // Don't refund after decode - same reasoning as above. + return Self::err_burn_full(Error::::CallNotAllowedForHighSecurityMultisig); } // Calculate dynamic fee based on number of signers @@ -692,12 +709,14 @@ pub mod pallet { }); } - // Refund weight: HS path was charged upfront, refund if non-HS - let actual_weight = if is_high_security { + // Calculate actual weight: bookkeeping + actual inner call weight (from + // get_dispatch_info). We reserved MaxInnerCallWeight upfront; refund the difference. + let bookkeeping_weight = if is_high_security { ::WeightInfo::propose_high_security(call_len) } else { ::WeightInfo::propose(call_len) }; + let actual_weight = bookkeeping_weight.saturating_add(call_weight); Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes }) } @@ -748,17 +767,18 @@ pub mod pallet { } })?; - // Calculate actual weight based on real call size + // Calculate actual weight based on real call size - use this for ALL paths + // after proposal is loaded, since reading the proposal incurs size-dependent cost. let actual_call_size = proposal.call.len() as u32; let actual_weight = ::WeightInfo::approve(actual_call_size); let current_block = frame_system::Pallet::::block_number(); if current_block > proposal.expiry { - return Self::err_with_weight(Error::::ProposalExpired, 2); + return Self::err_with_actual_weight(Error::::ProposalExpired, actual_weight); } if proposal.approvals.contains(&approver) { - return Self::err_with_weight(Error::::AlreadyApproved, 2); + return Self::err_with_actual_weight(Error::::AlreadyApproved, actual_weight); } // Add approval @@ -827,20 +847,23 @@ pub mod pallet { } })?; + // Calculate actual weight based on real call size - use for ALL paths + // after proposal is loaded, since reading the proposal incurs size-dependent cost. + let call_size = proposal.call.len() as u32; + let actual_weight = ::WeightInfo::cancel(call_size); + // Check if caller is the proposer (1 read already performed) if canceller != proposal.proposer { - return Self::err_with_weight(Error::::NotProposer, 1); + return Self::err_with_actual_weight(Error::::NotProposer, actual_weight); } // Check if proposal is cancellable (Active or Approved) if proposal.status != ProposalStatus::Active && proposal.status != ProposalStatus::Approved { - return Self::err_with_weight(Error::::ProposalNotActive, 1); + return Self::err_with_actual_weight(Error::::ProposalNotActive, actual_weight); } - let call_size = proposal.call.len() as u32; - // Remove proposal from storage and return deposit immediately Self::remove_proposal_and_return_deposit( &multisig_address, @@ -856,7 +879,6 @@ pub mod pallet { proposal_id, }); - let actual_weight = ::WeightInfo::cancel(call_size); Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes }) } @@ -906,19 +928,24 @@ pub mod pallet { } })?; + // Calculate actual weight based on real call size - use for ALL paths + // after proposal is loaded, since reading the proposal incurs size-dependent cost. + let call_size = proposal.call.len() as u32; + let actual_weight = ::WeightInfo::remove_expired(call_size); + // Active or Approved proposals can be removed when expired (Executed/Cancelled // are auto-removed). Approved+expired would otherwise be stuck if proposer // unavailable. if proposal.status != ProposalStatus::Active && proposal.status != ProposalStatus::Approved { - return Self::err_with_weight(Error::::ProposalNotActive, 2); + return Self::err_with_actual_weight(Error::::ProposalNotActive, actual_weight); } // Check if expired let current_block = frame_system::Pallet::::block_number(); if current_block <= proposal.expiry { - return Self::err_with_weight(Error::::ProposalNotExpired, 2); + return Self::err_with_actual_weight(Error::::ProposalNotExpired, actual_weight); } // Remove proposal from storage and return deposit @@ -937,9 +964,6 @@ pub mod pallet { removed_by: caller, }); - // Return actual weight based on proposal call size - let actual_weight = - ::WeightInfo::remove_expired(proposal.call.len() as u32); Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes }) } @@ -1065,42 +1089,53 @@ pub mod pallet { } })?; + // Calculate bookkeeping weight based on real call size - use for ALL paths + // after proposal is loaded, since reading the proposal incurs size-dependent cost. + let call_size = proposal.call.len() as u32; + let bookkeeping_weight = ::WeightInfo::execute(call_size); + // Must be Approved status if proposal.status != ProposalStatus::Approved { - return Self::err_with_weight(Error::::ProposalNotApproved, 2); + return Self::err_with_actual_weight( + Error::::ProposalNotApproved, + bookkeeping_weight, + ); } // Must not be expired let current_block = frame_system::Pallet::::block_number(); if current_block > proposal.expiry { - return Self::err_with_weight(Error::::ProposalExpired, 2); + return Self::err_with_actual_weight( + Error::::ProposalExpired, + bookkeeping_weight, + ); } // Decode the call + // After decode, we've done size-dependent work, so failures should burn full weight. let call = ::RuntimeCall::decode(&mut &proposal.call[..]) - .map_err(|_| Self::err_with_weight_raw(Error::::InvalidCall, 2))?; + .map_err(|_| Self::err_burn_full_raw(Error::::InvalidCall))?; // Re-check call weight at execute time (belt-and-suspenders). // MaxInnerCallWeight could have been lowered via runtime upgrade since propose time. + // After decode + get_dispatch_info, don't refund - burn the full reserved weight. let current_call_weight = call.get_dispatch_info().call_weight; let max_inner_weight = T::MaxInnerCallWeight::get(); if current_call_weight.any_gt(max_inner_weight) { - return Self::err_with_weight(Error::::CallWeightExceedsLimit, 2); + return Self::err_burn_full(Error::::CallWeightExceedsLimit); } // Re-check high-security whitelist at execute time. // The multisig's HS status may have changed since the proposal was created, // or the whitelist may have been updated via runtime upgrade. // This prevents bypassing HS restrictions by proposing before enabling HS. + // After decode + get_dispatch_info, don't refund - burn the full reserved weight. if T::HighSecurity::is_high_security(&multisig_address) && !T::HighSecurity::is_whitelisted(&call) { - return Self::err_with_weight(Error::::CallNotAllowedForHighSecurityMultisig, 2); + return Self::err_burn_full(Error::::CallNotAllowedForHighSecurityMultisig); } - // Calculate bookkeeping weight based on call size - let bookkeeping_weight = Self::bookkeeping_weight(proposal.call.len() as u32); - // EFFECTS: Remove proposal and return deposit BEFORE dispatch (reentrancy protection) Self::remove_proposal_and_return_deposit( &multisig_address, @@ -1142,7 +1177,7 @@ pub mod pallet { impl Pallet { /// Return an error with actual weight consumed instead of charging full upfront weight. - /// Use for early exits where minimal work was performed. + /// Use for early exits where minimal work was performed (only DB reads). fn err_with_weight(error: Error, reads: u64) -> DispatchResultWithPostInfo { Err(DispatchErrorWithPostInfo { post_info: PostDispatchInfo { @@ -1153,21 +1188,29 @@ pub mod pallet { }) } - /// Return a raw DispatchErrorWithPostInfo (not wrapped in Result). - /// Use when you need to map_err with a custom error. - fn err_with_weight_raw(error: Error, reads: u64) -> DispatchErrorWithPostInfo { - DispatchErrorWithPostInfo { - post_info: PostDispatchInfo { - actual_weight: Some(T::DbWeight::get().reads(reads)), - pays_fee: Pays::Yes, - }, + /// Return an error with a specific actual weight. + /// Use for failures after size-dependent work (e.g., after loading a proposal). + fn err_with_actual_weight(error: Error, weight: Weight) -> DispatchResultWithPostInfo { + Err(DispatchErrorWithPostInfo { + post_info: PostDispatchInfo { actual_weight: Some(weight), pays_fee: Pays::Yes }, error: error.into(), - } + }) } - /// Returns the multisig bookkeeping weight for execute (excludes inner call weight). - fn bookkeeping_weight(call_size: u32) -> Weight { - ::WeightInfo::execute(call_size) + /// Return an error that burns the full reserved weight (no refund). + /// Use for failures after expensive work like call decoding where we want to + /// prevent griefing with complex calls that get rejected. + fn err_burn_full(error: Error) -> DispatchResultWithPostInfo { + Err(Self::err_burn_full_raw(error)) + } + + /// Return a raw DispatchErrorWithPostInfo that burns the full reserved weight. + /// Use in map_err closures where the raw error type is needed. + fn err_burn_full_raw(error: Error) -> DispatchErrorWithPostInfo { + DispatchErrorWithPostInfo { + post_info: PostDispatchInfo { actual_weight: None, pays_fee: Pays::Yes }, + error: error.into(), + } } /// Normalize signers: sort and deduplicate. diff --git a/pallets/multisig/src/tests.rs b/pallets/multisig/src/tests.rs index a76ed577..06358f7c 100644 --- a/pallets/multisig/src/tests.rs +++ b/pallets/multisig/src/tests.rs @@ -293,6 +293,31 @@ fn create_multisig_deduplicates_signers() { }); } +/// Regression test: raw signer input is bounded BEFORE deduplication to prevent DoS. +/// An attacker could submit a huge duplicate-heavy vector that would dedup to ≤ MaxSigners, +/// but the sort/dedup cost is O(n log n) on the raw length. We reject oversized raw inputs +/// even if they would normalize to a valid count. +#[test] +fn create_multisig_rejects_oversized_raw_input_even_if_would_dedup_to_valid() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + let creator = alice(); + + // MaxSigners is 10 in mock. Create 11 signers that would dedup to just 2. + // Old behavior: would accept (dedup first, then check). + // New behavior: rejects immediately (check raw length first). + let signers = + vec![bob(), bob(), bob(), bob(), bob(), bob(), bob(), bob(), bob(), bob(), charlie()]; // 11 elements, but only 2 unique + assert_eq!(signers.len(), 11); + + // Should fail with TooManySigners even though dedup would yield only 2 + assert_noop!( + Multisig::create_multisig(RuntimeOrigin::signed(creator), signers, 2, 0), + Error::::TooManySigners + ); + }); +} + #[test] fn create_multiple_multisigs_increments_nonce() { new_test_ext().execute_with(|| { diff --git a/pallets/multisig/src/weights.rs b/pallets/multisig/src/weights.rs index 35519633..9a1432c6 100644 --- a/pallets/multisig/src/weights.rs +++ b/pallets/multisig/src/weights.rs @@ -65,7 +65,14 @@ pub struct SubstrateWeight(PhantomData); impl WeightInfo for SubstrateWeight { /// Storage: `Multisig::Multisigs` (r:1 w:1) /// Proof: `Multisig::Multisigs` (`max_values`: None, `max_size`: Some(6892), added: 9367, mode: `MaxEncodedLen`) + /// Storage: `System::Account` (r:1 w:0) + /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) + /// Storage: `Wormhole::PotentialWormholeBalance` (r:1 w:1) + /// Proof: `Wormhole::PotentialWormholeBalance` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) /// The range of component `s` is `[2, 100]`. + /// + /// Note: Weight includes worst-case Wormhole reveal_address path where a pre-funded + /// multisig address has nonzero balance, triggering PotentialWormholeBalance mutation. fn create_multisig(s: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `42` @@ -74,8 +81,11 @@ impl WeightInfo for SubstrateWeight { Weight::from_parts(20_952_861, 10357) // Standard Error: 991 .saturating_add(Weight::from_parts(17_405, 0).saturating_mul(s.into())) - .saturating_add(T::DbWeight::get().reads(1_u64)) - .saturating_add(T::DbWeight::get().writes(1_u64)) + // Base: 1 read (Multisigs) + 1 write (Multisigs) + // Wormhole reveal_address worst-case: 1 read (System::Account for balance) + // + 1 read (PotentialWormholeBalance) + 1 write (PotentialWormholeBalance) + .saturating_add(T::DbWeight::get().reads(3_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) } /// Storage: `Multisig::Multisigs` (r:1 w:1) /// Proof: `Multisig::Multisigs` (`max_values`: None, `max_size`: Some(6892), added: 9367, mode: `MaxEncodedLen`) @@ -212,7 +222,14 @@ impl WeightInfo for SubstrateWeight { impl WeightInfo for () { /// Storage: `Multisig::Multisigs` (r:1 w:1) /// Proof: `Multisig::Multisigs` (`max_values`: None, `max_size`: Some(6892), added: 9367, mode: `MaxEncodedLen`) + /// Storage: `System::Account` (r:1 w:0) + /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) + /// Storage: `Wormhole::PotentialWormholeBalance` (r:1 w:1) + /// Proof: `Wormhole::PotentialWormholeBalance` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) /// The range of component `s` is `[2, 100]`. + /// + /// Note: Weight includes worst-case Wormhole reveal_address path where a pre-funded + /// multisig address has nonzero balance, triggering PotentialWormholeBalance mutation. fn create_multisig(s: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `42` @@ -221,8 +238,11 @@ impl WeightInfo for () { Weight::from_parts(20_952_861, 10357) // Standard Error: 991 .saturating_add(Weight::from_parts(17_405, 0).saturating_mul(s.into())) - .saturating_add(RocksDbWeight::get().reads(1_u64)) - .saturating_add(RocksDbWeight::get().writes(1_u64)) + // Base: 1 read (Multisigs) + 1 write (Multisigs) + // Wormhole reveal_address worst-case: 1 read (System::Account for balance) + // + 1 read (PotentialWormholeBalance) + 1 write (PotentialWormholeBalance) + .saturating_add(RocksDbWeight::get().reads(3_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) } /// Storage: `Multisig::Multisigs` (r:1 w:1) /// Proof: `Multisig::Multisigs` (`max_values`: None, `max_size`: Some(6892), added: 9367, mode: `MaxEncodedLen`)