diff --git a/pallets/common/src/lib.rs b/pallets/common/src/lib.rs index 393849a99..9d3da90a6 100644 --- a/pallets/common/src/lib.rs +++ b/pallets/common/src/lib.rs @@ -328,46 +328,38 @@ impl ReservableCurrency for NoCurrency( call: &pallet_utility::Call, -) -> Option<(Vec<&::RuntimeCall>, bool)> { +) -> Option::RuntimeCall>> { let inner = utility_inner_calls(call); if inner.is_empty() { return None; } - let preserves_origin = matches!( - call, - pallet_utility::Call::batch { .. } | - pallet_utility::Call::batch_all { .. } | - pallet_utility::Call::force_batch { .. } - ); - Some((inner, preserves_origin)) + Some(inner) } /// Inspect a sudo call for wrapper semantics: returns inner calls. -/// Sudo always changes the origin (to Root or target), so `preserves_origin` is `false`. pub fn inspect_sudo_wrapper( call: &pallet_sudo::Call, -) -> Option<(Vec<&::RuntimeCall>, bool)> { +) -> Option::RuntimeCall>> { let inner = sudo_inner_calls(call); if inner.is_empty() { return None; } - Some((inner, false)) + Some(inner) } /// Inspect a proxy call for wrapper semantics: returns inner calls. -/// Proxy dispatches with the delegator's origin, so `preserves_origin` is `false`. pub fn inspect_proxy_wrapper( call: &pallet_proxy::Call, -) -> Option<(Vec<&::RuntimeCall>, bool)> { +) -> Option::RuntimeCall>> { let inner = proxy_inner_calls(call); if inner.is_empty() { return None; } - Some((inner, false)) + Some(inner) } /// Extract inner calls from a utility call variant. diff --git a/pallets/transaction-storage/src/extension.rs b/pallets/transaction-storage/src/extension.rs index ac6672948..33feade2f 100644 --- a/pallets/transaction-storage/src/extension.rs +++ b/pallets/transaction-storage/src/extension.rs @@ -17,9 +17,7 @@ //! Custom transaction extension for the transaction storage pallet. -use crate::{ - pallet::Origin, weights::WeightInfo, AuthorizationScope, Call, Config, Pallet, LOG_TARGET, -}; +use crate::{pallet::Origin, weights::WeightInfo, Call, Config, Pallet, LOG_TARGET}; use alloc::vec::Vec; use codec::{Decode, DecodeWithMemTracking, Encode}; use core::{fmt, marker::PhantomData}; @@ -31,15 +29,11 @@ use polkadot_sdk_frame::{ type RuntimeCallOf = ::RuntimeCall; -/// Result of [`CallInspector::traverse_storage_calls`]. +/// Result of [`CallInspector::traverse_storage_calls`]: whether any TransactionStorage +/// pallet calls (management calls like authorize_*, refresh_*, remove_expired_*) were found. #[derive(Default)] pub struct TraverseResult { - /// Whether any TransactionStorage pallet calls were visited. - found_storage: bool, - /// Whether the outermost wrapper preserves the caller's origin (e.g. batch). - preserves_origin: bool, - /// Whether any non-storage, non-wrapper calls were found in the call tree. - has_non_storage: bool, + pub found_storage: bool, } /// Maximum recursion depth for inspecting wrapper calls. @@ -49,19 +43,16 @@ pub const MAX_WRAPPER_DEPTH: u32 = 8; /// extrinsics (e.g. `Utility::batch`, `Sudo::sudo_as`). /// /// The runtime implements this for its `RuntimeCall` type, allowing the pallet extension -/// to recursively validate and consume storage authorization in wrapped calls, and to -/// transform the origin to [`Origin::Authorized`] for origin-preserving wrappers. +/// to recursively inspect wrapper calls for storage-mutating operations (which are rejected) +/// and management calls (which are validated). pub trait CallInspector: Clone + PartialEq + Eq + Default where RuntimeCallOf: IsSubType>, { - /// If `call` is a wrapper, return: - /// - The inner calls to inspect for storage authorization - /// - `true` if the wrapper passes origin through to inner calls (e.g. batch), `false` if it - /// changes the origin (e.g. sudo_as) + /// If `call` is a wrapper, return the inner calls to inspect for storage authorization. /// /// Returns `None` for non-wrapper calls. - fn inspect_wrapper(call: &RuntimeCallOf) -> Option<(Vec<&RuntimeCallOf>, bool)>; + fn inspect_wrapper(call: &RuntimeCallOf) -> Option>>; /// Returns `true` if `call` is a storage-mutating TransactionStorage call (store, /// store_with_cid_config, renew) — either directly or nested inside wrappers. @@ -85,7 +76,7 @@ where Call::store { .. } | Call::store_with_cid_config { .. } | Call::renew { .. } ); } - if let Some((inner_calls, _)) = Self::inspect_wrapper(call) { + if let Some(inner_calls) = Self::inspect_wrapper(call) { return inner_calls .into_iter() .any(|inner| Self::is_storage_mutating_call(inner, depth + 1)); @@ -93,12 +84,12 @@ where false } - /// Recursively traverse a call tree, applying `visitor` to each storage call found. + /// Recursively traverse a call tree, applying `visitor` to each + /// TransactionStorage pallet call found. /// - /// Returns [`TraverseResult`] with: - /// - `found_storage`: whether any storage calls were visited - /// - `preserves_origin`: whether the outermost wrapper preserves the caller's origin - /// - `has_non_storage`: whether any non-storage, non-wrapper calls were found + /// Returns [`TraverseResult`] with `found_storage` set if any pallet calls were visited. + /// Callers should use [`Self::is_storage_mutating_call`] first to reject wrappers + /// containing store/renew before calling this. fn traverse_storage_calls( call: &RuntimeCallOf, depth: u32, @@ -106,12 +97,9 @@ where ) -> Result { if let Some(inner_call) = call.is_sub_type() { visitor(inner_call)?; - // Direct storage call — `preserves_origin` doesn't matter here because - // `validate()` already handles origin transformation for direct calls - // before calling `traverse_storage_calls`. - return Ok(TraverseResult { found_storage: true, ..Default::default() }); + return Ok(TraverseResult { found_storage: true }); } - if let Some((inner_calls, preserves_origin)) = Self::inspect_wrapper(call) { + if let Some(inner_calls) = Self::inspect_wrapper(call) { if depth >= MAX_WRAPPER_DEPTH { tracing::debug!( target: LOG_TARGET, @@ -119,16 +107,15 @@ where ); return Err(InvalidTransaction::ExhaustsResources.into()); } - let mut result = TraverseResult { preserves_origin, ..Default::default() }; + let mut found_storage = false; for inner in inner_calls { - let inner_result = Self::traverse_storage_calls(inner, depth + 1, visitor)?; - result.found_storage |= inner_result.found_storage; - result.has_non_storage |= inner_result.has_non_storage; + found_storage |= + Self::traverse_storage_calls(inner, depth + 1, visitor)?.found_storage; } - return Ok(result); + return Ok(TraverseResult { found_storage }); } - // Not a storage call and not a wrapper — a non-storage call. - Ok(TraverseResult { has_non_storage: true, ..Default::default() }) + // Not a storage call and not a wrapper — ignore. + Ok(TraverseResult::default()) } } @@ -137,7 +124,7 @@ impl CallInspector for () where RuntimeCallOf: IsSubType>, { - fn inspect_wrapper(_: &RuntimeCallOf) -> Option<(Vec<&RuntimeCallOf>, bool)> { + fn inspect_wrapper(_: &RuntimeCallOf) -> Option>> { None } } @@ -146,18 +133,18 @@ where /// /// This extension handles **signed TransactionStorage transactions** via /// [`Pallet::validate_signed`]: -/// - **Store/renew calls**: Validates authorization in `validate()` and transforms the origin to -/// [`Origin::Authorized`] to carry authorization info. Then in `prepare()`, it consumes the -/// authorization extent (decrements remaining transactions/bytes) before the extrinsic executes. -/// This early consumption prevents large invalid store transactions from propagating through -/// mempools and the network — authorization is checked and spent at the extension level rather -/// than during dispatch. +/// - **Store/renew calls**: Must be submitted as **direct extrinsics** (not wrapped). Validates +/// authorization in `validate()` and transforms the origin to [`Origin::Authorized`] to carry +/// authorization info. Then in `prepare()`, it consumes the authorization extent (decrements +/// remaining transactions/bytes) before the extrinsic executes. This early consumption prevents +/// large invalid store transactions from propagating through mempools and the network — +/// authorization is checked and spent at the extension level rather than during dispatch. /// - **Authorization management calls** (authorize_*, refresh_*, remove_expired_*): Validates that -/// the signer satisfies the [`Config::Authorizer`] origin requirement. +/// the signer satisfies the [`Config::Authorizer`] origin requirement. These calls **can** be +/// wrapped (e.g. in `Utility::batch`). /// - **Wrapper calls** (e.g. `Utility::batch`, `Sudo::sudo`): Uses `I: CallInspector` to -/// recursively find and validate/consume storage authorization for inner storage calls. For -/// origin-preserving wrappers (batch), the origin is transformed to [`Origin::Authorized`] so -/// that inner `store`/`renew` dispatches pass [`Pallet::ensure_authorized`]. +/// recursively inspect inner calls. Rejects any wrapper containing store/renew calls. Allows +/// wrappers containing only management calls. /// /// The `I` type parameter controls wrapper inspection. Use `()` (the default) for no wrapper /// support, or provide a runtime-specific [`CallInspector`] implementation to enable recursive @@ -248,57 +235,22 @@ where return Ok((valid_tx, Some(who), origin)); } - // Wrapper call — validate storage authorization for inner calls. - // Accumulate ValidTransaction metadata (provides tags, priority, longevity) from - // each inner storage call so the mempool can deduplicate and prioritize correctly. + // Wrapper call — reject if it contains store/renew (must be direct extrinsics), + // then validate any management calls (authorize_*, refresh_*, remove_expired_*). + if I::is_storage_mutating_call(call, 0) { + return Err(InvalidTransaction::Call.into()); + } let mut combined_valid = ValidTransaction::default(); - let mut authorized_scope: Option> = None; - let mut has_management_call = false; let result = I::traverse_storage_calls(call, 0, &mut |inner_call| { - let (valid_tx, scope) = Pallet::::validate_signed(&who, inner_call)?; + let (valid_tx, _scope) = Pallet::::validate_signed(&who, inner_call)?; combined_valid = core::mem::take(&mut combined_valid).combine_with(valid_tx); - match scope { - // Store/renew calls return a scope and need the Authorized origin. - // Keep the first scope — different calls in a batch may resolve to - // different scopes (Preimage vs Account), but the scope value is only - // used to satisfy `ensure_authorized` at dispatch, which checks the - // origin type, not the scope variant. - Some(s) => { - authorized_scope.get_or_insert(s); - }, - // Authorization management calls (authorize_*, refresh_*, - // remove_expired_*) return None and need the original Signed origin. - None => { - has_management_call = true; - }, - } Ok(()) })?; if result.found_storage { - if authorized_scope.is_some() && - (result.has_non_storage || has_management_call) && - result.preserves_origin - { - // Reject batches that mix store/renew with other calls. - // Store/renew needs the origin transformed to Authorized, but other - // calls (both non-storage and authorization management) need the - // original Signed origin. Allowing both in an origin-preserving - // wrapper would cause: - // - silent dispatch failure of the non-storage calls (with `batch`), or - // - authorization leak where `prepare` consumes the allowance but `batch_all` - // reverts the store's writes after a later call fails. - return Err(InvalidTransaction::Call.into()); - } - if result.preserves_origin { - if let Some(scope) = authorized_scope { - // Transform origin so inner store/renew dispatches see Authorized. - origin.set_caller_from(Origin::::Authorized { who: who.clone(), scope }); - } - } return Ok((combined_valid, Some(who), origin)); } - // Not a storage-related call + // No TransactionStorage calls found in wrapper. Ok((ValidTransaction::default(), None, origin)) } diff --git a/runtimes/bulletin-polkadot/src/lib.rs b/runtimes/bulletin-polkadot/src/lib.rs index f9d9e075f..ceb865251 100644 --- a/runtimes/bulletin-polkadot/src/lib.rs +++ b/runtimes/bulletin-polkadot/src/lib.rs @@ -378,7 +378,7 @@ impl SortedMembers for TestAccounts { pub struct StorageCallInspector; impl pallet_transaction_storage::CallInspector for StorageCallInspector { - fn inspect_wrapper(call: &RuntimeCall) -> Option<(alloc::vec::Vec<&RuntimeCall>, bool)> { + fn inspect_wrapper(call: &RuntimeCall) -> Option> { match call { RuntimeCall::Utility(c) => inspect_utility_wrapper(c), RuntimeCall::Proxy(c) => inspect_proxy_wrapper(c), diff --git a/runtimes/bulletin-polkadot/tests/tests.rs b/runtimes/bulletin-polkadot/tests/tests.rs index 896e2ac99..a14938e64 100644 --- a/runtimes/bulletin-polkadot/tests/tests.rs +++ b/runtimes/bulletin-polkadot/tests/tests.rs @@ -1101,27 +1101,6 @@ fn wrap_call_utility_variants(call: RuntimeCall) -> Vec<(RuntimeCall, &'static s ] } -/// Assert that direct and utility-wrapper variants are rejected at validation time. -fn assert_rejected_at_validation( - signer: AccountKeyring, - call: RuntimeCall, - expected: TransactionValidityError, - label: &str, -) { - assert_eq!( - construct_and_apply_extrinsic(signer.pair(), call.clone()), - Err(expected), - "{label}: direct", - ); - for (wrapped, name) in wrap_call_utility_variants(call) { - assert_eq!( - construct_and_apply_extrinsic(signer.pair(), wrapped), - Err(expected), - "{label}: via {name}", - ); - } -} - fn provision_account(who: AccountKeyring) { frame_system::Pallet::::inc_providers(&who.to_account_id()); } @@ -1148,16 +1127,23 @@ fn wrapped_store_requires_authorization() { data: vec![42u8; 100], }); - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - attacker, - store_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "store", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(attacker.pair(), store_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "store: direct", ); - // sudo_as: attacker is NOT the sudo key, but sudo_as inner store call - // is checked for TransactionStorage auth at validation time. + // Utility wrappers: rejected because store is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(store_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(attacker.pair(), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "store: via {name}", + ); + } + + // sudo_as: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( attacker.pair(), @@ -1166,10 +1152,10 @@ fn wrapped_store_requires_authorization() { call: Box::new(store_call.clone()), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); - // proxy: inner store is checked for TransactionStorage auth at validation time. + // proxy: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( attacker.pair(), @@ -1179,7 +1165,7 @@ fn wrapped_store_requires_authorization() { call: Box::new(store_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } @@ -1199,16 +1185,23 @@ fn wrapped_store_with_cid_config_requires_authorization() { data: vec![42u8; 100], }); - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - attacker, - store_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "store_with_cid_config", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(attacker.pair(), store_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "store_with_cid_config: direct", ); - // sudo_as: inner store_with_cid_config is checked for TransactionStorage auth at - // validation. + // Utility wrappers: rejected because store is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(store_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(attacker.pair(), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "store_with_cid_config: via {name}", + ); + } + + // sudo_as: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( attacker.pair(), @@ -1217,10 +1210,10 @@ fn wrapped_store_with_cid_config_requires_authorization() { call: Box::new(store_call.clone()), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); - // proxy: inner store_with_cid_config is checked for TransactionStorage auth at validation. + // proxy: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( attacker.pair(), @@ -1230,7 +1223,7 @@ fn wrapped_store_with_cid_config_requires_authorization() { call: Box::new(store_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } @@ -1250,16 +1243,23 @@ fn wrapped_store_requires_authorization_even_for_relayer() { data: vec![99u8; 200], }); - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - relayer, - store_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "relayer store without auth", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(relayer.pair(), store_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "relayer store without auth: direct", ); - // sudo_as: relayer IS the sudo key, but inner store is checked for - // TransactionStorage auth at validation time. + // Utility wrappers: rejected because store is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(store_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(relayer.pair(), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "relayer store without auth: via {name}", + ); + } + + // sudo_as: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( relayer.pair(), @@ -1268,7 +1268,7 @@ fn wrapped_store_requires_authorization_even_for_relayer() { call: Box::new(store_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } @@ -1324,15 +1324,23 @@ fn wrapped_renew_requires_authorization() { index: 0, }); - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - attacker, - renew_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "renew", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(attacker.pair(), renew_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "renew: direct", ); - // sudo_as: inner renew is checked for TransactionStorage auth at validation. + // Utility wrappers: rejected because renew is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(renew_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(attacker.pair(), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "renew: via {name}", + ); + } + + // sudo_as: renew inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( attacker.pair(), @@ -1341,10 +1349,10 @@ fn wrapped_renew_requires_authorization() { call: Box::new(renew_call.clone()), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); - // proxy: inner renew is checked for TransactionStorage auth at validation. + // proxy: renew inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( attacker.pair(), @@ -1354,7 +1362,7 @@ fn wrapped_renew_requires_authorization() { call: Box::new(renew_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } @@ -1420,41 +1428,22 @@ fn wrapped_authorize_account_succeeds() { }); } +/// Store calls inside wrappers (batch, batch_all, force_batch) are rejected even when +/// authorized. Store/renew must be submitted as direct extrinsics. #[test] -fn authorized_wrapped_store_succeeds() { +fn authorized_wrapped_store_rejected() { run_test(|| { advance_block(); let signer = sudo_relayer_signer(); let who: AccountId = signer.to_account_id(); let data = vec![42u8; 100]; - // batch, batch_all, force_batch (not as_derivative — it changes origin internally) - let batch_variants = |call: RuntimeCall| -> Vec<(RuntimeCall, &'static str)> { - vec![ - ( - RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![call.clone()] }), - "batch", - ), - ( - RuntimeCall::Utility(pallet_utility::Call::batch_all { - calls: vec![call.clone()], - }), - "batch_all", - ), - ( - RuntimeCall::Utility(pallet_utility::Call::force_batch { calls: vec![call] }), - "force_batch", - ), - ] - }; - - // Authorize enough for direct + 3 batch variants - let num_calls = 4u32; + // Authorize enough for several calls. assert_ok!(runtime::TransactionStorage::authorize_account( RuntimeOrigin::root(), who.clone(), - num_calls, - num_calls as u64 * data.len() as u64, + 4, + 4 * data.len() as u64, )); let store_call = @@ -1463,25 +1452,26 @@ fn authorized_wrapped_store_succeeds() { // Direct store should succeed. assert_ok_ok(construct_and_apply_extrinsic(signer.pair(), store_call.clone())); - // Batch-wrapped store should also succeed. - for (wrapped, name) in batch_variants(store_call) { - let res = construct_and_apply_extrinsic(signer.pair(), wrapped); - assert!(res.is_ok(), "{name}: apply_extrinsic failed with {res:?}"); - assert!(res.unwrap().is_ok(), "{name}: dispatch failed"); + // Batch-wrapped store must be rejected. + for (wrapped, name) in wrap_call_utility_variants(store_call) { + assert_eq!( + construct_and_apply_extrinsic(signer.pair(), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "{name}: wrapped store must be rejected", + ); } - // All authorization should be consumed. + // Only the direct store consumed authorization (1 tx, data.len() bytes). assert_eq!( runtime::TransactionStorage::account_authorization_extent(who), - AuthorizationExtent { transactions: 0, bytes: 0 }, + AuthorizationExtent { transactions: 3, bytes: 3 * data.len() as u64 }, ); }); } -/// Batch containing two store calls where one uses preimage authorization and the other -/// uses account authorization. Both authorizations should be properly consumed. +/// Batch containing store calls is rejected — store must be submitted as direct extrinsics. #[test] -fn batch_store_with_mixed_preimage_and_account_auth() { +fn batch_store_with_mixed_preimage_and_account_auth_rejected() { run_test(|| { advance_block(); let signer = sudo_relayer_signer(); @@ -1514,21 +1504,22 @@ fn batch_store_with_mixed_preimage_and_account_auth() { let batch = RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![store_a, store_b] }); - // Batch should succeed — store_a uses preimage auth, store_b uses account auth. - let res = construct_and_apply_extrinsic(signer.pair(), batch); - assert!(res.is_ok(), "apply_extrinsic failed: {res:?}"); - assert!(res.unwrap().is_ok(), "dispatch failed"); + // Batch containing store calls is rejected. + assert_eq!( + construct_and_apply_extrinsic(signer.pair(), batch), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); - // Both authorizations should be consumed. + // Authorizations were NOT consumed (rejected before prepare). assert_eq!( runtime::TransactionStorage::preimage_authorization_extent(content_hash_a), - AuthorizationExtent { transactions: 0, bytes: 0 }, - "Preimage authorization for data_a should be consumed", + AuthorizationExtent { transactions: 1, bytes: 100 }, + "Preimage authorization should not be consumed", ); assert_eq!( runtime::TransactionStorage::account_authorization_extent(who), - AuthorizationExtent { transactions: 0, bytes: 0 }, - "Account authorization for data_b should be consumed", + AuthorizationExtent { transactions: 1, bytes: 200 }, + "Account authorization should not be consumed", ); }); } @@ -1539,25 +1530,28 @@ fn wrapped_call_respects_validate_signed_allowlist() { advance_block(); let signer = sudo_relayer_signer(); - // System::remark is not in the ValidateSigned allowlist. - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - signer, - RuntimeCall::System(frame_system::Call::remark { remark: vec![1, 2, 3] }), - TransactionValidityError::Invalid(InvalidTransaction::Call), - "System::remark", + let remark = RuntimeCall::System(frame_system::Call::remark { remark: vec![1, 2, 3] }); + + // System::remark is not in the ValidateSigned allowlist — rejected direct. + assert_eq!( + construct_and_apply_extrinsic(signer.pair(), remark.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "System::remark: direct", ); - // Note: sudo_as and proxy are NOT tested here because they have their own - // authorization mechanisms (sudo key, proxy delegation) and are intentionally - // permitted at the validation level. The pallet-side defense-in-depth checks - // protect against unauthorized operations through those paths. + // Also rejected inside utility wrappers. + for (wrapped, name) in wrap_call_utility_variants(remark) { + assert_eq!( + construct_and_apply_extrinsic(signer.pair(), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "System::remark: via {name}", + ); + } }); } -/// Batch mixing store (needs Authorized origin) with authorize_account (needs Signed origin) -/// is rejected at validation time to prevent silent dispatch failures or authorization leaks -/// with `batch_all`. +/// Batch containing store is rejected — store must be submitted as direct extrinsics, +/// regardless of what else is in the batch. #[test] fn mixed_batch_store_and_authorize_rejected() { run_test(|| { @@ -1610,7 +1604,7 @@ fn mixed_batch_store_and_authorize_rejected() { }); } -/// Batch mixing store with a non-storage call is rejected at validation. +/// Batch containing store with a non-storage call is rejected — store must be direct. #[test] fn mixed_batch_store_and_non_storage_call_rejected() { run_test(|| { @@ -1671,10 +1665,11 @@ fn max_recursion_depth_is_enforced() { call = RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![call] }); } - // Should fail with ExhaustsResources due to depth limit. + // Should fail with Call — store inside wrapper is rejected (the depth limit + // in is_storage_mutating_call treats excessively nested calls as storage-mutating). assert_eq!( construct_and_apply_extrinsic(signer.pair(), call), - Err(TransactionValidityError::Invalid(InvalidTransaction::ExhaustsResources)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } diff --git a/runtimes/bulletin-westend/src/storage.rs b/runtimes/bulletin-westend/src/storage.rs index d51eee105..e5f9e3bcd 100644 --- a/runtimes/bulletin-westend/src/storage.rs +++ b/runtimes/bulletin-westend/src/storage.rs @@ -61,7 +61,7 @@ parameter_types! { pub struct StorageCallInspector; impl pallet_transaction_storage::CallInspector for StorageCallInspector { - fn inspect_wrapper(call: &RuntimeCall) -> Option<(Vec<&RuntimeCall>, bool)> { + fn inspect_wrapper(call: &RuntimeCall) -> Option> { match call { RuntimeCall::Utility(c) => inspect_utility_wrapper(c), RuntimeCall::Sudo(c) => inspect_sudo_wrapper(c), diff --git a/runtimes/bulletin-westend/tests/tests.rs b/runtimes/bulletin-westend/tests/tests.rs index 37561a2b2..caa66f5f8 100644 --- a/runtimes/bulletin-westend/tests/tests.rs +++ b/runtimes/bulletin-westend/tests/tests.rs @@ -736,46 +736,39 @@ fn wrap_call_utility_variants(call: RuntimeCall) -> Vec<(RuntimeCall, &'static s } /// Assert that direct and utility-wrapper variants are rejected at validation time. -fn assert_rejected_at_validation( - signer: Sr25519Keyring, - call: RuntimeCall, - expected: TransactionValidityError, - label: &str, -) { - assert_eq!( - construct_and_apply_extrinsic(Some(signer.pair()), call.clone()), - Err(expected), - "{label}: direct", - ); - for (wrapped, name) in wrap_call_utility_variants(call) { - assert_eq!( - construct_and_apply_extrinsic(Some(signer.pair()), wrapped), - Err(expected), - "{label}: via {name}", - ); - } -} - #[test] fn wrapped_store_requires_authorization() { sp_io::TestExternalities::new(RuntimeGenesisConfig::default().build_storage().unwrap()) .execute_with(|| { advance_block(); let account = Sr25519Keyring::Alice; + let who: AccountId = account.to_account_id(); + + // Fund Alice so fee checks pass and ValidateStorageCalls can reject. + use frame_support::traits::fungible::Mutate; + Balances::mint_into(&who, 1_000_000_000_000).unwrap(); let store_call = RuntimeCall::TransactionStorage(TxStorageCall::::store { data: vec![42u8; 100], }); - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - account, - store_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "store", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), store_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "store: direct", ); - // sudo_as: inner store is checked for TransactionStorage auth at validation. + // Utility wrappers: rejected because store is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(store_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "store: via {name}", + ); + } + + // sudo_as: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( Some(account.pair()), @@ -784,7 +777,7 @@ fn wrapped_store_requires_authorization() { call: Box::new(store_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } @@ -795,6 +788,11 @@ fn wrapped_store_with_cid_config_requires_authorization() { .execute_with(|| { advance_block(); let account = Sr25519Keyring::Alice; + let who: AccountId = account.to_account_id(); + + // Fund Alice so fee checks pass and ValidateStorageCalls can reject. + use frame_support::traits::fungible::Mutate; + Balances::mint_into(&who, 1_000_000_000_000).unwrap(); let store_call = RuntimeCall::TransactionStorage(TxStorageCall::::store_with_cid_config { @@ -802,15 +800,23 @@ fn wrapped_store_with_cid_config_requires_authorization() { data: vec![42u8; 100], }); - // Direct + utility wrappers: rejected at validation time. - assert_rejected_at_validation( - account, - store_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "store_with_cid_config", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), store_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "store_with_cid_config: direct", ); - // sudo_as: inner store_with_cid_config is checked at validation. + // Utility wrappers: rejected because store is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(store_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "store_with_cid_config: via {name}", + ); + } + + // sudo_as: store inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( Some(account.pair()), @@ -819,13 +825,15 @@ fn wrapped_store_with_cid_config_requires_authorization() { call: Box::new(store_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } +/// Store calls inside wrappers (batch, batch_all, force_batch) are rejected even when +/// authorized. Store/renew must be submitted as direct extrinsics. #[test] -fn authorized_wrapped_store_succeeds() { +fn authorized_wrapped_store_rejected() { sp_io::TestExternalities::new(RuntimeGenesisConfig::default().build_storage().unwrap()) .execute_with(|| { advance_block(); @@ -833,41 +841,16 @@ fn authorized_wrapped_store_succeeds() { let who: AccountId = account.to_account_id(); let data = vec![42u8; 100]; - // Fund Alice so she can pay the wrapper overhead fees (batch itself isn't feeless). + // Fund Alice for fees. use frame_support::traits::fungible::Mutate; Balances::mint_into(&who, 1_000_000_000_000).unwrap(); - // batch, batch_all, force_batch (not as_derivative — it changes origin internally) - let batch_variants = |call: RuntimeCall| -> Vec<(RuntimeCall, &'static str)> { - vec![ - ( - RuntimeCall::Utility(pallet_utility::Call::batch { - calls: vec![call.clone()], - }), - "batch", - ), - ( - RuntimeCall::Utility(pallet_utility::Call::batch_all { - calls: vec![call.clone()], - }), - "batch_all", - ), - ( - RuntimeCall::Utility(pallet_utility::Call::force_batch { - calls: vec![call], - }), - "force_batch", - ), - ] - }; - - // Authorize enough for direct + 3 batch variants - let num_calls = 4u32; + // Authorize enough for several calls. assert_ok!(TransactionStorage::authorize_account( RuntimeOrigin::root(), who.clone(), - num_calls, - num_calls as u64 * data.len() as u64, + 4, + 4 * data.len() as u64, )); let store_call = RuntimeCall::TransactionStorage(TxStorageCall::::store { @@ -877,32 +860,33 @@ fn authorized_wrapped_store_succeeds() { // Direct store should succeed. assert_ok_ok(construct_and_apply_extrinsic(Some(account.pair()), store_call.clone())); - // Batch-wrapped store should also succeed. - for (wrapped, name) in batch_variants(store_call) { - let res = construct_and_apply_extrinsic(Some(account.pair()), wrapped); - assert!(res.is_ok(), "{name}: apply_extrinsic failed with {res:?}"); - assert!(res.unwrap().is_ok(), "{name}: dispatch failed"); + // Batch-wrapped store must be rejected. + for (wrapped, name) in wrap_call_utility_variants(store_call) { + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "{name}: wrapped store must be rejected", + ); } - // All authorization should be consumed. + // Only the direct store consumed authorization (1 tx, data.len() bytes). assert_eq!( TransactionStorage::account_authorization_extent(who), - AuthorizationExtent { transactions: 0, bytes: 0 }, + AuthorizationExtent { transactions: 3, bytes: 3 * data.len() as u64 }, ); }); } -/// Batch containing two store calls where one uses preimage authorization and the other -/// uses account authorization. Both authorizations should be properly consumed. +/// Batch containing store calls is rejected — store must be submitted as direct extrinsics. #[test] -fn batch_store_with_mixed_preimage_and_account_auth() { +fn batch_store_with_mixed_preimage_and_account_auth_rejected() { sp_io::TestExternalities::new(RuntimeGenesisConfig::default().build_storage().unwrap()) .execute_with(|| { advance_block(); let account = Sr25519Keyring::Alice; let who: AccountId = account.to_account_id(); - // Fund Alice for batch fee overhead. + // Fund Alice for fees. use frame_support::traits::fungible::Mutate; Balances::mint_into(&who, 1_000_000_000_000).unwrap(); @@ -933,21 +917,22 @@ fn batch_store_with_mixed_preimage_and_account_auth() { let batch = RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![store_a, store_b] }); - // Batch should succeed — store_a uses preimage auth, store_b uses account auth. - let res = construct_and_apply_extrinsic(Some(account.pair()), batch); - assert!(res.is_ok(), "apply_extrinsic failed: {res:?}"); - assert!(res.unwrap().is_ok(), "dispatch failed"); + // Batch containing store calls is rejected. + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), batch), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); - // Both authorizations should be consumed. + // Authorizations were NOT consumed (rejected before prepare). assert_eq!( TransactionStorage::preimage_authorization_extent(content_hash_a), - AuthorizationExtent { transactions: 0, bytes: 0 }, - "Preimage authorization for data_a should be consumed", + AuthorizationExtent { transactions: 1, bytes: 100 }, + "Preimage authorization should not be consumed", ); assert_eq!( TransactionStorage::account_authorization_extent(who), - AuthorizationExtent { transactions: 0, bytes: 0 }, - "Account authorization for data_b should be consumed", + AuthorizationExtent { transactions: 1, bytes: 200 }, + "Account authorization should not be consumed", ); }); } @@ -1084,15 +1069,23 @@ fn wrapped_renew_requires_authorization() { index: 0, }); - // Direct + utility wrappers: rejected at validation time (no authorization for renew). - assert_rejected_at_validation( - account, - renew_call.clone(), - TransactionValidityError::Invalid(InvalidTransaction::Payment), - "renew", + // Direct: rejected for missing authorization. + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), renew_call.clone()), + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + "renew: direct", ); - // sudo_as: inner renew is checked for TransactionStorage auth at validation. + // Utility wrappers: rejected because renew is not allowed inside wrappers. + for (wrapped, name) in wrap_call_utility_variants(renew_call.clone()) { + assert_eq!( + construct_and_apply_extrinsic(Some(account.pair()), wrapped), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + "renew: via {name}", + ); + } + + // sudo_as: renew inside wrapper is rejected. assert_eq!( construct_and_apply_extrinsic( Some(account.pair()), @@ -1101,7 +1094,7 @@ fn wrapped_renew_requires_authorization() { call: Box::new(renew_call), }), ), - Err(TransactionValidityError::Invalid(InvalidTransaction::Payment)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); }); } @@ -1194,9 +1187,8 @@ fn wrapped_authorize_account_succeeds() { }); } -/// Batch mixing store (needs Authorized origin) with authorize_account (needs Signed origin) -/// is rejected at validation time to prevent silent dispatch failures or authorization leaks -/// with `batch_all`. +/// Batch containing store is rejected — store must be submitted as direct extrinsics, +/// regardless of what else is in the batch. #[test] fn mixed_batch_store_and_authorize_rejected() { sp_io::TestExternalities::new(RuntimeGenesisConfig::default().build_storage().unwrap()) @@ -1254,8 +1246,7 @@ fn mixed_batch_store_and_authorize_rejected() { }); } -/// Batch mixing store with a non-storage call (e.g. System::remark) is rejected at -/// validation to prevent authorization leaks. +/// Batch containing store with a non-storage call is rejected — store must be direct. #[test] fn mixed_batch_store_and_non_storage_call_rejected() { sp_io::TestExternalities::new(RuntimeGenesisConfig::default().build_storage().unwrap()) @@ -1328,10 +1319,11 @@ fn max_recursion_depth_is_enforced() { call = RuntimeCall::Utility(pallet_utility::Call::batch { calls: vec![call] }); } - // Should fail with ExhaustsResources due to depth limit. + // Should fail with Call — store inside wrapper is rejected (the depth limit + // in is_storage_mutating_call treats excessively nested calls as storage-mutating). assert_err!( construct_and_apply_extrinsic(Some(account.pair()), call), - TransactionValidityError::Invalid(InvalidTransaction::ExhaustsResources) + TransactionValidityError::Invalid(InvalidTransaction::Call) ); }); }