diff --git a/audits/quantus-substrate-audit-20260421.pdf b/audits/quantus-substrate-audit-20260513.pdf similarity index 61% rename from audits/quantus-substrate-audit-20260421.pdf rename to audits/quantus-substrate-audit-20260513.pdf index 0d9da88a..435ffac1 100644 Binary files a/audits/quantus-substrate-audit-20260421.pdf and b/audits/quantus-substrate-audit-20260513.pdf differ diff --git a/pallets/mining-rewards/src/lib.rs b/pallets/mining-rewards/src/lib.rs index 4229531f..a9b0ff27 100644 --- a/pallets/mining-rewards/src/lib.rs +++ b/pallets/mining-rewards/src/lib.rs @@ -106,6 +106,15 @@ pub mod pallet { /// The reward amount redirected to treasury reward: BalanceOf, }, + /// Treasury mint failed - reward was not issued. + /// + /// This is a critical operational signal. If this event is observed, it indicates + /// either a misconfigured treasury account or a currency invariant violation. + /// The reward amount was permanently lost (not minted). + TreasuryMintFailed { + /// The reward amount that failed to mint + reward: BalanceOf, + }, } #[pallet::hooks] @@ -256,6 +265,7 @@ pub mod pallet { "Failed to redirect {:?} to treasury: {:?}", reward, e2 ); + Self::deposit_event(Event::TreasuryMintFailed { reward }); }, } }, @@ -278,6 +288,7 @@ pub mod pallet { "Failed to mint {:?} to treasury: {:?}", reward, e ); + Self::deposit_event(Event::TreasuryMintFailed { reward }); }, } }, diff --git a/pallets/multisig/src/benchmarking.rs b/pallets/multisig/src/benchmarking.rs index f1cd2df1..57dca2a0 100644 --- a/pallets/multisig/src/benchmarking.rs +++ b/pallets/multisig/src/benchmarking.rs @@ -140,17 +140,14 @@ mod benchmarks { status: ProposalStatus, deposit: crate::BalanceOf, ) { - use frame_support::dispatch::GetDispatchInfo; let system_call = frame_system::Call::::remark { remark: vec![1u8; call_size as usize] }; let runtime_call = ::RuntimeCall::from(system_call); - let call_weight = runtime_call.get_dispatch_info().call_weight; let encoded = runtime_call.encode(); let bounded_call: BoundedCallOf = encoded.try_into().unwrap(); let bounded_approvals: BoundedApprovalsOf = approvals.to_vec().try_into().unwrap(); let proposal_data = ProposalDataOf:: { proposer: proposer.clone(), call: bounded_call, - call_weight, expiry, approvals: bounded_approvals, deposit, diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index 0d217013..fcd2c3ce 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -44,7 +44,7 @@ mod tests; pub mod weights; use codec::{Decode, Encode, MaxEncodedLen}; -use frame_support::{traits::Get, weights::Weight, BoundedBTreeMap, BoundedVec}; +use frame_support::{traits::Get, BoundedBTreeMap, BoundedVec}; use scale_info::TypeInfo; use sp_runtime::RuntimeDebug; @@ -106,8 +106,6 @@ pub struct ProposalData::RuntimeCall::decode(&mut &proposal.call[..]) .map_err(|_| Self::err_with_weight_raw(Error::::InvalidCall, 2))?; + // Re-check call weight at execute time (belt-and-suspenders). + // MaxInnerCallWeight could have been lowered via runtime upgrade since propose time. + 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); + } + // 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. @@ -1078,7 +1087,7 @@ pub mod pallet { return Self::err_with_weight(Error::::CallNotAllowedForHighSecurityMultisig, 2); } - // Use the stored call weight (validated at propose time) + // 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) @@ -1103,11 +1112,12 @@ pub mod pallet { result: result.as_ref().map(|_| ()).map_err(|e| e.error), }); - // Calculate actual weight: bookkeeping + inner call's actual weight - // Use stored call_weight as fallback when post-dispatch info is unavailable + // Calculate actual weight: bookkeeping + inner call's actual weight. + // Use current_call_weight (recomputed at execute time) as fallback when + // post-dispatch info is unavailable. let actual_call_weight = match &result { Ok(info) | Err(DispatchErrorWithPostInfo { post_info: info, .. }) => - info.actual_weight.unwrap_or(proposal.call_weight), + info.actual_weight.unwrap_or(current_call_weight), }; let total_weight = bookkeeping_weight.saturating_add(actual_call_weight); diff --git a/pallets/multisig/src/mock.rs b/pallets/multisig/src/mock.rs index 8a6a223c..0c7d38ac 100644 --- a/pallets/multisig/src/mock.rs +++ b/pallets/multisig/src/mock.rs @@ -7,7 +7,7 @@ use core::{cell::RefCell, marker::PhantomData}; use crate as pallet_multisig; use frame_support::{ derive_impl, ord_parameter_types, parameter_types, - traits::{ConstU32, EitherOfDiverse, EqualPrivilegeOnly, Time}, + traits::{ConstU32, EitherOfDiverse, EqualPrivilegeOnly, Get, Time}, PalletId, }; use frame_system::{limits::BlockWeights, EnsureRoot, EnsureSignedBy}; @@ -155,8 +155,28 @@ parameter_types! { pub const ProposalFeeParam: Balance = 999; pub const SignerStepFactorParam: Permill = Permill::from_parts(10_000); pub const MaxExpiryDurationParam: u64 = 10000; - // 1 billion ref_time, 1 MB proof_size - generous limit for testing - pub const MaxInnerCallWeightParam: Weight = Weight::from_parts(1_000_000_000, 1_048_576); +} + +// Dynamic MaxInnerCallWeight for testing runtime upgrade scenarios +thread_local! { + static MAX_INNER_CALL_WEIGHT: RefCell = RefCell::new(Weight::from_parts(1_000_000_000, 1_048_576)); +} + +pub struct DynamicMaxInnerCallWeight; +impl Get for DynamicMaxInnerCallWeight { + fn get() -> Weight { + MAX_INNER_CALL_WEIGHT.with(|v| *v.borrow()) + } +} + +/// Set the max inner call weight (for testing runtime upgrade scenarios) +pub fn set_max_inner_call_weight(weight: Weight) { + MAX_INNER_CALL_WEIGHT.with(|v| *v.borrow_mut() = weight); +} + +/// Reset max inner call weight to default +pub fn reset_max_inner_call_weight() { + MAX_INNER_CALL_WEIGHT.with(|v| *v.borrow_mut() = Weight::from_parts(1_000_000_000, 1_048_576)); } impl pallet_multisig::Config for Test { @@ -170,7 +190,7 @@ impl pallet_multisig::Config for Test { type ProposalFee = ProposalFeeParam; type SignerStepFactor = SignerStepFactorParam; type MaxExpiryDuration = MaxExpiryDurationParam; - type MaxInnerCallWeight = MaxInnerCallWeightParam; + type MaxInnerCallWeight = DynamicMaxInnerCallWeight; type PalletId = MultisigPalletId; type WeightInfo = (); type HighSecurity = crate::tests::MockHighSecurity; diff --git a/pallets/multisig/src/tests.rs b/pallets/multisig/src/tests.rs index 2e6d1e9c..cc66c070 100644 --- a/pallets/multisig/src/tests.rs +++ b/pallets/multisig/src/tests.rs @@ -5,11 +5,11 @@ use codec::Encode; use frame_support::{ assert_noop, assert_ok, dispatch::GetDispatchInfo, - traits::{fungible::Mutate, Currency}, + traits::{fungible::Mutate, Currency, Get}, }; use qp_high_security::HighSecurityInspector; use sp_core::crypto::AccountId32; -use sp_runtime::DispatchError; +use sp_runtime::{DispatchError, Weight}; use std::{cell::RefCell, collections::BTreeSet}; // Thread-local storage for dynamically toggling high-security status in tests @@ -2299,7 +2299,7 @@ fn propose_rejects_call_exceeding_max_inner_weight() { assert!(too_heavy_call .get_dispatch_info() .call_weight - .any_gt(MaxInnerCallWeightParam::get())); + .any_gt(DynamicMaxInnerCallWeight::get())); let free_before_propose = Balances::free_balance(alice()); assert_err_ignore_postinfo( @@ -2413,3 +2413,61 @@ fn execute_allows_whitelisted_call_after_hs_enabled() { assert!(!Proposals::::contains_key(&multisig_address, 0)); }); } + +/// Test that execute rejects a proposal if MaxInnerCallWeight was lowered after propose +/// (simulates runtime upgrade scenario) +#[test] +fn execute_rejects_call_when_max_weight_lowered_after_propose() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + + // Reset to default weight limit + reset_max_inner_call_weight(); + + let signers = vec![alice(), bob()]; + assert_ok!(Multisig::create_multisig( + RuntimeOrigin::signed(alice()), + signers.clone(), + 2, + 0, + )); + let multisig_address = Multisig::derive_multisig_address(&signers, 2, 0); + + // Create a call with weight that's currently under the limit + // System::remark has very low weight, so it passes propose + let call = RuntimeCall::System(frame_system::Call::remark { remark: b"test".to_vec() }); + let call_weight = call.get_dispatch_info().call_weight; + + // Verify the call is under the current limit + assert!(!call_weight.any_gt(DynamicMaxInnerCallWeight::get())); + + // Propose succeeds + assert_ok!(Multisig::propose( + RuntimeOrigin::signed(alice()), + multisig_address.clone(), + call.encode().try_into().unwrap(), + 100, + )); + + // Bob approves - proposal is now ready for execution + assert_ok!(Multisig::approve(RuntimeOrigin::signed(bob()), multisig_address.clone(), 0,)); + + // Simulate runtime upgrade that lowers MaxInnerCallWeight to zero + set_max_inner_call_weight(Weight::zero()); + + // Now the call weight exceeds the (lowered) limit + assert!(call_weight.any_gt(DynamicMaxInnerCallWeight::get())); + + // Execute should fail with CallWeightExceedsLimit + assert_err_ignore_postinfo( + Multisig::execute(RuntimeOrigin::signed(alice()), multisig_address.clone(), 0), + Error::::CallWeightExceedsLimit.into(), + ); + + // Proposal should still exist (execute failed early) + assert!(Proposals::::contains_key(&multisig_address, 0)); + + // Reset for other tests + reset_max_inner_call_weight(); + }); +} diff --git a/pallets/reversible-transfers/src/lib.rs b/pallets/reversible-transfers/src/lib.rs index 697ff4cf..79f2ee9e 100644 --- a/pallets/reversible-transfers/src/lib.rs +++ b/pallets/reversible-transfers/src/lib.rs @@ -307,8 +307,6 @@ pub mod pallet { AccountNotHighSecurity, /// Guardian cannot be the account itself, because it is redundant. GuardianCannotBeSelf, - /// Recoverer cannot be the account itself, because it is redundant. - RecovererCannotBeSelf, /// The specified pending transaction ID was not found. PendingTxNotFound, /// The caller is not the original submitter of the transaction they are trying to cancel. @@ -321,8 +319,6 @@ pub mod pallet { SchedulingFailed, /// Failed to cancel the scheduled task with the scheduler pallet. CancellationFailed, - /// Failed to decode the OpaqueCall back into a RuntimeCall. - CallDecodingFailed, /// Call is invalid. InvalidCall, /// Invalid scheduler origin @@ -361,6 +357,12 @@ pub mod pallet { /// functionality. This design is intentional to provide maximum security guarantees: /// an attacker who gains access to the account cannot simply disable the protections. /// + /// This permanence also ensures that any funds subsequently sent to a compromised + /// account (e.g., from pending payments, contracts, or accidental deposits) remain + /// protected and can be recovered by the guardian via + /// [`recover_funds`](Self::recover_funds). The guardian can call `recover_funds` + /// repeatedly as needed. + /// /// Users who no longer wish to use high-security features can simply transfer their /// funds to a different account using [`schedule_transfer`](Self::schedule_transfer) /// or [`schedule_asset_transfer`](Self::schedule_asset_transfer). @@ -532,6 +534,15 @@ pub mod pallet { /// It cancels all pending transfers first (applying volume fees), then transfers /// the remaining free balance to the guardian. /// + /// # Repeated Recovery + /// + /// This function can be called multiple times on the same account. The high-security + /// status and guardian relationship are intentionally preserved after recovery, ensuring + /// that any funds subsequently deposited to the account (e.g., from pending payments, + /// contracts, or accidental deposits) remain protected and recoverable. + /// + /// # Error Handling + /// /// If releasing held funds fails for any transfer, that transfer is skipped (metadata /// preserved for manual retry via `cancel`) and a `TransferRecoveryFailed` event is /// emitted. Other transfers continue to be processed. @@ -556,38 +567,47 @@ pub mod pallet { PendingTransfersBySender::::get(&account).into_iter().collect(); for tx_id in pending_tx_ids.iter() { - if let Some(pending) = PendingTransfers::::get(tx_id) { - // Try to release held funds first - if let Err(e) = Self::release_held_funds_with_fee(&pending, &who, true) { + // PendingTransfersBySender and PendingTransfers should always be in sync. + // If not, this is an invariant violation - log defensively and skip. + let Some(pending) = PendingTransfers::::get(tx_id) else { + defensive!( + "PendingTransfersBySender/PendingTransfers inconsistency: \ + tx {:?} in sender index but not in PendingTransfers", + tx_id + ); + continue; + }; + + // Try to release held funds first + if let Err(e) = Self::release_held_funds_with_fee(&pending, &who, true) { + log::warn!( + "Failed to release held funds for tx {:?} during recovery: {:?}", + tx_id, + e + ); + // Skip - leave metadata intact for manual recovery via cancel + Self::deposit_event(Event::TransferRecoveryFailed { tx_id: *tx_id }); + continue; + } + + // Release succeeded - now remove metadata and cancel scheduler + PendingTransfers::::remove(tx_id); + + if let Some(id) = Self::make_schedule_id(tx_id).ok() { + if let Err(e) = T::Scheduler::cancel_named(id) { log::warn!( - "Failed to release held funds for tx {:?} during recovery: {:?}", + "Failed to cancel scheduled task for tx {:?}: {:?} (funds already released)", tx_id, e ); - // Skip - leave metadata intact for manual recovery via cancel - Self::deposit_event(Event::TransferRecoveryFailed { tx_id: *tx_id }); - continue; } - - // Release succeeded - now remove metadata and cancel scheduler - PendingTransfers::::remove(tx_id); - - if let Some(id) = Self::make_schedule_id(tx_id).ok() { - if let Err(e) = T::Scheduler::cancel_named(id) { - log::warn!( - "Failed to cancel scheduled task for tx {:?}: {:?} (funds already released)", - tx_id, - e - ); - } - } - - num_cancelled = num_cancelled.saturating_add(1); - Self::deposit_event(Event::TransactionCancelled { - who: who.clone(), - tx_id: *tx_id, - }); } + + num_cancelled = num_cancelled.saturating_add(1); + Self::deposit_event(Event::TransactionCancelled { + who: who.clone(), + tx_id: *tx_id, + }); } // Update sender index to only contain transfers that weren't cancelled diff --git a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs index 3ab52097..b95e58de 100644 --- a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs +++ b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs @@ -513,7 +513,7 @@ fn schedule_transfer_fails_not_reversible() { #[test] fn schedule_multiple_transfer_works() { new_test_ext().execute_with(|| { - let user = alice(); // User 1 is reversible from genesis with guardian=2, recoverer=3 + let user = alice(); // User 1 is high-security from genesis with guardian=2 let dest_user = bob(); let amount = 100; diff --git a/pallets/scheduler/src/lib.rs b/pallets/scheduler/src/lib.rs index 583eb715..55dcef8e 100644 --- a/pallets/scheduler/src/lib.rs +++ b/pallets/scheduler/src/lib.rs @@ -908,10 +908,13 @@ impl Pallet { (when, index): TaskAddressOf, ) -> Result<(), DispatchError> { let agenda = Agenda::::get(when); + // Use defensive check: callers are privileged (ScheduleOrigin) and for cancel_retry_named + // the address comes from Lookup, so a miss here indicates either stale input or + // Lookup/Agenda desync. let scheduled = agenda .get(index as usize) .and_then(Option::as_ref) - .ok_or(Error::::NotFound)?; + .defensive_ok_or(Error::::NotFound)?; Self::ensure_privilege(origin, &scheduled.origin)?; Retries::::remove((when, index)); Ok(()) diff --git a/pallets/treasury/src/lib.rs b/pallets/treasury/src/lib.rs index ffe290d7..6e4dbe68 100644 --- a/pallets/treasury/src/lib.rs +++ b/pallets/treasury/src/lib.rs @@ -1,8 +1,36 @@ #![cfg_attr(not(feature = "std"), no_std)] -//! Treasury configuration pallet. +//! # Treasury Configuration Pallet //! -//! Provides TreasuryProvider trait for mining-rewards integration. +//! This pallet provides a centralized surface for treasury-related runtime parameters +//! that can be adjusted by privileged origins (currently root/governance). +//! +//! ## Purpose & Rationale +//! +//! While the current functionality is limited to treasury account and portion settings +//! consumed by the `mining-rewards` pallet, this separation is intentional. The long-term +//! goal is to consolidate all "tunable knobs" that governance or a technical collective +//! should be able to adjust into a small, well-defined set of pallets. +//! +//! This architecture enables: +//! +//! - **Minimal privilege surface**: The technical collective's authority can be limited to a known +//! set of configuration parameters rather than arbitrary runtime calls. +//! - **Auditability**: All adjustable parameters are explicitly defined in dedicated pallets, +//! making it clear what can and cannot be changed post-genesis. +//! - **Future extensibility**: As the treasury subsystem grows (e.g., budgets, spending proposals, +//! vesting schedules), this pallet provides a natural home for that logic. +//! +//! ## Current Features +//! +//! - [`TreasuryAccount`]: The account that receives the treasury's share of mining rewards. +//! - [`TreasuryPortion`]: The percentage (as `Permill`) of mining rewards allocated to treasury. +//! - [`TreasuryProvider`] trait: Consumed by `mining-rewards` to query treasury configuration. +//! +//! ## Integration +//! +//! The `mining-rewards` pallet uses the [`TreasuryProvider`] trait to determine where and +//! how much of each block reward should be allocated to the treasury. pub mod weights; pub use weights::WeightInfo; diff --git a/runtime/tests/transactions/reversible_integration.rs b/runtime/tests/transactions/reversible_integration.rs index 2947e9ab..f3f021f2 100644 --- a/runtime/tests/transactions/reversible_integration.rs +++ b/runtime/tests/transactions/reversible_integration.rs @@ -20,7 +20,7 @@ fn high_security_end_to_end_flow() { // Accounts: // 1 = HS account (sender) // 2 = guardian - // 3 = recoverer (friend) + // 3 = third party (friend) // 4 = recipient of the initial transfer let mut ext = TestCommons::new_test_ext(); ext.execute_with(|| {