xcm-executor: validate destinations for ReserveWithdraw and Teleport …

…transfers

This change adds the required validation for stronger UX guarantees when using
`InitiateReserveWithdraw` or `InitiateTeleport` XCM instructions. Execution of
the instructions will fail if the local chain is not configured to trust the
"destination" or "reserve" chain as a reserve/trusted-teleporter for the provided
"assets".

With this change, misuse of `InitiateReserveWithdraw`/`InitiateTeleport` fails on
origin with no overall side-effects, rather than failing on destination (with
side-effects to origin's assets issuance).

The commit also makes the same validations for pallet-xcm transfers, and adds
regression tests.

Signed-off-by: Adrian Catangiu <[email protected]>

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs

@@ -51,6 +51,7 @@ mod imports {
 	pub use rococo_system_emulated_network::{
 		asset_hub_rococo_emulated_chain::{
 			asset_hub_rococo_runtime::{
+				self,
 				xcm_config::{
 					self as ahr_xcm_config, TokenLocation as RelayLocation,
 					XcmConfig as AssetHubRococoXcmConfig,

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/hybrid_transfers.rs

@@ -449,7 +449,16 @@ fn transfer_foreign_assets_from_para_to_para_through_asset_hub() {
 	let sov_of_receiver_on_ah = AssetHubRococo::sovereign_account_id_of(receiver_as_seen_by_ah);
 	let wnd_to_send = ASSET_HUB_ROCOCO_ED * 10_000_000;
 
-	// Configure destination chain to trust AH as reserve of WND
+	// Configure source and destination chains to trust AH as reserve of WND
+	PenpalA::execute_with(|| {
+		assert_ok!(<PenpalA as Chain>::System::set_storage(
+			<PenpalA as Chain>::RuntimeOrigin::root(),
+			vec![(
+				PenpalCustomizableAssetFromSystemAssetHub::key().to_vec(),
+				Location::new(2, [GlobalConsensus(Westend)]).encode(),
+			)],
+		));
+	});
 	PenpalB::execute_with(|| {
 		assert_ok!(<PenpalB as Chain>::System::set_storage(
 			<PenpalB as Chain>::RuntimeOrigin::root(),

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs

@@ -1548,3 +1548,58 @@ fn reserve_transfer_usdt_from_para_to_para_through_asset_hub() {
 	// Receiver's balance is increased
 	assert!(receiver_assets_after > receiver_assets_before);
 }
+
+/// Reserve Withdraw Native Asset from AssetHub to Parachain fails.
+#[test]
+fn reserve_withdraw_from_untrusted_reserve_fails() {
+	// Init values for Parachain Origin
+	let destination = AssetHubRococo::sibling_location_of(PenpalA::para_id());
+	let signed_origin =
+		<AssetHubRococo as Chain>::RuntimeOrigin::signed(AssetHubRococoSender::get().into());
+	let roc_to_send: Balance = ROCOCO_ED * 10000;
+	let roc_location = RelayLocation::get();
+
+	// Assets to send
+	let assets: Vec<Asset> = vec![(roc_location.clone(), roc_to_send).into()];
+	let fee_id: AssetId = roc_location.into();
+
+	// this should fail
+	AssetHubRococo::execute_with(|| {
+		let result = <AssetHubRococo as AssetHubRococoPallet>::PolkadotXcm::transfer_assets_using_type_and_then(
+			signed_origin.clone(),
+			bx!(destination.clone().into()),
+			bx!(assets.clone().into()),
+			bx!(TransferType::DestinationReserve),
+			bx!(fee_id.into()),
+			bx!(TransferType::DestinationReserve),
+			bx!(VersionedXcm::from(Xcm::<()>::new())),
+			Unlimited,
+		);
+		assert_err!(
+			result,
+			DispatchError::Module(sp_runtime::ModuleError {
+				index: 31,
+				error: [22, 0, 0, 0],
+				message: Some("InvalidAssetUnsupportedReserve")
+			})
+		);
+	});
+
+	// this should also fail
+	AssetHubRococo::execute_with(|| {
+		let xcm: Xcm<asset_hub_rococo_runtime::RuntimeCall> = Xcm(vec![
+			WithdrawAsset(assets.into()),
+			InitiateReserveWithdraw {
+				assets: Wild(All),
+				reserve: destination,
+				xcm: Xcm::<()>::new(),
+			},
+		]);
+		let result = <AssetHubRococo as AssetHubRococoPallet>::PolkadotXcm::execute(
+			signed_origin,
+			bx!(xcm::VersionedXcm::V4(xcm)),
+			Weight::MAX,
+		);
+		assert!(result.is_err());
+	});
+}

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/teleport.rs

@@ -527,3 +527,54 @@ fn bidirectional_teleport_foreign_assets_between_para_and_asset_hub() {
 		system_para_to_para_transfer_assets,
 	);
 }
+
+/// Teleport Native Asset from AssetHub to Parachain fails.
+#[test]
+fn teleport_to_untrusted_chain_fails() {
+	// Init values for Parachain Origin
+	let destination = AssetHubRococo::sibling_location_of(PenpalA::para_id());
+	let signed_origin =
+		<AssetHubRococo as Chain>::RuntimeOrigin::signed(AssetHubRococoSender::get().into());
+	let roc_to_send: Balance = ROCOCO_ED * 10000;
+	let roc_location = RelayLocation::get();
+
+	// Assets to send
+	let assets: Vec<Asset> = vec![(roc_location.clone(), roc_to_send).into()];
+	let fee_id: AssetId = roc_location.into();
+
+	// this should fail
+	AssetHubRococo::execute_with(|| {
+		let result = <AssetHubRococo as AssetHubRococoPallet>::PolkadotXcm::transfer_assets_using_type_and_then(
+			signed_origin.clone(),
+			bx!(destination.clone().into()),
+			bx!(assets.clone().into()),
+			bx!(TransferType::Teleport),
+			bx!(fee_id.into()),
+			bx!(TransferType::Teleport),
+			bx!(VersionedXcm::from(Xcm::<()>::new())),
+			Unlimited,
+		);
+		assert_err!(
+			result,
+			DispatchError::Module(sp_runtime::ModuleError {
+				index: 31,
+				error: [2, 0, 0, 0],
+				message: Some("Filtered")
+			})
+		);
+	});
+
+	// this should also fail
+	AssetHubRococo::execute_with(|| {
+		let xcm: Xcm<asset_hub_rococo_runtime::RuntimeCall> = Xcm(vec![
+			WithdrawAsset(assets.into()),
+			InitiateTeleport { assets: Wild(All), dest: destination, xcm: Xcm::<()>::new() },
+		]);
+		let result = <AssetHubRococo as AssetHubRococoPallet>::PolkadotXcm::execute(
+			signed_origin,
+			bx!(xcm::VersionedXcm::V4(xcm)),
+			Weight::MAX,
+		);
+		assert!(result.is_err());
+	});
+}

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs

@@ -48,6 +48,7 @@ mod imports {
 	pub use westend_system_emulated_network::{
 		asset_hub_westend_emulated_chain::{
 			asset_hub_westend_runtime::{
+				self,
 				xcm_config::{
 					self as ahw_xcm_config, WestendLocation as RelayLocation,
 					XcmConfig as AssetHubWestendXcmConfig,

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs

@@ -450,7 +450,16 @@ fn transfer_foreign_assets_from_para_to_para_through_asset_hub() {
 	let sov_of_receiver_on_ah = AssetHubWestend::sovereign_account_id_of(receiver_as_seen_by_ah);
 	let roc_to_send = ASSET_HUB_WESTEND_ED * 10_000_000;
 
-	// Configure destination chain to trust AH as reserve of ROC
+	// Configure source and destination chains to trust AH as reserve of ROC
+	PenpalA::execute_with(|| {
+		assert_ok!(<PenpalA as Chain>::System::set_storage(
+			<PenpalA as Chain>::RuntimeOrigin::root(),
+			vec![(
+				PenpalCustomizableAssetFromSystemAssetHub::key().to_vec(),
+				Location::new(2, [GlobalConsensus(Rococo)]).encode(),
+			)],
+		));
+	});
 	PenpalB::execute_with(|| {
 		assert_ok!(<PenpalB as Chain>::System::set_storage(
 			<PenpalB as Chain>::RuntimeOrigin::root(),

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs

@@ -1552,3 +1552,58 @@ fn reserve_transfer_usdt_from_para_to_para_through_asset_hub() {
 	// Receiver's balance is increased
 	assert!(receiver_assets_after > receiver_assets_before);
 }
+
+/// Reserve Withdraw Native Asset from AssetHub to Parachain fails.
+#[test]
+fn reserve_withdraw_from_untrusted_reserve_fails() {
+	// Init values for Parachain Origin
+	let destination = AssetHubWestend::sibling_location_of(PenpalA::para_id());
+	let signed_origin =
+		<AssetHubWestend as Chain>::RuntimeOrigin::signed(AssetHubWestendSender::get().into());
+	let roc_to_send: Balance = WESTEND_ED * 10000;
+	let roc_location = RelayLocation::get();
+
+	// Assets to send
+	let assets: Vec<Asset> = vec![(roc_location.clone(), roc_to_send).into()];
+	let fee_id: AssetId = roc_location.into();
+
+	// this should fail
+	AssetHubWestend::execute_with(|| {
+		let result = <AssetHubWestend as AssetHubWestendPallet>::PolkadotXcm::transfer_assets_using_type_and_then(
+			signed_origin.clone(),
+			bx!(destination.clone().into()),
+			bx!(assets.clone().into()),
+			bx!(TransferType::DestinationReserve),
+			bx!(fee_id.into()),
+			bx!(TransferType::DestinationReserve),
+			bx!(VersionedXcm::from(Xcm::<()>::new())),
+			Unlimited,
+		);
+		assert_err!(
+			result,
+			DispatchError::Module(sp_runtime::ModuleError {
+				index: 31,
+				error: [22, 0, 0, 0],
+				message: Some("InvalidAssetUnsupportedReserve")
+			})
+		);
+	});
+
+	// this should also fail
+	AssetHubWestend::execute_with(|| {
+		let xcm: Xcm<asset_hub_westend_runtime::RuntimeCall> = Xcm(vec![
+			WithdrawAsset(assets.into()),
+			InitiateReserveWithdraw {
+				assets: Wild(All),
+				reserve: destination,
+				xcm: Xcm::<()>::new(),
+			},
+		]);
+		let result = <AssetHubWestend as AssetHubWestendPallet>::PolkadotXcm::execute(
+			signed_origin,
+			bx!(xcm::VersionedXcm::V4(xcm)),
+			Weight::MAX,
+		);
+		assert!(result.is_err());
+	});
+}

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/teleport.rs

@@ -530,3 +530,54 @@ fn bidirectional_teleport_foreign_assets_between_para_and_asset_hub() {
 		system_para_to_para_transfer_assets,
 	);
 }
+
+/// Teleport Native Asset from AssetHub to Parachain fails.
+#[test]
+fn teleport_to_untrusted_chain_fails() {
+	// Init values for Parachain Origin
+	let destination = AssetHubWestend::sibling_location_of(PenpalA::para_id());
+	let signed_origin =
+		<AssetHubWestend as Chain>::RuntimeOrigin::signed(AssetHubWestendSender::get().into());
+	let roc_to_send: Balance = WESTEND_ED * 10000;
+	let roc_location = RelayLocation::get();
+
+	// Assets to send
+	let assets: Vec<Asset> = vec![(roc_location.clone(), roc_to_send).into()];
+	let fee_id: AssetId = roc_location.into();
+
+	// this should fail
+	AssetHubWestend::execute_with(|| {
+		let result = <AssetHubWestend as AssetHubWestendPallet>::PolkadotXcm::transfer_assets_using_type_and_then(
+			signed_origin.clone(),
+			bx!(destination.clone().into()),
+			bx!(assets.clone().into()),
+			bx!(TransferType::Teleport),
+			bx!(fee_id.into()),
+			bx!(TransferType::Teleport),
+			bx!(VersionedXcm::from(Xcm::<()>::new())),
+			Unlimited,
+		);
+		assert_err!(
+			result,
+			DispatchError::Module(sp_runtime::ModuleError {
+				index: 31,
+				error: [2, 0, 0, 0],
+				message: Some("Filtered")
+			})
+		);
+	});
+
+	// this should also fail
+	AssetHubWestend::execute_with(|| {
+		let xcm: Xcm<asset_hub_westend_runtime::RuntimeCall> = Xcm(vec![
+			WithdrawAsset(assets.into()),
+			InitiateTeleport { assets: Wild(All), dest: destination, xcm: Xcm::<()>::new() },
+		]);
+		let result = <AssetHubWestend as AssetHubWestendPallet>::PolkadotXcm::execute(
+			signed_origin,
+			bx!(xcm::VersionedXcm::V4(xcm)),
+			Weight::MAX,
+		);
+		assert!(result.is_err());
+	});
+}

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

@@ -1939,6 +1939,10 @@ impl<T: Config> Pallet<T> {
 	) -> Result<(Xcm<<T as Config>::RuntimeCall>, Xcm<()>), Error<T>> {
 		let value = (origin, vec![fees.clone()]);
 		ensure!(T::XcmReserveTransferFilter::contains(&value), Error::<T>::Filtered);
+		ensure!(
+			<T::XcmExecutor as XcmAssetTransfers>::IsReserve::contains(&fees, &dest),
+			Error::<T>::InvalidAssetUnsupportedReserve
+		);
 
 		let context = T::UniversalLocation::get();
 		let reanchored_fees = fees
@@ -1973,6 +1977,12 @@ impl<T: Config> Pallet<T> {
 		let value = (origin, assets);
 		ensure!(T::XcmReserveTransferFilter::contains(&value), Error::<T>::Filtered);
 		let (_, assets) = value;
+		for asset in assets.iter() {
+			ensure!(
+				<T::XcmExecutor as XcmAssetTransfers>::IsReserve::contains(&asset, &dest),
+				Error::<T>::InvalidAssetUnsupportedReserve
+			);
+		}
 
 		// max assets is `assets` (+ potentially separately handled fee)
 		let max_assets =
@@ -2079,6 +2089,10 @@ impl<T: Config> Pallet<T> {
 	) -> Result<(Xcm<<T as Config>::RuntimeCall>, Xcm<()>), Error<T>> {
 		let value = (origin, vec![fees.clone()]);
 		ensure!(T::XcmTeleportFilter::contains(&value), Error::<T>::Filtered);
+		ensure!(
+			<T::XcmExecutor as XcmAssetTransfers>::IsTeleporter::contains(&fees, &dest),
+			Error::<T>::Filtered
+		);
 
 		let context = T::UniversalLocation::get();
 		let reanchored_fees = fees
@@ -2134,6 +2148,12 @@ impl<T: Config> Pallet<T> {
 		let value = (origin, assets);
 		ensure!(T::XcmTeleportFilter::contains(&value), Error::<T>::Filtered);
 		let (_, assets) = value;
+		for asset in assets.iter() {
+			ensure!(
+				<T::XcmExecutor as XcmAssetTransfers>::IsTeleporter::contains(&asset, &dest),
+				Error::<T>::Filtered
+			);
+		}
 
 		// max assets is `assets` (+ potentially separately handled fee)
 		let max_assets =

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

@@ -1002,13 +1002,18 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			InitiateReserveWithdraw { assets, reserve, xcm } => {
 				let old_holding = self.holding.clone();
 				let result = Config::TransactionalProcessor::process(|| {
+					let assets = self.holding.saturating_take(assets);
+					// Must ensure that we recognise the assets as being managed by the destination.
+					#[cfg(not(feature = "runtime-benchmarks"))]
+					for asset in assets.assets_iter() {
+						ensure!(
+							Config::IsReserve::contains(&asset, &reserve),
+							XcmError::UntrustedReserveLocation
+						);
+					}
 					// Note that here we are able to place any assets which could not be reanchored
 					// back into Holding.
-					let assets = Self::reanchored(
-						self.holding.saturating_take(assets),
-						&reserve,
-						Some(&mut self.holding),
-					);
+					let assets = Self::reanchored(assets, &reserve, Some(&mut self.holding));
 					let mut message = vec![WithdrawAsset(assets), ClearOrigin];
 					message.extend(xcm.0.into_iter());
 					self.send(reserve, Xcm(message), FeeReason::InitiateReserveWithdraw)?;
@@ -1024,6 +1029,14 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				let result = (|| -> Result<(), XcmError> {
 					// We must do this first in order to resolve wildcards.
 					let assets = self.holding.saturating_take(assets);
+					// Must ensure that we have teleport trust with destination for these assets.
+					#[cfg(not(feature = "runtime-benchmarks"))]
+					for asset in assets.assets_iter() {
+						ensure!(
+							Config::IsTeleporter::contains(&asset, &dest),
+							XcmError::UntrustedTeleportLocation
+						);
+					}
 					for asset in assets.assets_iter() {
 						// We should check that the asset can actually be teleported out (for this
 						// to be in error, there would need to be an accounting violation by

prdoc/pr_5660.prdoc

@@ -0,0 +1,24 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "xcm-executor: validate destinations for ReserveWithdraw and Teleport transfers"
+
+doc:
+  - audience:
+      - Runtime User
+      - Runtime Dev
+    description: |
+      This change adds the required validation for stronger UX guarantees when using
+      `InitiateReserveWithdraw` or `InitiateTeleport` XCM instructions. Execution of
+      the instructions will fail if the local chain is not configured to trust the
+      "destination" or "reserve" chain as a reserve/trusted-teleporter for the provided
+      "assets".
+      With this change, misuse of `InitiateReserveWithdraw`/`InitiateTeleport` fails on
+      origin with no overall side-effects, rather than failing on destination (with
+      side-effects to origin's assets issuance).
+      The commit also makes the same validations for pallet-xcm transfers, and adds
+      regression tests.
+
+crates:
+  - name: staging-xcm-executor
+    bump: patch