[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

@@ -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(())
 	}
 }

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];

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]

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 = ();

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

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 { .. }));
+}

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;

prdoc/pr_7605.prdoc

+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