Fix: origin-rewriting wrappers bypass high-security restrictions

pallets/multisig/src/lib.rs

@@ -1130,9 +1130,7 @@ pub mod pallet {
 			// 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)
-			{
+			if !T::HighSecurity::is_call_allowed(&multisig_address, &call) {
 				return Self::err_burn_full(Error::<T>::CallNotAllowedForHighSecurityMultisig);
 			}
 

primitives/high-security/src/lib.rs

@@ -122,6 +122,15 @@ pub trait HighSecurityInspector<AccountId, RuntimeCall> {
 	/// `Some(guardian_account)` if the account has a guardian, `None` otherwise
 	fn guardian(who: &AccountId) -> Option<AccountId>;
 
+	/// 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`.
+	fn is_call_allowed(who: &AccountId, call: &RuntimeCall) -> bool {
+		!Self::is_high_security(who) || Self::is_whitelisted(call)
+	}
+
 	// NOTE: No benchmarking-specific methods in the trait!
 	// Production API should not be polluted by test/benchmark requirements.
 	// Use pallet-specific helpers instead (e.g.,

runtime/src/configs/mod.rs

@@ -52,6 +52,7 @@ use pallet_ranked_collective::Linear;
 use pallet_transaction_payment::{ConstFeeMultiplier, FungibleAdapter, Multiplier};
 use smallvec::smallvec;
 
+use qp_high_security::HighSecurityInspector;
 use qp_scheduler::BlockNumberOrTimestamp;
 use sp_runtime::{
 	traits::{AccountIdConversion, BlakeTwo256, One},
@@ -623,6 +624,77 @@ impl qp_high_security::HighSecurityInspector<AccountId, RuntimeCall> for HighSec
 		// Delegate to reversible-transfers pallet
 		pallet_reversible_transfers::Pallet::<Runtime>::get_guardian(who)
 	}
+
+	fn is_call_allowed(who: &AccountId, call: &RuntimeCall) -> bool {
+		Self::call_allowed_for(who, call, 0)
+	}
+}
+
+impl HighSecurityConfig {
+	/// Maximum wrapper nesting the high-security resolver traverses. Far above any realistic
+	/// legitimate nesting yet far below `frame_support::MAX_EXTRINSIC_DEPTH` (256, enforced at
+	/// decode), so it bounds resolver work even if the decode limit ever changed. Calls nested
+	/// deeper fail closed (the transaction is rejected) rather than escaping the whitelist.
+	const MAX_CALL_DEPTH: u32 = 16;
+
+	/// Recursively verify that `call`, dispatched with `signer` as the effective signed
+	/// origin, never reaches a non-whitelisted call as a high-security account.
+	///
+	/// Origin-rewriting wrappers (`Utility::as_derivative`, `Recovery::as_recovered`)
+	/// synthesize a fresh `Signed` origin *after* top-level transaction validation, so the
+	/// high-security whitelist must be re-checked at the effective origin. Origin-preserving
+	/// combinators (`batch`/`batch_all`/`force_batch`/`if_else`) are traversed with the same
+	/// signer. `dispatch_as`/`with_weight` and the scheduler are root-only, so they are not an
+	/// unprivileged bypass and are intentionally not traversed.
+	fn call_allowed_for(signer: &AccountId, call: &RuntimeCall, depth: u32) -> bool {
+		if depth > Self::MAX_CALL_DEPTH {
+			return false;
+		}
+		if Self::is_high_security(signer) && !Self::is_whitelisted(call) {
+			return false;
+		}
+		match call {
+			RuntimeCall::Utility(pallet_utility::Call::as_derivative { index, call }) => {
+				let pseudonym = pallet_utility::derivative_account_id(signer.clone(), *index);
+				Self::call_allowed_for(&pseudonym, call, depth + 1)
+			},
+			RuntimeCall::Recovery(pallet_recovery::Call::as_recovered { account, call }) =>
+				match account {
+					sp_runtime::MultiAddress::Id(target) =>
+						Self::call_allowed_for(target, call, depth + 1),
+					// Other address kinds are unresolvable by the runtime lookup and cannot
+					// dispatch, so there is no effective origin to enforce.
+					_ => true,
+				},
+			RuntimeCall::Utility(pallet_utility::Call::batch { calls }) |
+			RuntimeCall::Utility(pallet_utility::Call::batch_all { calls }) |
+			RuntimeCall::Utility(pallet_utility::Call::force_batch { calls }) =>
+				calls.iter().all(|c| Self::call_allowed_for(signer, c, depth + 1)),
+			RuntimeCall::Utility(pallet_utility::Call::if_else { main, fallback }) =>
+				Self::call_allowed_for(signer, main, depth + 1) &&
+					Self::call_allowed_for(signer, fallback, depth + 1),
+			_ => true,
+		}
+	}
+
+	/// Upper bound on the `is_high_security` storage reads `call_allowed_for` performs for
+	/// `call` (one per traversed node), used to weight the transaction extension.
+	pub(crate) fn high_security_read_count(call: &RuntimeCall) -> u64 {
+		let inner = match call {
+			RuntimeCall::Utility(pallet_utility::Call::as_derivative { call, .. }) |
+			RuntimeCall::Recovery(pallet_recovery::Call::as_recovered { call, .. }) =>
+				Self::high_security_read_count(call),
+			RuntimeCall::Utility(pallet_utility::Call::batch { calls }) |
+			RuntimeCall::Utility(pallet_utility::Call::batch_all { calls }) |
+			RuntimeCall::Utility(pallet_utility::Call::force_batch { calls }) =>
+				calls.iter().map(Self::high_security_read_count).sum(),
+			RuntimeCall::Utility(pallet_utility::Call::if_else { main, fallback }) =>
+				Self::high_security_read_count(main)
+					.saturating_add(Self::high_security_read_count(fallback)),
+			_ => 0,
+		};
+		inner.saturating_add(1)
+	}
 }
 
 impl pallet_multisig::Config for Runtime {

runtime/src/transaction_extensions.rs

@@ -42,8 +42,10 @@ impl<T: pallet_reversible_transfers::Config + Send + Sync + alloc::fmt::Debug>
 
 	const IDENTIFIER: &'static str = "ReversibleTransactionExtension";
 
-	fn weight(&self, _call: &RuntimeCall) -> Weight {
-		T::DbWeight::get().reads(1)
+	fn weight(&self, call: &RuntimeCall) -> Weight {
+		// One `is_high_security` read per node traversed by the recursive whitelist check.
+		T::DbWeight::get()
+			.reads(crate::configs::HighSecurityConfig::high_security_read_count(call))
 	}
 
 	fn prepare(
@@ -70,14 +72,12 @@ impl<T: pallet_reversible_transfers::Config + Send + Sync + alloc::fmt::Debug>
 		let who = ensure_signed(origin.clone())
 			.map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::BadSigner))?;
 
-		// Check if account is high-security using the same inspector as multisig
-		if crate::configs::HighSecurityConfig::is_high_security(&who) {
-			// Use the same whitelist check as multisig
-			if crate::configs::HighSecurityConfig::is_whitelisted(call) {
-				return Ok((ValidTransaction::default(), (), origin));
-			} else {
-				return Err(TransactionValidityError::Invalid(InvalidTransaction::Custom(1)));
-			}
+		// Enforce the high-security whitelist at the *effective* dispatched origin: this
+		// recursively resolves origin-rewriting wrappers (`as_derivative`/`as_recovered`) and
+		// origin-preserving combinators (`batch*`/`if_else`) so a non-high-security outer signer
+		// cannot dispatch a non-whitelisted call as a high-security account.
+		if !crate::configs::HighSecurityConfig::is_call_allowed(&who, call) {
+			return Err(TransactionValidityError::Invalid(InvalidTransaction::Custom(1)));
 		}
 
 		Ok((ValidTransaction::default(), (), origin))
@@ -441,32 +441,34 @@ mod tests {
 		});
 	}
 
-	fn check_call(call: RuntimeCall) -> Result<(), TransactionValidityError> {
-		// Test the reversible transaction extension
-		let ext = ReversibleTransactionExtension::<Runtime>::new();
+	// Run the reversible transaction extension's `validate` for `call` signed by `signer`.
+	fn validate_with(signer: AccountId, call: &RuntimeCall) -> Result<(), TransactionValidityError> {
+		ReversibleTransactionExtension::<Runtime>::new()
+			.validate(
+				RuntimeOrigin::signed(signer),
+				call,
+				&Default::default(),
+				0,
+				(),
+				&TxBaseImplication::<()>(()),
+				frame_support::pallet_prelude::TransactionSource::External,
+			)
+			.map(|_| ())
+	}
 
+	fn check_call(call: RuntimeCall) -> Result<(), TransactionValidityError> {
 		// Verify Charlie is high-security
 		assert!(ReversibleTransfers::is_high_security(&charlie()).is_some());
 
 		let origin = RuntimeOrigin::signed(charlie());
 
 		// Test the prepare method
-		ext.clone().prepare((), &origin, &call, &Default::default(), 0).unwrap();
-
-		assert_eq!((), ());
+		ReversibleTransactionExtension::<Runtime>::new()
+			.prepare((), &origin, &call, &Default::default(), 0)
+			.unwrap();
 
 		// Test the validate method
-		let result = ext.validate(
-			origin,
-			&call,
-			&Default::default(),
-			0,
-			(),
-			&TxBaseImplication::<()>(()),
-			frame_support::pallet_prelude::TransactionSource::External,
-		);
-
-		result.map(|_| ())
+		validate_with(charlie(), &call)
 	}
 
 	#[test]
@@ -558,6 +560,146 @@ mod tests {
 		});
 	}
 
+	// =========================================================================
+	// Origin-rewriting wrappers must not bypass high-security restrictions.
+	// A non-high-security outer signer must not be able to dispatch a
+	// non-whitelisted call as a high-security account via `as_recovered` /
+	// `as_derivative`, including when nested under `batch`.
+	// =========================================================================
+
+	fn boxed(call: RuntimeCall) -> alloc::boxed::Box<RuntimeCall> {
+		alloc::boxed::Box::new(call)
+	}
+
+	fn non_whitelisted_transfer() -> RuntimeCall {
+		RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
+			dest: MultiAddress::Id(bob()),
+			value: 10 * EXISTENTIAL_DEPOSIT,
+		})
+	}
+
+	fn whitelisted_schedule() -> RuntimeCall {
+		RuntimeCall::ReversibleTransfers(pallet_reversible_transfers::Call::schedule_transfer {
+			dest: MultiAddress::Id(bob()),
+			amount: 10 * EXISTENTIAL_DEPOSIT,
+		})
+	}
+
+	#[test]
+	fn as_recovered_cannot_bypass_high_security() {
+		new_test_ext().execute_with(|| {
+			// Charlie is high-security (from genesis); bob is the (non-high-security) rescuer.
+			let blocked = RuntimeCall::Recovery(pallet_recovery::Call::as_recovered {
+				account: MultiAddress::Id(charlie()),
+				call: boxed(non_whitelisted_transfer()),
+			});
+			assert_eq!(
+				validate_with(bob(), &blocked).unwrap_err(),
+				TransactionValidityError::Invalid(InvalidTransaction::Custom(1))
+			);
+
+			// A whitelisted inner call dispatched as the high-security account is allowed.
+			let allowed = RuntimeCall::Recovery(pallet_recovery::Call::as_recovered {
+				account: MultiAddress::Id(charlie()),
+				call: boxed(whitelisted_schedule()),
+			});
+			assert_ok!(validate_with(bob(), &allowed));
+		});
+	}
+
+	#[test]
+	fn as_derivative_cannot_bypass_high_security() {
+		new_test_ext().execute_with(|| {
+			// Make alice's index-0 derivative a high-security account.
+			let derivative = pallet_utility::derivative_account_id(alice(), 0u16);
+			Balances::make_free_balance_be(&derivative, EXISTENTIAL_DEPOSIT * 100);
+			assert_ok!(ReversibleTransfers::set_high_security(
+				RuntimeOrigin::signed(derivative.clone()),
+				qp_scheduler::BlockNumberOrTimestamp::BlockNumber(10),
+				bob(),
+			));
+			assert!(ReversibleTransfers::is_high_security(&derivative).is_some());
+
+			// Dispatching a non-whitelisted call as the high-security derivative is blocked.
+			let blocked = RuntimeCall::Utility(pallet_utility::Call::as_derivative {
+				index: 0,
+				call: boxed(non_whitelisted_transfer()),
+			});
+			assert_eq!(
+				validate_with(alice(), &blocked).unwrap_err(),
+				TransactionValidityError::Invalid(InvalidTransaction::Custom(1))
+			);
+
+			// A whitelisted inner call as the derivative is allowed.
+			let allowed = RuntimeCall::Utility(pallet_utility::Call::as_derivative {
+				index: 0,
+				call: boxed(whitelisted_schedule()),
+			});
+			assert_ok!(validate_with(alice(), &allowed));
+
+			// A different (non-high-security) derivative is unaffected.
+			let control = RuntimeCall::Utility(pallet_utility::Call::as_derivative {
+				index: 1,
+				call: boxed(non_whitelisted_transfer()),
+			});
+			assert_ok!(validate_with(alice(), &control));
+		});
+	}
+
+	#[test]
+	fn batch_wrapped_origin_rewriter_cannot_bypass_high_security() {
+		new_test_ext().execute_with(|| {
+			let inner = RuntimeCall::Recovery(pallet_recovery::Call::as_recovered {
+				account: MultiAddress::Id(charlie()),
+				call: boxed(non_whitelisted_transfer()),
+			});
+			let batch =
+				RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![inner] });
+			assert_eq!(
+				validate_with(bob(), &batch).unwrap_err(),
+				TransactionValidityError::Invalid(InvalidTransaction::Custom(1))
+			);
+		});
+	}
+
+	#[test]
+	fn non_high_security_origin_rewriter_is_unaffected() {
+		new_test_ext().execute_with(|| {
+			// bob is not high-security; recovering a non-high-security account and transferring
+			// is not restricted by the high-security whitelist.
+			let call = RuntimeCall::Recovery(pallet_recovery::Call::as_recovered {
+				account: MultiAddress::Id(alice()),
+				call: boxed(non_whitelisted_transfer()),
+			});
+			assert_ok!(validate_with(bob(), &call));
+		});
+	}
+
+	#[test]
+	fn over_nested_call_is_rejected_fail_closed() {
+		new_test_ext().execute_with(|| {
+			// Wrap a (harmless) call in `n` nested batches.
+			let nest = |levels: u32| {
+				let mut call =
+					RuntimeCall::System(frame_system::Call::remark { remark: vec![1] });
+				for _ in 0..levels {
+					call = RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![call] });
+				}
+				call
+			};
+
+			// Within the depth bound the call is resolved normally and allowed.
+			assert_ok!(validate_with(bob(), &nest(8)));
+
+			// Beyond the bound the resolver fails closed: the transaction is rejected rather than
+			// allowed to escape the whitelist, capping resolver work regardless of contents.
+			assert_eq!(
+				validate_with(bob(), &nest(20)).unwrap_err(),
+				TransactionValidityError::Invalid(InvalidTransaction::Custom(1))
+			);
+		});
+	}
+
 	// =========================================================================
 	// Tests for event-based WormholeProofRecorderExtension
 	// =========================================================================