From 583fdd7dff49cee5d8c3a796918b8bd4d0aa71d9 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 10 Sep 2024 13:03:49 +0300 Subject: [PATCH] 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 --- .../tests/assets/asset-hub-rococo/src/lib.rs | 1 + .../src/tests/hybrid_transfers.rs | 11 +++- .../src/tests/reserve_transfer.rs | 55 +++++++++++++++++++ .../asset-hub-rococo/src/tests/teleport.rs | 51 +++++++++++++++++ .../tests/assets/asset-hub-westend/src/lib.rs | 1 + .../src/tests/hybrid_transfers.rs | 11 +++- .../src/tests/reserve_transfer.rs | 55 +++++++++++++++++++ .../asset-hub-westend/src/tests/teleport.rs | 51 +++++++++++++++++ polkadot/xcm/pallet-xcm/src/lib.rs | 20 +++++++ polkadot/xcm/xcm-executor/src/lib.rs | 23 ++++++-- prdoc/pr_5660.prdoc | 24 ++++++++ 11 files changed, 296 insertions(+), 7 deletions(-) create mode 100644 prdoc/pr_5660.prdoc diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs index f4fe1478f3ed..0e43108a417b 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs +++ b/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, diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/hybrid_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/hybrid_transfers.rs index 7ff6d6c193c9..7bb25d7cec62 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/hybrid_transfers.rs +++ b/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!(::System::set_storage( + ::RuntimeOrigin::root(), + vec![( + PenpalCustomizableAssetFromSystemAssetHub::key().to_vec(), + Location::new(2, [GlobalConsensus(Westend)]).encode(), + )], + )); + }); PenpalB::execute_with(|| { assert_ok!(::System::set_storage( ::RuntimeOrigin::root(), diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs index faff5f7660c2..8aad4b392b2c 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs +++ b/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 = + ::RuntimeOrigin::signed(AssetHubRococoSender::get().into()); + let roc_to_send: Balance = ROCOCO_ED * 10000; + let roc_location = RelayLocation::get(); + + // Assets to send + let assets: Vec = vec![(roc_location.clone(), roc_to_send).into()]; + let fee_id: AssetId = roc_location.into(); + + // this should fail + AssetHubRococo::execute_with(|| { + let result = ::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 = Xcm(vec![ + WithdrawAsset(assets.into()), + InitiateReserveWithdraw { + assets: Wild(All), + reserve: destination, + xcm: Xcm::<()>::new(), + }, + ]); + let result = ::PolkadotXcm::execute( + signed_origin, + bx!(xcm::VersionedXcm::V4(xcm)), + Weight::MAX, + ); + assert!(result.is_err()); + }); +} diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/teleport.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/teleport.rs index c8da801a14bf..d6cf819118e9 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/teleport.rs +++ b/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 = + ::RuntimeOrigin::signed(AssetHubRococoSender::get().into()); + let roc_to_send: Balance = ROCOCO_ED * 10000; + let roc_location = RelayLocation::get(); + + // Assets to send + let assets: Vec = vec![(roc_location.clone(), roc_to_send).into()]; + let fee_id: AssetId = roc_location.into(); + + // this should fail + AssetHubRococo::execute_with(|| { + let result = ::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 = Xcm(vec![ + WithdrawAsset(assets.into()), + InitiateTeleport { assets: Wild(All), dest: destination, xcm: Xcm::<()>::new() }, + ]); + let result = ::PolkadotXcm::execute( + signed_origin, + bx!(xcm::VersionedXcm::V4(xcm)), + Weight::MAX, + ); + assert!(result.is_err()); + }); +} diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs index f568fb4101db..d0016b5a1b15 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs +++ b/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, diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs index 975bacea7b4f..4d6cdd9a94d6 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs +++ b/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!(::System::set_storage( + ::RuntimeOrigin::root(), + vec![( + PenpalCustomizableAssetFromSystemAssetHub::key().to_vec(), + Location::new(2, [GlobalConsensus(Rococo)]).encode(), + )], + )); + }); PenpalB::execute_with(|| { assert_ok!(::System::set_storage( ::RuntimeOrigin::root(), diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs index 53b6939298da..0100e8e34ef3 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs +++ b/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 = + ::RuntimeOrigin::signed(AssetHubWestendSender::get().into()); + let roc_to_send: Balance = WESTEND_ED * 10000; + let roc_location = RelayLocation::get(); + + // Assets to send + let assets: Vec = vec![(roc_location.clone(), roc_to_send).into()]; + let fee_id: AssetId = roc_location.into(); + + // this should fail + AssetHubWestend::execute_with(|| { + let result = ::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 = Xcm(vec![ + WithdrawAsset(assets.into()), + InitiateReserveWithdraw { + assets: Wild(All), + reserve: destination, + xcm: Xcm::<()>::new(), + }, + ]); + let result = ::PolkadotXcm::execute( + signed_origin, + bx!(xcm::VersionedXcm::V4(xcm)), + Weight::MAX, + ); + assert!(result.is_err()); + }); +} diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/teleport.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/teleport.rs index 15d39858acca..ddb82a954a87 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/teleport.rs +++ b/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 = + ::RuntimeOrigin::signed(AssetHubWestendSender::get().into()); + let roc_to_send: Balance = WESTEND_ED * 10000; + let roc_location = RelayLocation::get(); + + // Assets to send + let assets: Vec = vec![(roc_location.clone(), roc_to_send).into()]; + let fee_id: AssetId = roc_location.into(); + + // this should fail + AssetHubWestend::execute_with(|| { + let result = ::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 = Xcm(vec![ + WithdrawAsset(assets.into()), + InitiateTeleport { assets: Wild(All), dest: destination, xcm: Xcm::<()>::new() }, + ]); + let result = ::PolkadotXcm::execute( + signed_origin, + bx!(xcm::VersionedXcm::V4(xcm)), + Weight::MAX, + ); + assert!(result.is_err()); + }); +} diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 05d9046ab192..ee448c3df606 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -1939,6 +1939,10 @@ impl Pallet { ) -> Result<(Xcm<::RuntimeCall>, Xcm<()>), Error> { let value = (origin, vec![fees.clone()]); ensure!(T::XcmReserveTransferFilter::contains(&value), Error::::Filtered); + ensure!( + ::IsReserve::contains(&fees, &dest), + Error::::InvalidAssetUnsupportedReserve + ); let context = T::UniversalLocation::get(); let reanchored_fees = fees @@ -1973,6 +1977,12 @@ impl Pallet { let value = (origin, assets); ensure!(T::XcmReserveTransferFilter::contains(&value), Error::::Filtered); let (_, assets) = value; + for asset in assets.iter() { + ensure!( + ::IsReserve::contains(&asset, &dest), + Error::::InvalidAssetUnsupportedReserve + ); + } // max assets is `assets` (+ potentially separately handled fee) let max_assets = @@ -2079,6 +2089,10 @@ impl Pallet { ) -> Result<(Xcm<::RuntimeCall>, Xcm<()>), Error> { let value = (origin, vec![fees.clone()]); ensure!(T::XcmTeleportFilter::contains(&value), Error::::Filtered); + ensure!( + ::IsTeleporter::contains(&fees, &dest), + Error::::Filtered + ); let context = T::UniversalLocation::get(); let reanchored_fees = fees @@ -2134,6 +2148,12 @@ impl Pallet { let value = (origin, assets); ensure!(T::XcmTeleportFilter::contains(&value), Error::::Filtered); let (_, assets) = value; + for asset in assets.iter() { + ensure!( + ::IsTeleporter::contains(&asset, &dest), + Error::::Filtered + ); + } // max assets is `assets` (+ potentially separately handled fee) let max_assets = diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index a8110ca3d19f..14920f36445b 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -1002,13 +1002,18 @@ impl XcmExecutor { 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 XcmExecutor { 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 diff --git a/prdoc/pr_5660.prdoc b/prdoc/pr_5660.prdoc new file mode 100644 index 000000000000..172b515df02f --- /dev/null +++ b/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