Add validate_xcm_nesting to the ParentAsUmp and ChildParachainRouter

cumulus/pallets/xcmp-queue/src/lib.rs

@@ -916,7 +916,8 @@ impl<T: Config> SendXcm for Pallet<T> {
 				let price = T::PriceForSiblingDelivery::price_for_delivery(id, &xcm);
 				let versioned_xcm = T::VersionWrapper::wrap_version(&d, xcm)
 					.map_err(|()| SendError::DestinationUnsupported)?;
-				validate_xcm_nesting(&versioned_xcm)
+				versioned_xcm
+					.validate_xcm_nesting()
 					.map_err(|()| SendError::ExceedsMaxMessageSize)?;
 
 				Ok(((id, versioned_xcm), price))
@@ -932,10 +933,6 @@ impl<T: Config> SendXcm for Pallet<T> {
 
 	fn deliver((id, xcm): (ParaId, VersionedXcm<()>)) -> Result<XcmHash, SendError> {
 		let hash = xcm.using_encoded(sp_io::hashing::blake2_256);
-		defensive_assert!(
-			validate_xcm_nesting(&xcm).is_ok(),
-			"Tickets are valid prior to delivery by trait XCM; qed"
-		);
 
 		match Self::send_fragment(id, XcmpMessageFormat::ConcatenatedVersionedXcm, xcm) {
 			Ok(_) => {
@@ -950,16 +947,6 @@ impl<T: Config> SendXcm for Pallet<T> {
 	}
 }
 
-/// Checks that the XCM is decodable with `MAX_XCM_DECODE_DEPTH`.
-///
-/// Note that this uses the limit of the sender - not the receiver. It it best effort.
-pub(crate) fn validate_xcm_nesting(xcm: &VersionedXcm<()>) -> Result<(), ()> {
-	xcm.using_encoded(|mut enc| {
-		VersionedXcm::<()>::decode_all_with_depth_limit(MAX_XCM_DECODE_DEPTH, &mut enc).map(|_| ())
-	})
-	.map_err(|_| ())
-}
-
 impl<T: Config> FeeTracker for Pallet<T> {
 	type Id = ParaId;
 

cumulus/primitives/utility/src/lib.rs

@@ -69,6 +69,9 @@ where
 			let price = P::price_for_delivery((), &xcm);
 			let versioned_xcm =
 				W::wrap_version(&d, xcm).map_err(|()| SendError::DestinationUnsupported)?;
+			versioned_xcm
+				.validate_xcm_nesting()
+				.map_err(|()| SendError::ExceedsMaxMessageSize)?;
 			let data = versioned_xcm.encode();
 
 			Ok((data, price))
@@ -526,6 +529,8 @@ impl<
 mod test_xcm_router {
 	use super::*;
 	use cumulus_primitives_core::UpwardMessage;
+	use frame_support::assert_ok;
+	use xcm::MAX_XCM_DECODE_DEPTH;
 
 	/// Validates [`validate`] for required Some(destination) and Some(message)
 	struct OkFixedXcmHashWithAssertingRequiredInputsSender;
@@ -621,6 +626,29 @@ mod test_xcm_router {
 			)>(dest.into(), message)
 		);
 	}
+
+	#[test]
+	fn parent_as_ump_validate_nested_xcm_works() {
+		let dest = Parent;
+
+		type Router = ParentAsUmp<(), (), ()>;
+
+		// Message that is not too deeply nested:
+		let mut good = Xcm(vec![ClearOrigin]);
+		for _ in 0..MAX_XCM_DECODE_DEPTH - 1 {
+			good = Xcm(vec![SetAppendix(good)]);
+		}
+
+		// Check that the good message is validated:
+		assert_ok!(<Router as SendXcm>::validate(&mut Some(dest.into()), &mut Some(good.clone())));
+
+		// Nesting the message one more time should reject it:
+		let bad = Xcm(vec![SetAppendix(good)]);
+		assert_eq!(
+			Err(SendError::ExceedsMaxMessageSize),
+			<Router as SendXcm>::validate(&mut Some(dest.into()), &mut Some(bad))
+		);
+	}
 }
 #[cfg(test)]
 mod test_trader {

polkadot/runtime/common/src/integration_tests.rs

@@ -39,7 +39,7 @@ use primitives::{
 	MAX_CODE_SIZE,
 };
 use runtime_parachains::{
-	configuration, origin, paras, shared, Origin as ParaOrigin, ParaLifecycle,
+	configuration, dmp, origin, paras, shared, Origin as ParaOrigin, ParaLifecycle,
 };
 use sp_core::H256;
 use sp_io::TestExternalities;
@@ -84,6 +84,7 @@ frame_support::construct_runtime!(
 		Paras: paras,
 		ParasShared: shared,
 		ParachainsOrigin: origin,
+		Dmp: dmp,
 
 		// Para Onboarding Pallets
 		Registrar: paras_registrar,
@@ -201,6 +202,8 @@ impl shared::Config for Test {
 	type DisabledValidators = ();
 }
 
+impl dmp::Config for Test {}
+
 impl origin::Config for Test {}
 
 parameter_types! {

polkadot/runtime/common/src/xcm_sender.rs

@@ -119,7 +119,9 @@ where
 		let config = configuration::ActiveConfig::<T>::get();
 		let para = id.into();
 		let price = P::price_for_delivery(para, &xcm);
-		let blob = W::wrap_version(&d, xcm).map_err(|()| DestinationUnsupported)?.encode();
+		let versioned_xcm = W::wrap_version(&d, xcm).map_err(|()| DestinationUnsupported)?;
+		versioned_xcm.validate_xcm_nesting().map_err(|()| ExceedsMaxMessageSize)?;
+		let blob = versioned_xcm.encode();
 		dmp::Pallet::<T>::can_queue_downward_message(&config, &para, &blob)
 			.map_err(Into::<SendError>::into)?;
 
@@ -236,9 +238,11 @@ impl EnsureForParachain for () {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use frame_support::parameter_types;
+	use crate::integration_tests::new_test_ext;
+	use frame_support::{assert_ok, parameter_types};
 	use runtime_parachains::FeeTracker;
 	use sp_runtime::FixedU128;
+	use xcm::MAX_XCM_DECODE_DEPTH;
 
 	parameter_types! {
 		pub const BaseDeliveryFee: u128 = 300_000_000;
@@ -297,4 +301,40 @@ mod tests {
 			(FeeAssetId::get(), result).into()
 		);
 	}
+
+	#[test]
+	fn child_parachain_router_validate_nested_xcm_works() {
+		let dest = Parachain(5555);
+
+		type Router = ChildParachainRouter<
+			crate::integration_tests::Test,
+			(),
+			NoPriceForMessageDelivery<ParaId>,
+		>;
+
+		// Message that is not too deeply nested:
+		let mut good = Xcm(vec![ClearOrigin]);
+		for _ in 0..MAX_XCM_DECODE_DEPTH - 1 {
+			good = Xcm(vec![SetAppendix(good)]);
+		}
+
+		new_test_ext().execute_with(|| {
+			configuration::ActiveConfig::<crate::integration_tests::Test>::mutate(|c| {
+				c.max_downward_message_size = u32::MAX;
+			});
+
+			// Check that the good message is validated:
+			assert_ok!(<Router as SendXcm>::validate(
+				&mut Some(dest.into()),
+				&mut Some(good.clone())
+			));
+
+			// Nesting the message one more time should reject it:
+			let bad = Xcm(vec![SetAppendix(good)]);
+			assert_eq!(
+				Err(ExceedsMaxMessageSize),
+				<Router as SendXcm>::validate(&mut Some(dest.into()), &mut Some(bad))
+			);
+		});
+	}
 }

polkadot/xcm/src/lib.rs

@@ -25,7 +25,7 @@
 extern crate alloc;
 
 use derivative::Derivative;
-use parity_scale_codec::{Decode, Encode, Error as CodecError, Input, MaxEncodedLen};
+use parity_scale_codec::{Decode, DecodeLimit, Encode, Error as CodecError, Input, MaxEncodedLen};
 use scale_info::TypeInfo;
 
 pub mod v2;
@@ -456,6 +456,23 @@ impl<C> IdentifyVersion for VersionedXcm<C> {
 	}
 }
 
+impl<C> VersionedXcm<C> {
+	/// Checks that the XCM is decodable with `MAX_XCM_DECODE_DEPTH`. Consequently, it also checks
+	/// all decode implementations and limits, such as MAX_ITEMS_IN_ASSETS or
+	/// MAX_INSTRUCTIONS_TO_DECODE.
+	///
+	/// Note that this uses the limit of the sender - not the receiver. It is a best effort.
+	pub fn validate_xcm_nesting(&self) -> Result<(), ()> {
+		self.using_encoded(|mut enc| {
+			Self::decode_all_with_depth_limit(MAX_XCM_DECODE_DEPTH, &mut enc).map(|_| ())
+		})
+		.map_err(|e| {
+			log::error!(target: "xcm::validate_xcm_nesting", "Decode error: {e:?} for xcm: {self:?}!");
+			()
+		})
+	}
+}
+
 impl<RuntimeCall> From<v2::Xcm<RuntimeCall>> for VersionedXcm<RuntimeCall> {
 	fn from(x: v2::Xcm<RuntimeCall>) -> Self {
 		VersionedXcm::V2(x)
@@ -704,3 +721,77 @@ fn size_limits() {
 	}
 	assert!(!test_failed);
 }
+
+#[test]
+fn validate_xcm_nesting_works() {
+	use crate::latest::{
+		prelude::{GeneralIndex, ReserveAssetDeposited, SetAppendix},
+		Assets, Xcm, MAX_INSTRUCTIONS_TO_DECODE, MAX_ITEMS_IN_ASSETS,
+	};
+
+	// closure generates assets of `count`
+	let assets = |count| {
+		let mut assets = Assets::new();
+		for i in 0..count {
+			assets.push((GeneralIndex(i as u128), 100).into());
+		}
+		assets
+	};
+
+	// closer generates `Xcm` with nested instructions of `depth`
+	let with_instr = |depth| {
+		let mut xcm = Xcm::<()>(vec![]);
+		for _ in 0..depth - 1 {
+			xcm = Xcm::<()>(vec![SetAppendix(xcm)]);
+		}
+		xcm
+	};
+
+	// `MAX_INSTRUCTIONS_TO_DECODE` check
+	assert!(VersionedXcm::<()>::from(Xcm(vec![
+		ReserveAssetDeposited(assets(1));
+		(MAX_INSTRUCTIONS_TO_DECODE - 1) as usize
+	]))
+	.validate_xcm_nesting()
+	.is_ok());
+	assert!(VersionedXcm::<()>::from(Xcm(vec![
+		ReserveAssetDeposited(assets(1));
+		MAX_INSTRUCTIONS_TO_DECODE as usize
+	]))
+	.validate_xcm_nesting()
+	.is_ok());
+	assert!(VersionedXcm::<()>::from(Xcm(vec![
+		ReserveAssetDeposited(assets(1));
+		(MAX_INSTRUCTIONS_TO_DECODE + 1) as usize
+	]))
+	.validate_xcm_nesting()
+	.is_err());
+
+	// `MAX_XCM_DECODE_DEPTH` check
+	assert!(VersionedXcm::<()>::from(with_instr(MAX_XCM_DECODE_DEPTH - 1))
+		.validate_xcm_nesting()
+		.is_ok());
+	assert!(VersionedXcm::<()>::from(with_instr(MAX_XCM_DECODE_DEPTH))
+		.validate_xcm_nesting()
+		.is_ok());
+	assert!(VersionedXcm::<()>::from(with_instr(MAX_XCM_DECODE_DEPTH + 1))
+		.validate_xcm_nesting()
+		.is_err());
+
+	// `MAX_ITEMS_IN_ASSETS` check
+	assert!(VersionedXcm::<()>::from(Xcm(vec![ReserveAssetDeposited(assets(
+		MAX_ITEMS_IN_ASSETS
+	))]))
+	.validate_xcm_nesting()
+	.is_ok());
+	assert!(VersionedXcm::<()>::from(Xcm(vec![ReserveAssetDeposited(assets(
+		MAX_ITEMS_IN_ASSETS - 1
+	))]))
+	.validate_xcm_nesting()
+	.is_ok());
+	assert!(VersionedXcm::<()>::from(Xcm(vec![ReserveAssetDeposited(assets(
+		MAX_ITEMS_IN_ASSETS + 1
+	))]))
+	.validate_xcm_nesting()
+	.is_err());
+}