From ca929c305c38338b75086d061ca2633212ae6b17 Mon Sep 17 00:00:00 2001
From: "paritytech-cmd-bot-polkadot-sdk[bot]"
<179002856+paritytech-cmd-bot-polkadot-sdk[bot]@users.noreply.github.com>
Date: Tue, 18 Feb 2025 19:22:39 +0100
Subject: [PATCH] [stable2412] Backport #7423 (#7605)
Backport #7423 into `stable2412` from franciscoaguirre.
See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.
<!--
# To be used by other automation, do not modify:
original-pr-number: #${pull_number}
-->
---------
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
---
polkadot/xcm/xcm-builder/src/barriers.rs | 91 ++++-
polkadot/xcm/xcm-builder/src/matcher.rs | 14 +
.../xcm/xcm-builder/src/tests/barriers.rs | 374 +++++++++++++++++-
polkadot/xcm/xcm-builder/src/tests/mock.rs | 11 +-
polkadot/xcm/xcm-executor/src/lib.rs | 42 +-
.../src/tests/initiate_transfer.rs | 113 ++++++
polkadot/xcm/xcm-executor/src/tests/mock.rs | 29 +-
prdoc/pr_7605.prdoc | 17 +
8 files changed, 652 insertions(+), 39 deletions(-)
create mode 100644 prdoc/pr_7605.prdoc
diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs
index 27153a3f441..ab19495841a 100644
--- a/polkadot/xcm/xcm-builder/src/barriers.rs
+++ b/polkadot/xcm/xcm-builder/src/barriers.rs
@@ -20,7 +20,7 @@ use crate::{CreateMatcher, MatchXcm};
use core::{cell::Cell, marker::PhantomData, ops::ControlFlow, result::Result};
use frame_support::{
ensure,
- traits::{Contains, Get, ProcessMessageError},
+ traits::{Contains, ContainsPair, Get, Nothing, ProcessMessageError},
};
use polkadot_parachain_primitives::primitives::IsSystem;
use xcm::prelude::*;
@@ -290,11 +290,25 @@ impl<T: Contains<Location>> ShouldExecute for AllowUnpaidExecutionFrom<T> {
}
/// Allows execution from any origin that is contained in `T` (i.e. `T::Contains(origin)`) if the
-/// message begins with the instruction `UnpaidExecution`.
+/// message explicitly includes the `UnpaidExecution` instruction.
///
/// Use only for executions from trusted origin groups.
-pub struct AllowExplicitUnpaidExecutionFrom<T>(PhantomData<T>);
-impl<T: Contains<Location>> ShouldExecute for AllowExplicitUnpaidExecutionFrom<T> {
+///
+/// Allows for the message to receive teleports or reserve asset transfers and altering
+/// the origin before indicating `UnpaidExecution`.
+///
+/// Origin altering instructions are executed so the barrier can more accurately reject messages
+/// whose effective origin at the time of calling `UnpaidExecution` is not allowed.
+/// This means `T` will be checked against the actual origin _after_ being modified by prior
+/// instructions.
+///
+/// In order to execute the `AliasOrigin` instruction, the `Aliasers` type should be set to the same
+/// `Aliasers` item in the XCM configuration. If it isn't, then all messages with an `AliasOrigin`
+/// instruction will be rejected.
+pub struct AllowExplicitUnpaidExecutionFrom<T, Aliasers = Nothing>(PhantomData<(T, Aliasers)>);
+impl<T: Contains<Location>, Aliasers: ContainsPair<Location, Location>> ShouldExecute
+ for AllowExplicitUnpaidExecutionFrom<T, Aliasers>
+{
fn should_execute<Call>(
origin: &Location,
instructions: &mut [Instruction<Call>],
@@ -306,12 +320,69 @@ impl<T: Contains<Location>> ShouldExecute for AllowExplicitUnpaidExecutionFrom<T
"AllowExplicitUnpaidExecutionFrom origin: {:?}, instructions: {:?}, max_weight: {:?}, properties: {:?}",
origin, instructions, max_weight, _properties,
);
- ensure!(T::contains(origin), ProcessMessageError::Unsupported);
- instructions.matcher().match_next_inst(|inst| match inst {
- UnpaidExecution { weight_limit: Limited(m), .. } if m.all_gte(max_weight) => Ok(()),
- UnpaidExecution { weight_limit: Unlimited, .. } => Ok(()),
- _ => Err(ProcessMessageError::Overweight(max_weight)),
- })?;
+ // We will read up to 5 instructions before `UnpaidExecution`.
+ // This allows up to 3 asset transfer instructions, thus covering all possible transfer
+ // types, followed by a potential origin altering instruction, and a potential `SetHints`.
+ let mut actual_origin = origin.clone();
+ let processed = Cell::new(0usize);
+ let instructions_to_process = 5;
+ instructions
+ .matcher()
+ // We skip set hints and all types of asset transfer instructions.
+ .match_next_inst_while(
+ |inst| {
+ processed.get() < instructions_to_process &&
+ matches!(
+ inst,
+ ReceiveTeleportedAsset(_) |
+ ReserveAssetDeposited(_) | WithdrawAsset(_) |
+ SetHints { .. }
+ )
+ },
+ |_| {
+ processed.set(processed.get() + 1);
+ Ok(ControlFlow::Continue(()))
+ },
+ )?
+ // Then we go through all origin altering instructions and we
+ // alter the original origin.
+ .match_next_inst_while(
+ |_| processed.get() < instructions_to_process,
+ |inst| {
+ match inst {
+ ClearOrigin => {
+ // We don't support the `ClearOrigin` instruction since we always need
+ // to know the origin to know if it's allowed unpaid execution.
+ return Err(ProcessMessageError::Unsupported);
+ },
+ AliasOrigin(target) =>
+ if Aliasers::contains(&actual_origin, &target) {
+ actual_origin = target.clone();
+ } else {
+ return Err(ProcessMessageError::Unsupported);
+ },
+ DescendOrigin(child) if child != &Here => {
+ let Ok(_) = actual_origin.append_with(child.clone()) else {
+ return Err(ProcessMessageError::Unsupported);
+ };
+ },
+ _ => return Ok(ControlFlow::Break(())),
+ };
+ processed.set(processed.get() + 1);
+ Ok(ControlFlow::Continue(()))
+ },
+ )?
+ // We finally match on the required `UnpaidExecution` instruction.
+ .match_next_inst(|inst| match inst {
+ UnpaidExecution { weight_limit: Limited(m), .. } if m.all_gte(max_weight) => Ok(()),
+ UnpaidExecution { weight_limit: Unlimited, .. } => Ok(()),
+ _ => Err(ProcessMessageError::Overweight(max_weight)),
+ })?;
+
+ // After processing all the instructions, `actual_origin` was modified and we
+ // check if it's allowed to have unpaid execution.
+ ensure!(T::contains(&actual_origin), ProcessMessageError::Unsupported);
+
Ok(())
}
}
diff --git a/polkadot/xcm/xcm-builder/src/matcher.rs b/polkadot/xcm/xcm-builder/src/matcher.rs
index ab515f18052..2fed64bccde 100644
--- a/polkadot/xcm/xcm-builder/src/matcher.rs
+++ b/polkadot/xcm/xcm-builder/src/matcher.rs
@@ -179,6 +179,20 @@ mod tests {
use std::{vec, vec::Vec};
use xcm::latest::prelude::*;
+ #[test]
+ fn match_next_inst_works() {
+ let test_cases: Vec<(Vec<Instruction<()>>, bool)> =
+ vec![(vec![ClearOrigin], true), (vec![Trap(0)], false)];
+
+ for (mut xcm, expected) in test_cases.into_iter() {
+ let result = xcm.matcher().match_next_inst(|inst| match inst {
+ ClearOrigin => Ok(()),
+ _ => Err(ProcessMessageError::Unsupported),
+ });
+ assert_eq!(result.is_ok(), expected);
+ }
+ }
+
#[test]
fn match_next_inst_while_works() {
let mut xcm: Vec<Instruction<()>> = vec![ClearOrigin];
diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs
index d8805274d3a..c95b2fc6942 100644
--- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs
+++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs
@@ -147,9 +147,10 @@ fn allow_explicit_unpaid_should_work() {
TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() },
]);
- AllowExplicitUnpaidFrom::set(vec![Parent.into()]);
+ AllowExplicitUnpaidFrom::set(vec![Parent.into(), (Parent, Parachain(1000)).into()]);
+ type ExplicitUnpaidBarrier<T> = AllowExplicitUnpaidExecutionFrom<T, mock::Aliasers>;
- let r = AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ let r = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
&Parachain(1).into(),
good_message.inner_mut(),
Weight::from_parts(20, 20),
@@ -157,7 +158,7 @@ fn allow_explicit_unpaid_should_work() {
);
assert_eq!(r, Err(ProcessMessageError::Unsupported));
- let r = AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ let r = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
&Parent.into(),
bad_message1.inner_mut(),
Weight::from_parts(20, 20),
@@ -165,7 +166,7 @@ fn allow_explicit_unpaid_should_work() {
);
assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20))));
- let r = AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ let r = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
&Parent.into(),
bad_message2.inner_mut(),
Weight::from_parts(20, 20),
@@ -173,7 +174,7 @@ fn allow_explicit_unpaid_should_work() {
);
assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20))));
- let r = AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ let r = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
&Parent.into(),
good_message.inner_mut(),
Weight::from_parts(20, 20),
@@ -189,7 +190,7 @@ fn allow_explicit_unpaid_should_work() {
TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() },
]);
- let r = AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ let r = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
&Parent.into(),
message_with_different_weight_parts.inner_mut(),
Weight::from_parts(20, 20),
@@ -197,13 +198,372 @@ fn allow_explicit_unpaid_should_work() {
);
assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20))));
- let r = AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ let r = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
&Parent.into(),
message_with_different_weight_parts.inner_mut(),
Weight::from_parts(10, 10),
&mut props(Weight::zero()),
);
assert_eq!(r, Ok(()));
+
+ // Invalid since location to alias is not allowed.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .receive_teleported_asset((Here, 100u128))
+ .alias_origin(Parachain(1000))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(30, 30),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Unsupported));
+
+ // Valid because all parachains are children of the relay chain.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .receive_teleported_asset((Here, 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(30, 30),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ // Valid.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(30, 30),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ // Invalid because `ClearOrigin` clears origin and `UnpaidExecution`
+ // can't know if there are enough permissions.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .receive_teleported_asset((Here, 100u128))
+ .clear_origin()
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(30, 30),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Unsupported));
+
+ // Valid.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited((Parent, 100u128))
+ .descend_origin(Parachain(1000))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(40, 40),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ // Invalid because of `ClearOrigin`.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .receive_teleported_asset((Here, 100u128))
+ .clear_origin()
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(30, 30),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Unsupported));
+
+ // Invalid because there is no `UnpaidExecution`.
+ let mut message = Xcm::<()>::builder_unsafe()
+ .receive_teleported_asset((Here, 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(30, 30),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::BadFormat));
+
+ // Invalid because even though alias is valid, it can't use `UnpaidExecution`.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, AccountId32 { id: [128u8; 32], network: None }))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(60, 60),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Unsupported));
+
+ // Invalid because `UnpaidExecution` specifies less weight than needed.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Limited(Weight::from_parts(50, 50)), None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(60, 60),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Overweight(Weight::from_parts(60, 60))));
+
+ // Invalid because of too many instructions before `UnpaidExecution`.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, AccountId32 { id: [128u8; 32], network: None }))
+ .unpaid_execution(Limited(Weight::from_parts(50, 50)), None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(70, 70),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Overweight(Weight::from_parts(70, 70))));
+
+ // Valid.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(60, 60),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ // Valid.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .descend_origin(Parachain(1000))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = ExplicitUnpaidBarrier::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(60, 60),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+}
+
+#[test]
+fn allow_explicit_unpaid_fails_with_alias_origin_if_no_aliasers() {
+ AllowExplicitUnpaidFrom::set(vec![(Parent, Parachain(1000)).into()]);
+
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut good_message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .descend_origin(Parachain(1000))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result =
+ AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ good_message.inner_mut(),
+ Weight::from_parts(100, 100),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut bad_message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ // Barrier has `Aliasers` set as `Nothing` by default, rejecting message if it
+ // has an `AliasOrigin` instruction.
+ let result =
+ AllowExplicitUnpaidExecutionFrom::<IsInVec<AllowExplicitUnpaidFrom>>::should_execute(
+ &Parent.into(),
+ bad_message.inner_mut(),
+ Weight::from_parts(100, 100),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Unsupported));
+}
+
+#[test]
+fn allow_explicit_unpaid_with_computed_origin() {
+ AllowExplicitUnpaidFrom::set(vec![
+ (Parent, Parachain(1000)).into(),
+ (Parent, Parent, GlobalConsensus(Polkadot), Parachain(1000)).into(),
+ ]);
+ type ExplicitUnpaidBarrier<T> = AllowExplicitUnpaidExecutionFrom<T, mock::Aliasers>;
+
+ // Message that passes without `WithComputedOrigin` should also pass with it.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = WithComputedOrigin::<
+ ExplicitUnpaidBarrier<IsInVec<AllowExplicitUnpaidFrom>>,
+ ExecutorUniversalLocation,
+ ConstU32<2>,
+ >::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(100, 100),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ // Can manipulate origin before the inner barrier.
+ // For example, to act as another network.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .universal_origin(Polkadot)
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, Parent, GlobalConsensus(Polkadot), Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = WithComputedOrigin::<
+ ExplicitUnpaidBarrier<IsInVec<AllowExplicitUnpaidFrom>>,
+ ExecutorUniversalLocation,
+ ConstU32<2>,
+ >::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(100, 100),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Ok(()));
+
+ // Any invalid conversions from the new origin fail.
+ let assets: Vec<Asset> = vec![
+ (Parent, 100u128).into(),
+ ((Parent, PalletInstance(10), GeneralIndex(1000)), 100u128).into(),
+ ];
+ let mut message = Xcm::<()>::builder_unsafe()
+ .universal_origin(Polkadot)
+ .set_hints(vec![AssetClaimer {
+ location: AccountId32 { id: [100u8; 32], network: None }.into(),
+ }])
+ .receive_teleported_asset((Here, 100u128))
+ .reserve_asset_deposited(assets)
+ .withdraw_asset((GeneralIndex(1), 100u128))
+ .alias_origin((Parent, Parachain(1000)))
+ .unpaid_execution(Unlimited, None)
+ .build();
+ let result = WithComputedOrigin::<
+ ExplicitUnpaidBarrier<IsInVec<AllowExplicitUnpaidFrom>>,
+ ExecutorUniversalLocation,
+ ConstU32<2>,
+ >::should_execute(
+ &Parent.into(),
+ message.inner_mut(),
+ Weight::from_parts(100, 100),
+ &mut props(Weight::zero()),
+ );
+ assert_eq!(result, Err(ProcessMessageError::Unsupported));
}
#[test]
diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs
index bec7b253977..47e1198190d 100644
--- a/polkadot/xcm/xcm-builder/src/tests/mock.rs
+++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs
@@ -22,9 +22,9 @@ use crate::{
EnsureDecodableXcm,
};
pub use crate::{
- AliasForeignAccountId32, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
- AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, FixedRateOfFungible,
- FixedWeightBounds, TakeWeightCredit,
+ AliasChildLocation, AliasForeignAccountId32, AllowExplicitUnpaidExecutionFrom,
+ AllowKnownQueryResponses, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom,
+ FixedRateOfFungible, FixedWeightBounds, TakeWeightCredit,
};
pub use alloc::collections::{btree_map::BTreeMap, btree_set::BTreeSet};
pub use codec::{Decode, Encode};
@@ -732,6 +732,9 @@ impl Contains<Location> for ParentPrefix {
}
}
+/// Pairs (location1, location2) where location1 can alias as location2.
+pub type Aliasers = (AliasForeignAccountId32<SiblingPrefix>, AliasChildLocation);
+
pub struct TestConfig;
impl Config for TestConfig {
type RuntimeCall = TestCall;
@@ -757,7 +760,7 @@ impl Config for TestConfig {
type MessageExporter = TestMessageExporter;
type CallDispatcher = TestCall;
type SafeCallFilter = Everything;
- type Aliasers = AliasForeignAccountId32<SiblingPrefix>;
+ type Aliasers = Aliasers;
type TransactionalProcessor = ();
type HrmpNewChannelOpenRequestHandler = ();
type HrmpChannelAcceptedHandler = ();
diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs
index d0f18aea1ab..e2becdbdcd3 100644
--- a/polkadot/xcm/xcm-executor/src/lib.rs
+++ b/polkadot/xcm/xcm-executor/src/lib.rs
@@ -1194,7 +1194,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
// transferring the other assets. This is required to satisfy the
// `MAX_ASSETS_FOR_BUY_EXECUTION` limit in the `AllowTopLevelPaidExecutionFrom`
// barrier.
- if let Some(remote_fees) = remote_fees {
+ let remote_fees_paid = if let Some(remote_fees) = remote_fees {
let reanchored_fees = match remote_fees {
AssetTransferFilter::Teleport(fees_filter) => {
let teleport_fees = self
@@ -1239,11 +1239,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
// move these assets to the fees register for covering execution and paying
// any subsequent fees
message.push(PayFees { asset: fees });
+ true
} else {
- // unpaid execution
- message
- .push(UnpaidExecution { weight_limit: Unlimited, check_origin: None });
- }
+ false
+ };
// add any extra asset transfers
for asset_filter in assets {
@@ -1270,23 +1269,36 @@ impl<Config: config::Config> XcmExecutor<Config> {
)?,
};
}
+
if preserve_origin {
- // preserve current origin for subsequent user-controlled instructions on
- // remote chain
- let original_origin = self
+ // We alias the origin if it's not a noop (origin != `Here`).
+ if let Some(original_origin) = self
.origin_ref()
+ .filter(|origin| *origin != &Location::here())
.cloned()
- .and_then(|origin| {
- Self::try_reanchor(origin, &destination)
- .map(|(reanchored, _)| reanchored)
- .ok()
- })
- .ok_or(XcmError::BadOrigin)?;
- message.push(AliasOrigin(original_origin));
+ {
+ // preserve current origin for subsequent user-controlled instructions on
+ // remote chain
+ let reanchored_origin = Self::try_reanchor(original_origin, &destination)?.0;
+ message.push(AliasOrigin(reanchored_origin));
+ }
} else {
// clear origin for subsequent user-controlled instructions on remote chain
message.push(ClearOrigin);
}
+
+ // If not intending to pay for fees then we append the `UnpaidExecution`
+ // _AFTER_ origin altering instructions.
+ // When origin is not preserved, it's probably going to fail on the receiver.
+ if !remote_fees_paid {
+ // We push the UnpaidExecution instruction to notify we do not intend to pay
+ // for fees.
+ // The receiving chain must decide based on the origin of the message if they
+ // accept this.
+ message
+ .push(UnpaidExecution { weight_limit: Unlimited, check_origin: None });
+ }
+
// append custom instructions
message.extend(remote_xcm.0.into_iter());
// send the onward XCM
diff --git a/polkadot/xcm/xcm-executor/src/tests/initiate_transfer.rs b/polkadot/xcm/xcm-executor/src/tests/initiate_transfer.rs
index 09ed1f44cc4..8786186006b 100644
--- a/polkadot/xcm/xcm-executor/src/tests/initiate_transfer.rs
+++ b/polkadot/xcm/xcm-executor/src/tests/initiate_transfer.rs
@@ -20,6 +20,7 @@
//! [Fellowship RFC 122](https://github.com/polkadot-fellows/rfCs/pull/122), and the
//! [specification](https://github.com/polkadot-fellows/xcm-format) for more information.
+use codec::Encode;
use xcm::{latest::AssetTransferFilter, prelude::*};
use super::mock::*;
@@ -104,3 +105,115 @@ fn preserves_origin() {
assert!(matches!(instr.next().unwrap(), RefundSurplus));
assert!(matches!(instr.next().unwrap(), DepositAsset { .. }));
}
+
+#[test]
+fn unpaid_execution_goes_after_origin_alteration() {
+ // Make sure the sender has enough funds to withdraw.
+ add_asset(SENDER, (Here, 100u128));
+
+ let xcm_on_destination =
+ Xcm::builder_unsafe().refund_surplus().deposit_asset(All, RECIPIENT).build();
+ let asset: Asset = (Here, 90u128).into();
+ let xcm = Xcm::builder()
+ .withdraw_asset((Here, 100u128))
+ .pay_fees((Here, 10u128))
+ .initiate_transfer(
+ Parent,
+ None, // We specify no remote fees.
+ true, // Preserve origin, necessary for `UnpaidExecution`.
+ vec![AssetTransferFilter::ReserveDeposit(asset.into())],
+ xcm_on_destination,
+ )
+ .build();
+
+ // We initialize the executor with the SENDER origin, which is not waived.
+ let (mut vm, _) = instantiate_executor(SENDER, xcm.clone());
+
+ // Program fails with `BadOrigin`.
+ let result = vm.bench_process(xcm);
+ assert!(result.is_ok(), "execution error {:?}", result);
+
+ let (destination, sent_message) = sent_xcm().pop().unwrap();
+ assert_eq!(destination, Parent.into());
+ assert_eq!(sent_message.len(), 5);
+ let mut instructions = sent_message.inner().iter();
+ assert!(matches!(instructions.next().unwrap(), ReserveAssetDeposited(..)));
+ assert!(matches!(
+ instructions.next().unwrap(),
+ AliasOrigin(origin) if matches!(origin.unpack(), (0, [Parachain(1000), AccountId32 { id: SENDER, network: None }]))
+ ));
+ assert!(matches!(instructions.next().unwrap(), UnpaidExecution { .. }));
+ assert!(matches!(instructions.next().unwrap(), RefundSurplus));
+ assert!(matches!(instructions.next().unwrap(), DepositAsset { .. }));
+}
+
+#[test]
+fn no_alias_origin_if_root() {
+ // Make sure the sender has enough funds to withdraw.
+ add_asset(Here, (Here, 100u128));
+
+ let xcm_on_destination =
+ Xcm::builder_unsafe().refund_surplus().deposit_asset(All, RECIPIENT).build();
+ let asset: Asset = (Here, 90u128).into();
+ let xcm = Xcm::builder()
+ .withdraw_asset((Here, 100u128))
+ .pay_fees((Here, 10u128))
+ .initiate_transfer(
+ Parent,
+ None, // We specify no remote fees.
+ true, // Preserve origin, necessary for `UnpaidExecution`.
+ vec![AssetTransferFilter::ReserveDeposit(asset.into())],
+ xcm_on_destination,
+ )
+ .build();
+
+ // We initialize the executor with the SENDER origin, which is not waived.
+ let (mut vm, _) = instantiate_executor(Here, xcm.clone());
+
+ // Program fails with `BadOrigin`.
+ let result = vm.bench_process(xcm);
+ assert!(result.is_ok(), "execution error {:?}", result);
+
+ let (destination, sent_message) = sent_xcm().pop().unwrap();
+ assert_eq!(destination, Parent.into());
+ assert_eq!(sent_message.len(), 4);
+ let mut instructions = sent_message.inner().iter();
+ assert!(matches!(instructions.next().unwrap(), ReserveAssetDeposited(..)));
+ assert!(matches!(instructions.next().unwrap(), UnpaidExecution { .. }));
+ assert!(matches!(instructions.next().unwrap(), RefundSurplus));
+ assert!(matches!(instructions.next().unwrap(), DepositAsset { .. }));
+}
+
+// We simulate going from one system parachain to another without
+// having to pay remote fees.
+#[test]
+fn unpaid_transact() {
+ let to_another_system_para: Location = (Parent, Parachain(1001)).into();
+ // We want to execute some call in the receiving chain.
+ let xcm_on_destination = Xcm::builder_unsafe()
+ .transact(OriginKind::Superuser, None, b"".encode())
+ .build();
+ let xcm = Xcm::builder_unsafe()
+ .initiate_transfer(
+ to_another_system_para.clone(),
+ None, // We specify no remote fees.
+ true, // Preserve necessary for `UnpaidExecution`.
+ vec![], // No need for assets.
+ xcm_on_destination,
+ )
+ .build();
+
+ // We initialize the executor with the root origin, which is waived.
+ let (mut vm, _) = instantiate_executor(Here, xcm.clone());
+
+ // Program executes successfully.
+ let result = vm.bench_process(xcm.clone());
+ assert!(result.is_ok(), "execution error: {:?}", result);
+
+ let (destination, sent_message) = sent_xcm().pop().unwrap();
+ assert_eq!(destination, to_another_system_para);
+ assert_eq!(sent_message.len(), 2);
+ let mut instructions = sent_message.inner().iter();
+ assert!(matches!(instructions.next().unwrap(), UnpaidExecution { .. }));
+ assert!(matches!(instructions.next().unwrap(), Transact { .. }));
+}
diff --git a/polkadot/xcm/xcm-executor/src/tests/mock.rs b/polkadot/xcm/xcm-executor/src/tests/mock.rs
index 9cf258331f3..c0bcfe88d2b 100644
--- a/polkadot/xcm/xcm-executor/src/tests/mock.rs
+++ b/polkadot/xcm/xcm-executor/src/tests/mock.rs
@@ -29,8 +29,11 @@ use sp_runtime::traits::Dispatchable;
use xcm::prelude::*;
use crate::{
- traits::{DropAssets, Properties, ShouldExecute, TransactAsset, WeightBounds, WeightTrader},
- AssetsInHolding, Config, XcmExecutor,
+ traits::{
+ DropAssets, FeeManager, Properties, ShouldExecute, TransactAsset, WeightBounds,
+ WeightTrader,
+ },
+ AssetsInHolding, Config, FeeReason, XcmExecutor,
};
/// We create an XCVM instance instead of calling `XcmExecutor::<_>::prepare_and_execute` so we
@@ -244,6 +247,26 @@ pub fn sent_xcm() -> Vec<(Location, Xcm<()>)> {
SENT_XCM.with(|q| (*q.borrow()).clone())
}
+/// A mock contract address that doesn't need to pay for fees.
+pub const WAIVED_CONTRACT_ADDRESS: [u8; 20] = [128; 20];
+
+/// Test fee manager that will waive the fee for some origins.
+///
+/// Doesn't do anything with the fee, which effectively burns it.
+pub struct TestFeeManager;
+impl FeeManager for TestFeeManager {
+ fn is_waived(origin: Option<&Location>, _: FeeReason) -> bool {
+ let Some(origin) = origin else { return false };
+ // Match the root origin and a particular smart contract account.
+ matches!(
+ origin.unpack(),
+ (0, []) | (0, [AccountKey20 { network: None, key: WAIVED_CONTRACT_ADDRESS }])
+ )
+ }
+
+ fn handle_fee(_: Assets, _: Option<&XcmContext>, _: FeeReason) {}
+}
+
/// Test XcmConfig that uses all the test implementations in this file.
pub struct XcmConfig;
impl Config for XcmConfig {
@@ -265,7 +288,7 @@ impl Config for XcmConfig {
type SubscriptionService = ();
type PalletInstancesInfo = ();
type MaxAssetsIntoHolding = MaxAssetsIntoHolding;
- type FeeManager = ();
+ type FeeManager = TestFeeManager;
type MessageExporter = ();
type UniversalAliases = Nothing;
type CallDispatcher = Self::RuntimeCall;
diff --git a/prdoc/pr_7605.prdoc b/prdoc/pr_7605.prdoc
new file mode 100644
index 00000000000..30e22a7ccf8
--- /dev/null
+++ b/prdoc/pr_7605.prdoc
@@ -0,0 +1,17 @@
+title: Fix issue with InitiateTransfer and UnpaidExecution
+doc:
+- audience: Runtime Dev
+ description:
+ Fix issue where setting the `remote_fees` field of `InitiateTransfer` to `None` could lead to unintended bypassing of fees in certain conditions.
+ `UnpaidExecution` is now appended **after** origin alteration.
+ If planning to use `UnpaidExecution`, you need to set `preserve_origin = true`.
+
+ The `AllowExplicitUnpaidExecutionFrom` barrier now allows instructions for receiving funds before origin altering instructions before
+ the actual `UnpaidExecution`.
+ It takes a new generic, `Aliasers`, needed for executing `AliasOrigin` to see if the effective origin is allowed to use `UnpaidExecution`.
+ This should be set to the same value as `Aliasers` in the XCM configuration.
+crates:
+- name: staging-xcm-builder
+ bump: patch
+- name: staging-xcm-executor
+ bump: patch
--
GitLab