diff --git a/Cargo.lock b/Cargo.lock index 64c68d81d42b..d04808c0f089 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2595,6 +2595,8 @@ dependencies = [ "sp-runtime 31.0.1", "sp-std 14.0.0", "staging-xcm 7.0.0", + "staging-xcm-builder 7.0.0", + "staging-xcm-executor 7.0.0", ] [[package]] diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml b/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml index 2fbb96d75163..9c5c7c513909 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml +++ b/cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml @@ -19,6 +19,8 @@ sp-core = { workspace = true } sp-runtime = { workspace = true } sp-std = { workspace = true } xcm = { workspace = true } +xcm-builder = { workspace = true } +xcm-executor = { workspace = true } [features] default = ["std"] @@ -32,6 +34,8 @@ std = [ "sp-core/std", "sp-runtime/std", "sp-std/std", + "xcm-builder/std", + "xcm-executor/std", "xcm/std", ] @@ -41,5 +45,7 @@ runtime-benchmarks = [ "pallet-message-queue/runtime-benchmarks", "snowbridge-core/runtime-benchmarks", "sp-runtime/runtime-benchmarks", + "xcm-builder/runtime-benchmarks", + "xcm-executor/runtime-benchmarks", "xcm/runtime-benchmarks", ] diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs new file mode 100644 index 000000000000..6b5dee3de384 --- /dev/null +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs @@ -0,0 +1,57 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use core::{marker::PhantomData, ops::ControlFlow}; +use cumulus_primitives_core::Weight; +use frame_support::traits::{Contains, ProcessMessageError}; +use xcm::prelude::{ExportMessage, Instruction, Location, NetworkId}; + +use xcm_builder::{CreateMatcher, MatchXcm}; +use xcm_executor::traits::{DenyExecution, Properties}; + +/// Deny execution if the message contains instruction `ExportMessage` with +/// a. origin is contained in `FromOrigin` (i.e.`FromOrigin::Contains(origin)`) +/// b. network is contained in `ToGlobalConsensus`, (i.e. `ToGlobalConsensus::contains(network)`) +pub struct DenyExportMessageFrom( + PhantomData<(FromOrigin, ToGlobalConsensus)>, +); + +impl DenyExecution + for DenyExportMessageFrom +where + FromOrigin: Contains, + ToGlobalConsensus: Contains, +{ + fn deny_execution( + origin: &Location, + message: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + // This barrier only cares about messages with `origin` matching `FromOrigin`. + if !FromOrigin::contains(origin) { + return Ok(()) + } + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ExportMessage { network, .. } if ToGlobalConsensus::contains(network) => + Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } +} diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs b/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs index b806b8cdb22d..dbd87bea2142 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs @@ -14,9 +14,11 @@ // limitations under the License. #![cfg_attr(not(feature = "std"), no_std)] +pub mod barriers; pub mod digest_item; pub mod message_queue; pub mod xcm_version; +pub use barriers::DenyExportMessageFrom; pub use digest_item::CustomDigestItem; pub use message_queue::{AggregateMessageOrigin, BridgeHubMessageRouter}; diff --git a/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs new file mode 100644 index 000000000000..84c135728d5d --- /dev/null +++ b/cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs @@ -0,0 +1,85 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// This file is part of Cumulus. + +// Cumulus is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Cumulus is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Cumulus. If not, see . + +#![cfg(test)] +use bridge_hub_common::DenyExportMessageFrom; +use frame_support::{ + parameter_types, + traits::{Equals, EverythingBut, ProcessMessageError::Unsupported}, +}; +use xcm::prelude::{ + AliasOrigin, ByGenesis, ExportMessage, Here, Instruction, Location, NetworkId, Parachain, + Weight, +}; +use xcm_executor::traits::{DenyExecution, Properties}; + +#[test] +fn test_deny_export_message_from() { + parameter_types! { + pub Source1: Location = Location::new(1, Parachain(1)); + pub Source2: Location = Location::new(1, Parachain(2)); + pub Remote1: NetworkId = ByGenesis([1;32]); + pub Remote2: NetworkId = ByGenesis([2;32]); + } + + // Deny ExportMessage when both of the conditions met: + // 1: source != Source1 + // 2: network == Remote1 + pub type Denied = DenyExportMessageFrom>, Equals>; + + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + Denied::deny_execution( + &origin, + &mut xcm, + Weight::zero(), + &mut Properties { weight_credit: Weight::zero(), message_id: None } + ), + expected_result + ); + }; + + // A message without an `ExportMessage` should pass + assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), Ok(())); + + // `ExportMessage` from source1 and remote1 should pass + assert_deny_execution( + vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], + Source1::get(), + Ok(()), + ); + + // `ExportMessage` from source1 and remote2 should pass + assert_deny_execution( + vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], + Source1::get(), + Ok(()), + ); + + // `ExportMessage` from source2 and remote2 should pass + assert_deny_execution( + vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }], + Source2::get(), + Ok(()), + ); + + // `ExportMessage` from source2 and remote1 should be banned + assert_deny_execution( + vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }], + Source2::get(), + Err(Unsupported), + ); +} diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 27153a3f441d..9f9d3c232122 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -24,7 +24,7 @@ use frame_support::{ }; use polkadot_parachain_primitives::primitives::IsSystem; use xcm::prelude::*; -use xcm_executor::traits::{CheckSuspension, OnResponse, Properties, ShouldExecute}; +use xcm_executor::traits::{CheckSuspension, DenyExecution, OnResponse, Properties, ShouldExecute}; /// Execution barrier that just takes `max_weight` from `properties.weight_credit`. /// @@ -444,12 +444,12 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain { /// If it passes the Deny, and matches one of the Allow cases then it is let through. pub struct DenyThenTry(PhantomData, PhantomData) where - Deny: ShouldExecute, + Deny: DenyExecution, Allow: ShouldExecute; impl ShouldExecute for DenyThenTry where - Deny: ShouldExecute, + Deny: DenyExecution, Allow: ShouldExecute, { fn should_execute( @@ -458,15 +458,15 @@ where max_weight: Weight, properties: &mut Properties, ) -> Result<(), ProcessMessageError> { - Deny::should_execute(origin, message, max_weight, properties)?; + Deny::deny_execution(origin, message, max_weight, properties)?; Allow::should_execute(origin, message, max_weight, properties) } } // See issue pub struct DenyReserveTransferToRelayChain; -impl ShouldExecute for DenyReserveTransferToRelayChain { - fn should_execute( +impl DenyExecution for DenyReserveTransferToRelayChain { + fn deny_execution( origin: &Location, message: &mut [Instruction], _max_weight: Weight, @@ -499,8 +499,6 @@ impl ShouldExecute for DenyReserveTransferToRelayChain { _ => Ok(ControlFlow::Continue(())), }, )?; - - // Permit everything else Ok(()) } } diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index d8805274d3a5..2fb8e8ed0363 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -532,3 +532,235 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { Ok(()), ); } + +#[test] +fn deny_then_try_works() { + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::Yield` when XCM contains + /// `ClearTransactStatus` + struct DenyClearTransactStatusAsYield; + impl DenyExecution for DenyClearTransactStatusAsYield { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTransactStatus { .. } => Err(ProcessMessageError::Yield), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::BadFormat` when XCM + /// contains `ClearOrigin` with origin location from `Here` + struct DenyClearOriginFromHereAsBadFormat; + impl DenyExecution for DenyClearOriginFromHereAsBadFormat { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin { .. } => + if origin.clone() == Here.into_location() { + Err(ProcessMessageError::BadFormat) + } else { + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::StackLimitReached` when XCM + /// contains a single `UnsubscribeVersion` + struct DenyUnsubscribeVersionAsStackLimitReached; + impl DenyExecution for DenyUnsubscribeVersionAsStackLimitReached { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + if instructions.len() != 1 { + return Ok(()) + } + match instructions.get(0).unwrap() { + UnsubscribeVersion { .. } => Err(ProcessMessageError::StackLimitReached), + _ => Ok(()), + } + } + } + + /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single `ClearError`, + /// else return `ProcessMessageError::Yield` + struct AllowSingleClearErrorOrYield; + impl ShouldExecute for AllowSingleClearErrorOrYield { + fn should_execute( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().assert_remaining_insts(1)?.match_next_inst( + |inst| match inst { + ClearError { .. } => Ok(()), + _ => Err(ProcessMessageError::Yield), + }, + )?; + Ok(()) + } + } + + /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains `ClearTopic` and + /// origin from `Here`, else return `ProcessMessageError::Unsupported` + struct AllowClearTopicFromHere; + impl ShouldExecute for AllowClearTopicFromHere { + fn should_execute( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + ensure!(origin.clone() == Here.into_location(), ProcessMessageError::Unsupported); + let mut found = false; + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTopic { .. } => { + found = true; + Ok(ControlFlow::Break(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + ensure!(found, ProcessMessageError::Unsupported); + Ok(()) + } + } + // closure for (xcm, origin) testing with `DenyThenTry` + let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + pub type Barrier = DenyThenTry< + ( + DenyClearTransactStatusAsYield, + DenyClearOriginFromHereAsBadFormat, + DenyUnsubscribeVersionAsStackLimitReached, + ), + (AllowSingleClearErrorOrYield, AllowClearTopicFromHere), + >; + assert_eq!( + Barrier::should_execute( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + + // Deny cases: + // trigger DenyClearTransactStatusAsYield + assert_should_execute( + vec![ClearTransactStatus], + Parachain(1).into_location(), + Err(ProcessMessageError::Yield), + ); + // DenyClearTransactStatusAsYield wins against AllowSingleClearErrorOrYield + assert_should_execute( + vec![ClearError, ClearTransactStatus], + Parachain(1).into_location(), + Err(ProcessMessageError::Yield), + ); + // trigger DenyClearOriginFromHereAsBadFormat + assert_should_execute( + vec![ClearOrigin], + Here.into_location(), + Err(ProcessMessageError::BadFormat), + ); + // trigger DenyUnsubscribeVersionAsStackLimitReached + assert_should_execute( + vec![UnsubscribeVersion], + Here.into_location(), + Err(ProcessMessageError::StackLimitReached), + ); + + // deny because none of the allow items match + assert_should_execute( + vec![ClearError, ClearTopic], + Parachain(1).into_location(), + Err(ProcessMessageError::Unsupported), + ); + + // ok + assert_should_execute(vec![ClearError], Parachain(1).into_location(), Ok(())); + assert_should_execute(vec![ClearTopic], Here.into(), Ok(())); + assert_should_execute(vec![ClearError, ClearTopic], Here.into_location(), Ok(())); +} + +#[test] +fn deny_reserve_transfer_to_relaychain_should_work() { + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyReserveTransferToRelayChain::deny_execution( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + // deny DepositReserveAsset to RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // deny InitiateReserveWithdraw to RelayChain + assert_deny_execution( + vec![InitiateReserveWithdraw { + assets: Wild(All), + reserve: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // deny TransferReserveAsset to RelayChain + assert_deny_execution( + vec![TransferReserveAsset { + assets: vec![].into(), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // accept DepositReserveAsset to destination other than RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Here.into_location(), + xcm: vec![].into(), + }], + Here.into_location(), + Ok(()), + ); + // others instructions should pass + assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); +} diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index bec7b253977b..127888104a4a 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -31,6 +31,7 @@ pub use codec::{Decode, Encode}; pub use core::{ cell::{Cell, RefCell}, fmt::Debug, + ops::ControlFlow, }; use frame_support::traits::{ContainsPair, Everything}; pub use frame_support::{ @@ -40,11 +41,11 @@ pub use frame_support::{ traits::{Contains, Get, IsInVec}, }; pub use xcm::latest::{prelude::*, QueryId, Weight}; -use xcm_executor::traits::{Properties, QueryHandler, QueryResponseStatus}; pub use xcm_executor::{ traits::{ - AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, Enact, ExportXcm, FeeManager, - FeeReason, LockError, OnResponse, TransactAsset, + AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm, + FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler, + QueryResponseStatus, TransactAsset, }, AssetsInHolding, Config, }; diff --git a/polkadot/xcm/xcm-executor/src/traits/mod.rs b/polkadot/xcm/xcm-executor/src/traits/mod.rs index feb2922bcdff..fe73477f516f 100644 --- a/polkadot/xcm/xcm-executor/src/traits/mod.rs +++ b/polkadot/xcm/xcm-executor/src/traits/mod.rs @@ -42,7 +42,7 @@ pub use on_response::{OnResponse, QueryHandler, QueryResponseStatus, VersionChan mod process_transaction; pub use process_transaction::ProcessTransaction; mod should_execute; -pub use should_execute::{CheckSuspension, Properties, ShouldExecute}; +pub use should_execute::{CheckSuspension, DenyExecution, Properties, ShouldExecute}; mod transact_asset; pub use transact_asset::TransactAsset; mod hrmp; diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index ec9ef70cc817..48a562a1648d 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -127,3 +127,67 @@ impl CheckSuspension for Tuple { false } } + +/// Trait to determine whether the execution engine should not execute a given XCM. +/// +/// Can be amalgamated into a tuple to have multiple traits. If any of the tuple elements returns +/// `Err(ProcessMessageError)`, the execution stops. Else, `Ok(())` is returned if all elements +/// accept the message. +pub trait DenyExecution { + /// Returns `Ok(())` if there is no reason to deny execution, + /// while `Err(ProcessMessageError)` indicates there is a reason to deny execution. + /// + /// - `origin`: The origin (sender) of the message. + /// - `instructions`: The message itself. + /// - `max_weight`: The (possibly over-) estimation of the weight of execution of the message. + /// - `properties`: Various pre-established properties of the message which may be mutated by + /// this API. + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError>; +} + +#[impl_trait_for_tuples::impl_for_tuples(10)] +impl DenyExecution for Tuple { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + for_tuples!( #( + let barrier = core::any::type_name::(); + match Tuple::deny_execution(origin, instructions, max_weight, properties) { + Err(error) => { + tracing::error!( + target: "xcm::deny_execution", + ?origin, + ?instructions, + ?max_weight, + ?properties, + ?error, + %barrier, + "did not pass barrier", + ); + return Err(error); + }, + Ok(()) => { + tracing::trace!( + target: "xcm::deny_execution", + ?origin, + ?instructions, + ?max_weight, + ?properties, + %barrier, + "pass barrier", + ); + }, + } + )* ); + + Ok(()) + } +} diff --git a/prdoc/pr_7169.prdoc b/prdoc/pr_7169.prdoc new file mode 100644 index 000000000000..f78dbfd8d2cd --- /dev/null +++ b/prdoc/pr_7169.prdoc @@ -0,0 +1,14 @@ +title: 'xcm: fix DenyThenTry when work with multiple Deny tuples' +doc: +- audience: Runtime User + description: |- + This PR changes the behavior of DenyThenTry to fix #7148 + If any of the tuple elements returns `Err(())`, the execution stops. + Else, `Ok(_)` is returned if all elements accept the message. +crates: +- name: staging-xcm-executor + bump: minor +- name: staging-xcm-builder + bump: minor +- name: bridge-hub-common + bump: minor