diff --git a/bridges/modules/xcm-bridge-hub-router/src/lib.rs b/bridges/modules/xcm-bridge-hub-router/src/lib.rs index c6008802ae9a..ece72ac8494b 100644 --- a/bridges/modules/xcm-bridge-hub-router/src/lib.rs +++ b/bridges/modules/xcm-bridge-hub-router/src/lib.rs @@ -328,40 +328,58 @@ impl, I: 'static> SendXcm for Pallet { xcm: &mut Option>, ) -> SendResult { log::trace!(target: LOG_TARGET, "validate - msg: {xcm:?}, destination: {dest:?}"); - // `dest` and `xcm` are required here - let dest_ref = dest.as_ref().ok_or(SendError::MissingArgument)?; - let xcm_ref = xcm.as_ref().ok_or(SendError::MissingArgument)?; - - // we won't have an access to `dest` and `xcm` in the `deliver` method, so precompute - // everything required here - let message_size = xcm_ref.encoded_size() as _; - - // bridge doesn't support oversized/overweight messages now. So it is better to drop such - // messages here than at the bridge hub. Let's check the message size. - if message_size > HARD_MESSAGE_SIZE_LIMIT { - return Err(SendError::ExceedsMaxMessageSize) - } - // We need to ensure that the known `dest`'s XCM version can comprehend the current `xcm` - // program. This may seem like an additional, unnecessary check, but it is not. A similar - // check is probably performed by the `ViaBridgeHubExporter`, which attempts to send a - // versioned message to the sibling bridge hub. However, the local bridge hub may have a - // higher XCM version than the remote `dest`. Once again, it is better to discard such - // messages here than at the bridge hub (e.g., to avoid losing funds). - let destination_version = T::DestinationVersion::get_version_for(dest_ref) - .ok_or(SendError::DestinationUnsupported)?; - let _ = VersionedXcm::from(xcm_ref.clone()) - .into_version(destination_version) - .map_err(|()| SendError::DestinationUnsupported)?; - - // just use exporter to validate destination and insert instructions to pay message fee - // at the sibling/child bridge hub - // - // the cost will include both cost of: (1) to-sibling bridge hub delivery (returned by - // the `Config::ToBridgeHubSender`) and (2) to-bridged bridge hub delivery (returned by - // `Self::exporter_for`) - ViaBridgeHubExporter::::validate(dest, xcm) - .map(|(ticket, cost)| ((message_size, ticket), cost)) + // In case of success, the `ViaBridgeHubExporter` can modify XCM instructions and consume + // `dest` / `xcm`, so we retain the clone of original message and the destination for later + // `DestinationVersion` validation. + let xcm_to_dest_clone = xcm.clone(); + let dest_clone = dest.clone(); + + // First, use the inner exporter to validate the destination to determine if it is even + // routable. If it is not, return an error. If it is, then the XCM is extended with + // instructions to pay the message fee at the sibling/child bridge hub. The cost will + // include both the cost of (1) delivery to the sibling bridge hub (returned by + // `Config::ToBridgeHubSender`) and (2) delivery to the bridged bridge hub (returned by + // `Self::exporter_for`). + match ViaBridgeHubExporter::::validate(dest, xcm) { + Ok((ticket, cost)) => { + // If the ticket is ok, it means we are routing with this router, so we need to + // apply more validations to the cloned `dest` and `xcm`, which are required here. + let xcm_to_dest_clone = xcm_to_dest_clone.ok_or(SendError::MissingArgument)?; + let dest_clone = dest_clone.ok_or(SendError::MissingArgument)?; + + // We won't have access to `dest` and `xcm` in the `deliver` method, so we need to + // precompute everything required here. However, `dest` and `xcm` were consumed by + // `ViaBridgeHubExporter`, so we need to use their clones. + let message_size = xcm_to_dest_clone.encoded_size() as _; + + // The bridge doesn't support oversized or overweight messages. Therefore, it's + // better to drop such messages here rather than at the bridge hub. Let's check the + // message size." + if message_size > HARD_MESSAGE_SIZE_LIMIT { + return Err(SendError::ExceedsMaxMessageSize) + } + + // We need to ensure that the known `dest`'s XCM version can comprehend the current + // `xcm` program. This may seem like an additional, unnecessary check, but it is + // not. A similar check is probably performed by the `ViaBridgeHubExporter`, which + // attempts to send a versioned message to the sibling bridge hub. However, the + // local bridge hub may have a higher XCM version than the remote `dest`. Once + // again, it is better to discard such messages here than at the bridge hub (e.g., + // to avoid losing funds). + let destination_version = T::DestinationVersion::get_version_for(&dest_clone) + .ok_or(SendError::DestinationUnsupported)?; + let _ = VersionedXcm::from(xcm_to_dest_clone) + .into_version(destination_version) + .map_err(|()| SendError::DestinationUnsupported)?; + + Ok(((message_size, ticket), cost)) + }, + Err(e) => { + log::trace!(target: LOG_TARGET, "validate - ViaBridgeHubExporter - error: {e:?}"); + Err(e) + }, + } } fn deliver(ticket: Self::Ticket) -> Result { @@ -452,24 +470,51 @@ mod tests { #[test] fn not_applicable_if_destination_is_within_other_network() { run_test(|| { + // unroutable dest + let dest = Location::new(2, [GlobalConsensus(ByGenesis([0; 32])), Parachain(1000)]); + let xcm: Xcm<()> = vec![ClearOrigin].into(); + + // check that router does not consume when `NotApplicable` + let mut xcm_wrapper = Some(xcm.clone()); assert_eq!( - send_xcm::( - Location::new(2, [GlobalConsensus(Rococo), Parachain(1000)]), - vec![].into(), - ), + XcmBridgeHubRouter::validate(&mut Some(dest.clone()), &mut xcm_wrapper), Err(SendError::NotApplicable), ); + // XCM is NOT consumed and untouched + assert_eq!(Some(xcm.clone()), xcm_wrapper); + + // check the full `send_xcm` + assert_eq!(send_xcm::(dest, xcm,), Err(SendError::NotApplicable),); }); } #[test] fn exceeds_max_message_size_if_size_is_above_hard_limit() { run_test(|| { + // routable dest with XCM version + let dest = + Location::new(2, [GlobalConsensus(BridgedNetworkId::get()), Parachain(1000)]); + // oversized XCM + let xcm: Xcm<()> = vec![ClearOrigin; HARD_MESSAGE_SIZE_LIMIT as usize].into(); + + // dest is routable with the inner router + assert_ok!(ViaBridgeHubExporter::::validate( + &mut Some(dest.clone()), + &mut Some(xcm.clone()) + )); + + // check for oversized message + let mut xcm_wrapper = Some(xcm.clone()); + assert_eq!( + XcmBridgeHubRouter::validate(&mut Some(dest.clone()), &mut xcm_wrapper), + Err(SendError::ExceedsMaxMessageSize), + ); + // XCM is consumed by the inner router + assert!(xcm_wrapper.is_none()); + + // check the full `send_xcm` assert_eq!( - send_xcm::( - Location::new(2, [GlobalConsensus(Rococo), Parachain(1000)]), - vec![ClearOrigin; HARD_MESSAGE_SIZE_LIMIT as usize].into(), - ), + send_xcm::(dest, xcm,), Err(SendError::ExceedsMaxMessageSize), ); }); @@ -478,11 +523,28 @@ mod tests { #[test] fn destination_unsupported_if_wrap_version_fails() { run_test(|| { + // routable dest but we don't know XCM version + let dest = UnknownXcmVersionForRoutableLocation::get(); + let xcm: Xcm<()> = vec![ClearOrigin].into(); + + // dest is routable with the inner router + assert_ok!(ViaBridgeHubExporter::::validate( + &mut Some(dest.clone()), + &mut Some(xcm.clone()) + )); + + // check that it does not pass XCM version check + let mut xcm_wrapper = Some(xcm.clone()); + assert_eq!( + XcmBridgeHubRouter::validate(&mut Some(dest.clone()), &mut xcm_wrapper), + Err(SendError::DestinationUnsupported), + ); + // XCM is consumed by the inner router + assert!(xcm_wrapper.is_none()); + + // check the full `send_xcm` assert_eq!( - send_xcm::( - UnknownXcmVersionLocation::get(), - vec![ClearOrigin].into(), - ), + send_xcm::(dest, xcm,), Err(SendError::DestinationUnsupported), ); }); diff --git a/bridges/modules/xcm-bridge-hub-router/src/mock.rs b/bridges/modules/xcm-bridge-hub-router/src/mock.rs index 54e10966d51b..20c86d1da9a2 100644 --- a/bridges/modules/xcm-bridge-hub-router/src/mock.rs +++ b/bridges/modules/xcm-bridge-hub-router/src/mock.rs @@ -61,7 +61,7 @@ parameter_types! { Some((BridgeFeeAsset::get(), BASE_FEE).into()) ) ]; - pub UnknownXcmVersionLocation: Location = Location::new(2, [GlobalConsensus(BridgedNetworkId::get()), Parachain(9999)]); + pub UnknownXcmVersionForRoutableLocation: Location = Location::new(2, [GlobalConsensus(BridgedNetworkId::get()), Parachain(9999)]); } #[derive_impl(frame_system::config_preludes::TestDefaultConfig)] @@ -76,7 +76,7 @@ impl pallet_xcm_bridge_hub_router::Config<()> for TestRuntime { type BridgedNetworkId = BridgedNetworkId; type Bridges = NetworkExportTable; type DestinationVersion = - LatestOrNoneForLocationVersionChecker>; + LatestOrNoneForLocationVersionChecker>; type BridgeHubOrigin = EnsureRoot; type ToBridgeHubSender = TestToBridgeHubSender;