Skip to content
Binary file not shown.
11 changes: 11 additions & 0 deletions pallets/mining-rewards/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,15 @@ pub mod pallet {
/// The reward amount redirected to treasury
reward: BalanceOf<T>,
},
/// 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<T>,
},
}

#[pallet::hooks]
Expand Down Expand Up @@ -256,6 +265,7 @@ pub mod pallet {
"Failed to redirect {:?} to treasury: {:?}",
reward, e2
);
Self::deposit_event(Event::TreasuryMintFailed { reward });
},
}
},
Expand All @@ -278,6 +288,7 @@ pub mod pallet {
"Failed to mint {:?} to treasury: {:?}",
reward, e
);
Self::deposit_event(Event::TreasuryMintFailed { reward });
},
}
},
Expand Down
3 changes: 0 additions & 3 deletions pallets/multisig/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,17 +140,14 @@ mod benchmarks {
status: ProposalStatus,
deposit: crate::BalanceOf<T>,
) {
use frame_support::dispatch::GetDispatchInfo;
let system_call = frame_system::Call::<T>::remark { remark: vec![1u8; call_size as usize] };
let runtime_call = <T as Config>::RuntimeCall::from(system_call);
let call_weight = runtime_call.get_dispatch_info().call_weight;
let encoded = runtime_call.encode();
let bounded_call: BoundedCallOf<T> = encoded.try_into().unwrap();
let bounded_approvals: BoundedApprovalsOf<T> = approvals.to_vec().try_into().unwrap();
let proposal_data = ProposalDataOf::<T> {
proposer: proposer.clone(),
call: bounded_call,
call_weight,
expiry,
approvals: bounded_approvals,
deposit,
Expand Down
28 changes: 19 additions & 9 deletions pallets/multisig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -106,8 +106,6 @@ pub struct ProposalData<AccountId, Balance, BlockNumber, BoundedCall, BoundedApp
pub proposer: AccountId,
/// The encoded call to be executed
pub call: BoundedCall,
/// The declared weight of the inner call (captured at propose time)
pub call_weight: Weight,
/// Expiry block number
pub expiry: BlockNumber,
/// List of accounts that have approved this proposal
Expand Down Expand Up @@ -150,7 +148,11 @@ pub mod pallet {
///
/// This establishes an explicit baseline for future storage migrations.
/// Increment this and add a migration hook when storage layout changes.
const STORAGE_VERSION: StorageVersion = StorageVersion::new(0);
///
/// Version history:
/// - 0: Initial version
/// - 1: Removed `call_weight` field from ProposalData (weight is recomputed at execute time)
const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
Comment thread
illuzen marked this conversation as resolved.

#[pallet::pallet]
#[pallet::storage_version(STORAGE_VERSION)]
Expand Down Expand Up @@ -654,7 +656,6 @@ pub mod pallet {
ProposalData {
proposer: proposer.clone(),
call,
call_weight,
expiry,
approvals,
deposit,
Expand Down Expand Up @@ -1068,6 +1069,14 @@ pub mod pallet {
let call = <T as Config>::RuntimeCall::decode(&mut &proposal.call[..])
.map_err(|_| Self::err_with_weight_raw(Error::<T>::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::<T>::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.
Expand All @@ -1078,7 +1087,7 @@ pub mod pallet {
return Self::err_with_weight(Error::<T>::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)
Expand All @@ -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);

Expand Down
28 changes: 24 additions & 4 deletions pallets/multisig/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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<Weight> = RefCell::new(Weight::from_parts(1_000_000_000, 1_048_576));
}

pub struct DynamicMaxInnerCallWeight;
impl Get<Weight> 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 {
Expand All @@ -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;
Expand Down
64 changes: 61 additions & 3 deletions pallets/multisig/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -2413,3 +2413,61 @@ fn execute_allows_whitelisted_call_after_hs_enabled() {
assert!(!Proposals::<Test>::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::<Test>::CallWeightExceedsLimit.into(),
);

// Proposal should still exist (execute failed early)
assert!(Proposals::<Test>::contains_key(&multisig_address, 0));

// Reset for other tests
reset_max_inner_call_weight();
});
}
80 changes: 50 additions & 30 deletions pallets/reversible-transfers/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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.
Expand All @@ -556,38 +567,47 @@ pub mod pallet {
PendingTransfersBySender::<T>::get(&account).into_iter().collect();

for tx_id in pending_tx_ids.iter() {
if let Some(pending) = PendingTransfers::<T>::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::<T>::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::<T>::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::<T>::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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Loading
Loading