XCM: Process PayFees only once (#7641)

The `PayFees` instruction should only ever be used once. If it's used
more than once, it's just a noop.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
(cherry picked from commit 46139cd6)

polkadot/xcm/src/v5/mod.rs

@@ -1050,10 +1050,11 @@ pub enum Instruction<Call> {
 	/// Errors: If the given origin is `Some` and not equal to the current Origin register.
 	UnpaidExecution { weight_limit: WeightLimit, check_origin: Option<Location> },
 
-	/// Pay Fees.
+	/// Takes an asset, uses it to pay for execution and puts the rest in the fees register.
 	///
 	/// Successor to `BuyExecution`.
-	/// Defined in fellowship RFC 105.
+	/// Defined in [Fellowship RFC 105](https://github.com/polkadot-fellows/RFCs/pull/105).
+	/// Subsequent `PayFees` after the first one are noops.
 	#[builder(pays_fees)]
 	PayFees { asset: Asset },
 

polkadot/xcm/xcm-executor/src/lib.rs

@@ -93,6 +93,7 @@ pub struct XcmExecutor<Config: config::Config> {
 	/// Stores the current message's weight.
 	message_weight: Weight,
 	asset_claimer: Option<Location>,
+	already_paid_fees: bool,
 	_config: PhantomData<Config>,
 }
 
@@ -200,6 +201,9 @@ impl<Config: config::Config> XcmExecutor<Config> {
 	pub fn set_message_weight(&mut self, weight: Weight) {
 		self.message_weight = weight;
 	}
+	pub fn already_paid_fees(&self) -> bool {
+		self.already_paid_fees
+	}
 }
 
 pub struct WeighedMessage<Call>(Weight, Xcm<Call>);
@@ -356,6 +360,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			asset_used_in_buy_execution: None,
 			message_weight: Weight::zero(),
 			asset_claimer: None,
+			already_paid_fees: false,
 			_config: PhantomData,
 		}
 	}
@@ -1355,14 +1360,12 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				result
 			},
 			PayFees { asset } => {
-				// Message was not weighed, there is nothing to pay.
-				if self.message_weight == Weight::zero() {
-					tracing::warn!(
-						target: "xcm::executor::PayFees",
-						"Message was not weighed or weight was 0. Nothing will be charged.",
-					);
+				// If we've already paid for fees, do nothing.
+				if self.already_paid_fees {
 					return Ok(());
 				}
+				// Make sure `PayFees` won't be processed again.
+				self.already_paid_fees = true;
 				// Record old holding in case we need to rollback.
 				let old_holding = self.holding.clone();
 				// The max we're willing to pay for fees is decided by the `asset` operand.
@@ -1371,19 +1374,21 @@ impl<Config: config::Config> XcmExecutor<Config> {
 					asset_for_fees = ?asset,
 					message_weight = ?self.message_weight,
 				);
-				let max_fee =
-					self.holding.try_take(asset.into()).map_err(|_| XcmError::NotHoldingFees)?;
 				// Pay for execution fees.
 				let result = Config::TransactionalProcessor::process(|| {
+					let max_fee =
+						self.holding.try_take(asset.into()).map_err(|_| XcmError::NotHoldingFees)?;
 					let unspent =
 						self.trader.buy_weight(self.message_weight, max_fee, &self.context)?;
-					// Move unspent to the `fees` register.
+					// Move unspent to the `fees` register, it can later be moved to holding
+					// by calling `RefundSurplus`.
 					self.fees.subsume_assets(unspent);
 					Ok(())
 				});
 				if Config::TransactionalProcessor::IS_TRANSACTIONAL && result.is_err() {
-					// Rollback.
+					// Rollback on error.
 					self.holding = old_holding;
+					self.already_paid_fees = false;
 				}
 				result
 			},

polkadot/xcm/xcm-executor/src/tests/mock.rs

@@ -30,8 +30,8 @@ use xcm::prelude::*;
 
 use crate::{
 	traits::{
-		DropAssets, FeeManager, Properties, ShouldExecute, TransactAsset, WeightBounds,
-		WeightTrader,
+		DropAssets, FeeManager, ProcessTransaction, Properties, ShouldExecute, TransactAsset,
+		WeightBounds, WeightTrader,
 	},
 	AssetsInHolding, Config, FeeReason, XcmExecutor,
 };
@@ -267,6 +267,20 @@ impl FeeManager for TestFeeManager {
 	fn handle_fee(_: Assets, _: Option<&XcmContext>, _: FeeReason) {}
 }
 
+/// Dummy transactional processor that doesn't rollback storage changes, just
+/// aims to rollback executor state.
+pub struct TestTransactionalProcessor;
+impl ProcessTransaction for TestTransactionalProcessor {
+	const IS_TRANSACTIONAL: bool = true;
+
+	fn process<F>(f: F) -> Result<(), XcmError>
+	where
+		F: FnOnce() -> Result<(), XcmError>,
+	{
+		f()
+	}
+}
+
 /// Test XcmConfig that uses all the test implementations in this file.
 pub struct XcmConfig;
 impl Config for XcmConfig {
@@ -294,7 +308,7 @@ impl Config for XcmConfig {
 	type CallDispatcher = Self::RuntimeCall;
 	type SafeCallFilter = Everything;
 	type Aliasers = Nothing;
-	type TransactionalProcessor = ();
+	type TransactionalProcessor = TestTransactionalProcessor;
 	type HrmpNewChannelOpenRequestHandler = ();
 	type HrmpChannelAcceptedHandler = ();
 	type HrmpChannelClosingHandler = ();

polkadot/xcm/xcm-executor/src/tests/pay_fees.rs

@@ -78,6 +78,7 @@ fn works_for_delivery_fees() {
 	// Build xcm.
 	let xcm = Xcm::<TestCall>::builder()
 		.withdraw_asset((Here, 100u128))
+		// We load `10` plancks to pay for fees.
 		.pay_fees((Here, 10u128))
 		// Send a bunch of messages, each charging delivery fees.
 		.report_error(query_response_info.clone())
@@ -212,6 +213,36 @@ fn putting_all_assets_in_pay_fees() {
 	assert_eq!(asset_list(RECIPIENT), []);
 }
 
+#[test]
+fn putting_more_than_available_fails() {
+	// Make sure the sender has enough funds to withdraw.
+	add_asset(SENDER, (Here, 100u128));
+
+	// Build xcm.
+	let xcm = Xcm::<TestCall>::builder()
+		.withdraw_asset((Here, 100u128))
+		.pay_fees((Here, 200u128)) // 200% destined for fees, there's not even that much!
+		.deposit_asset(All, RECIPIENT)
+		.build();
+
+	let (mut vm, weight) = instantiate_executor(SENDER, xcm.clone());
+
+	// Program fails.
+	assert!(vm.bench_process(xcm).is_err());
+
+	// Everything is left in the `holding` register.
+	assert_eq!(get_first_fungible(vm.holding()).unwrap(), (Here, 100u128).into());
+	// Nothing in the `fees` register.
+	assert_eq!(get_first_fungible(vm.fees()), None);
+
+	// The recipient received no assets since message failed.
+	assert_eq!(asset_list(RECIPIENT), []);
+
+	// Leftover assets get trapped.
+	assert!(vm.bench_post_process(weight).ensure_complete().is_ok());
+	assert_eq!(asset_list(TRAPPED_ASSETS), [(Here, 100u128).into()]);
+}
+
 #[test]
 fn refunding_too_early() {
 	// Make sure the sender has enough funds to withdraw.
@@ -255,3 +286,58 @@ fn refunding_too_early() {
 	// No messages were "sent".
 	assert_eq!(sent_xcm(), Vec::new());
 }
+
+#[test]
+fn pay_fees_is_processed_only_once() {
+	// Make sure the sender has enough funds to withdraw.
+	add_asset(SENDER, (Here, 100u128));
+
+	let xcm = Xcm::<TestCall>::builder()
+		.withdraw_asset((Here, 100u128))
+		.pay_fees((Here, 10u128))
+		// Will both be a noop.
+		.pay_fees((Parent, 10u128))
+		.pay_fees((Here, 200u128))
+		.deposit_asset(All, RECIPIENT)
+		.build();
+
+	let (mut vm, weight) = instantiate_executor(SENDER, xcm.clone());
+
+	// Program runs successfully.
+	assert!(vm.bench_process(xcm).is_ok());
+
+	// Nothing left in the `holding` register.
+	assert_eq!(get_first_fungible(vm.holding()), None);
+	// Only the first `PayFees` was executed and execution fees are 4,
+	// so there are 6 fees left in the `fees` register.
+	assert_eq!(get_first_fungible(vm.fees()).unwrap(), (Here, 6u128).into());
+
+	// Leftover fees get trapped.
+	assert!(vm.bench_post_process(weight).ensure_complete().is_ok());
+	assert_eq!(asset_list(TRAPPED_ASSETS), [(Here, 6u128).into()]);
+}
+
+#[test]
+fn already_paid_fees_rolls_back_on_error() {
+	// Make sure the sender has enough funds to withdraw.
+	add_asset(SENDER, (Here, 100u128));
+
+	let xcm = Xcm::<TestCall>::builder()
+		.withdraw_asset((Here, 100u128))
+		.pay_fees((Here, 200u128))
+		.deposit_asset(All, RECIPIENT)
+		.build();
+
+	let (mut vm, _) = instantiate_executor(SENDER, xcm.clone());
+
+	// Program fails.
+	assert!(vm.bench_process(xcm).is_err());
+
+	// Everything left in the `holding` register.
+	assert_eq!(get_first_fungible(vm.holding()).unwrap(), (Here, 100u128).into());
+	// Nothing in the `fees` register.
+	assert_eq!(get_first_fungible(vm.fees()), None);
+
+	// Already paid fees is false.
+	assert_eq!(vm.already_paid_fees(), false);
+}

prdoc/pr_7641.prdoc

+title: 'XCM: Process PayFees only once'
+doc:
+- audience: Runtime Dev
+  description: "The `PayFees` instruction should only ever be used once. If it's used\
+    \ more than once, it's just a noop.\r\n"
+crates:
+- name: staging-xcm-executor
+  bump: patch
+- name: staging-xcm
+  bump: patch