Fix issue with InitiateTransfer and UnpaidExecution (#7423)

Fix issue where setting the `remote_fees` field of `InitiateTransfer` to
`None` could lead to unintended bypassing of fees in certain conditions.

Changes made to fix this:
- `remote_fees: None` now results in the `UnpaidExecution` instruction
being appended *after* the origin altering instruction, be it
`AliasOrigin` or `ClearOrigin`. This means `preserve_origin: true` must
be set if you want to have any chance of not paying for fees.
- The `AliasOrigin` instruction is not appended if the executor is
called with the root location (`Here`) since it would alias to itself.
Although this self-aliasing could be done, it needs the ecosystem to add
a new aliasing instruction, so we just skip it.
- Tweaked the `AllowExplicitUnpaidExecutionFrom` barrier to allow
receiving assets (via teleport or reserve asset transfer) and altering
the origin before actually using `UnpaidExecution`. This is to allow
unpaid teleports to work with `InitiateTransfer`.
- For this, the barrier now executes origin altering instructions and
keeps track of the modified origin. It then checks if this final origin
has enough permissions to not pay for fees. In order to follow the
`AliasOrigin` instruction it now takes a new generic `Aliasers` that
should be set to the XCM config item of the same name. This new generic
has a default value of `()`, effectively disallowing the use of
`AliasOrigin`.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Adrian Catangiu <[email protected]>

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