From c6ea9a939361319448d9ab5a5a72eabd63097c3b Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Thu, 17 Aug 2023 12:36:52 -0300 Subject: [PATCH] Enforce max assets in multiassets for some instructions --- xcm/src/v3/mod.rs | 4 +- xcm/src/v3/multiasset.rs | 2 +- xcm/src/v3/traits.rs | 3 ++ xcm/xcm-executor/src/lib.rs | 12 ++++++ xcm/xcm-simulator/example/src/lib.rs | 55 +++++++++++++++++++++++++++- 5 files changed, 72 insertions(+), 4 deletions(-) diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index 360867957862..fe789292eb6d 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -42,7 +42,7 @@ pub use junction::{BodyId, BodyPart, Junction, NetworkId}; pub use junctions::Junctions; pub use multiasset::{ AssetId, AssetInstance, Fungibility, MultiAsset, MultiAssetFilter, MultiAssets, - WildFungibility, WildMultiAsset, + WildFungibility, WildMultiAsset, MAX_ITEMS_IN_MULTIASSETS, }; pub use multilocation::{ Ancestor, AncestorThen, InteriorMultiLocation, MultiLocation, Parent, ParentThen, @@ -188,7 +188,7 @@ pub mod prelude { WeightLimit::{self, *}, WildFungibility::{self, Fungible as WildFungible, NonFungible as WildNonFungible}, WildMultiAsset::{self, *}, - XcmContext, XcmHash, XcmWeightInfo, VERSION as XCM_VERSION, + XcmContext, XcmHash, XcmWeightInfo, MAX_ITEMS_IN_MULTIASSETS, VERSION as XCM_VERSION, }; } pub use super::{Instruction, Xcm}; diff --git a/xcm/src/v3/multiasset.rs b/xcm/src/v3/multiasset.rs index 1668d1b870dc..8c075fc55055 100644 --- a/xcm/src/v3/multiasset.rs +++ b/xcm/src/v3/multiasset.rs @@ -508,7 +508,7 @@ pub struct MultiAssets(Vec); /// Maximum number of items we expect in a single `MultiAssets` value. Note this is not (yet) /// enforced, and just serves to provide a sensible `max_encoded_len` for `MultiAssets`. -const MAX_ITEMS_IN_MULTIASSETS: usize = 20; +pub const MAX_ITEMS_IN_MULTIASSETS: usize = 20; impl MaxEncodedLen for MultiAssets { fn max_encoded_len() -> usize { diff --git a/xcm/src/v3/traits.rs b/xcm/src/v3/traits.rs index 128be42c2a2b..f1e84035bd67 100644 --- a/xcm/src/v3/traits.rs +++ b/xcm/src/v3/traits.rs @@ -157,6 +157,9 @@ pub enum Error { WeightNotComputable, /// Recursion stack limit reached ExceedsStackLimit, + /// Assets exceeded maximum + // TODO (XCMv4): Add to spec + TooManyAssets, } impl MaxEncodedLen for Error { diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index a48cd3259d67..3d1856b3fcaa 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -479,6 +479,9 @@ impl XcmExecutor { WithdrawAsset(assets) => { // Take `assets` from the origin account (on-chain) and place in holding. let origin = *self.origin_ref().ok_or(XcmError::BadOrigin)?; + if assets.len() > MAX_ITEMS_IN_MULTIASSETS { + return Err(XcmError::TooManyAssets) + } for asset in assets.into_inner().into_iter() { Config::AssetTransactor::withdraw_asset(&asset, &origin, Some(&self.context))?; self.subsume_asset(asset)?; @@ -488,6 +491,9 @@ impl XcmExecutor { ReserveAssetDeposited(assets) => { // check whether we trust origin to be our reserve location for this asset. let origin = *self.origin_ref().ok_or(XcmError::BadOrigin)?; + if assets.len() > MAX_ITEMS_IN_MULTIASSETS { + return Err(XcmError::TooManyAssets) + } for asset in assets.into_inner().into_iter() { // Must ensure that we recognise the asset as being managed by the origin. ensure!( @@ -526,6 +532,9 @@ impl XcmExecutor { }, ReceiveTeleportedAsset(assets) => { let origin = *self.origin_ref().ok_or(XcmError::BadOrigin)?; + if assets.len() > MAX_ITEMS_IN_MULTIASSETS { + return Err(XcmError::TooManyAssets) + } // check whether we trust origin to teleport this asset to us via config trait. for asset in assets.inner() { // We only trust the origin to send us assets that they identify as their @@ -719,6 +728,9 @@ impl XcmExecutor { }, ClaimAsset { assets, ticket } => { let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?; + if assets.len() > MAX_ITEMS_IN_MULTIASSETS { + return Err(XcmError::TooManyAssets) + } let ok = Config::AssetClaims::claim_assets(origin, &ticket, &assets, &self.context); ensure!(ok, XcmError::UnknownClaim); for asset in assets.into_inner().into_iter() { diff --git a/xcm/xcm-simulator/example/src/lib.rs b/xcm/xcm-simulator/example/src/lib.rs index 1f6956485a84..59ad22463b3e 100644 --- a/xcm/xcm-simulator/example/src/lib.rs +++ b/xcm/xcm-simulator/example/src/lib.rs @@ -145,7 +145,7 @@ mod tests { use codec::Encode; use frame_support::{assert_ok, weights::Weight}; use xcm::latest::QueryResponseInfo; - use xcm_simulator::TestExt; + use xcm_simulator::{fake_message_hash, TestExt, XcmExecutor}; // Helper function for forming buy execution message fn buy_execution(fees: impl Into) -> Instruction { @@ -649,4 +649,57 @@ mod tests { ); }); } + + #[test] + fn too_many_assets_throws_error() { + use crate::Outcome::Incomplete; + use xcm_simulator::XcmError::TooManyAssets; + + let assets = MultiAssets::from(vec![ + (Here, 1).into(), + (Parent, 1).into(), + (MultiLocation::new(2, Here), 1).into(), + (MultiLocation::new(3, Here), 1).into(), + (MultiLocation::new(4, Here), 1).into(), + (MultiLocation::new(5, Here), 1).into(), + (MultiLocation::new(6, Here), 1).into(), + (MultiLocation::new(7, Here), 1).into(), + (MultiLocation::new(8, Here), 1).into(), + (MultiLocation::new(9, Here), 1).into(), + (MultiLocation::new(10, Here), 1).into(), + (MultiLocation::new(11, Here), 1).into(), + (MultiLocation::new(12, Here), 1).into(), + (MultiLocation::new(13, Here), 1).into(), + (MultiLocation::new(14, Here), 1).into(), + (MultiLocation::new(15, Here), 1).into(), + (MultiLocation::new(16, Here), 1).into(), + (MultiLocation::new(17, Here), 1).into(), + (MultiLocation::new(18, Here), 1).into(), + (MultiLocation::new(19, Here), 1).into(), + (MultiLocation::new(20, Here), 1).into(), + ]); + let message = Xcm(vec![WithdrawAsset(assets.clone())]); + let hash = fake_message_hash(&message); + let result = + XcmExecutor::::execute_xcm(Here, message, hash, Weight::MAX); + assert!(matches!(result, Incomplete(_, TooManyAssets))); + + let message = Xcm(vec![ReserveAssetDeposited(assets.clone())]); + let hash = fake_message_hash(&message); + let result = + XcmExecutor::::execute_xcm(Here, message, hash, Weight::MAX); + assert!(matches!(result, Incomplete(_, TooManyAssets))); + + let message = Xcm(vec![ReceiveTeleportedAsset(assets.clone())]); + let hash = fake_message_hash(&message); + let result = + XcmExecutor::::execute_xcm(Here, message, hash, Weight::MAX); + assert!(matches!(result, Incomplete(_, TooManyAssets))); + + let message = Xcm(vec![ClaimAsset { assets: assets.clone(), ticket: Here.into() }]); + let hash = fake_message_hash(&message); + let result = + XcmExecutor::::execute_xcm(Here, message, hash, Weight::MAX); + assert!(matches!(result, Incomplete(_, TooManyAssets))); + } }