diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index e0fc4bcfe6..0c7d4ff4be 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -22,7 +22,7 @@ use bp_message_dispatch::MessageDispatch as _; use bp_message_lane::{ - source_chain::LaneMessageVerifier, + source_chain::{LaneMessageVerifier, Sender}, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages}, InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData, }; @@ -149,25 +149,34 @@ pub mod source { #[derive(RuntimeDebug)] pub struct FromThisChainMessageVerifier(PhantomData); - impl - LaneMessageVerifier>, FromThisChainMessagePayload, BalanceOf>> + pub(crate) const BAD_ORIGIN: &str = "Unable to match the source origin to expected target origin."; + pub(crate) const TOO_LOW_FEE: &str = "Provided fee is below minimal threshold required by the lane."; + + impl LaneMessageVerifier>, FromThisChainMessagePayload, BalanceOf>> for FromThisChainMessageVerifier + where + B: MessageBridge, + AccountIdOf>: PartialEq + Clone, { type Error = &'static str; fn verify_message( - _submitter: &AccountIdOf>, + submitter: &Sender>>, delivery_and_dispatch_fee: &BalanceOf>, _lane: &LaneId, payload: &FromThisChainMessagePayload, ) -> Result<(), Self::Error> { + // Do the dispatch-specific check. We assume that the target chain uses + // `CallDispatch`, so we verify the message accordingly. + pallet_bridge_call_dispatch::verify_message_origin(submitter, payload).map_err(|_| BAD_ORIGIN)?; + let minimal_fee_in_bridged_tokens = estimate_message_dispatch_and_delivery_fee::(payload, B::RELAYER_FEE_PERCENT)?; // compare with actual fee paid let actual_fee_in_bridged_tokens = B::this_balance_to_bridged_balance(*delivery_and_dispatch_fee); if actual_fee_in_bridged_tokens < minimal_fee_in_bridged_tokens { - return Err("Too low fee paid"); + return Err(TOO_LOW_FEE); } Ok(()) @@ -639,7 +648,7 @@ mod tests { } } - #[derive(Debug, PartialEq, Decode, Encode)] + #[derive(Debug, PartialEq, Decode, Encode, Clone)] struct ThisChainAccountId(u32); #[derive(Debug, PartialEq, Decode, Encode)] struct ThisChainSigner(u32); @@ -780,6 +789,8 @@ mod tests { ); } + const TEST_LANE_ID: &LaneId = b"test"; + #[test] fn message_fee_is_checked_by_verifier() { const EXPECTED_MINIMAL_FEE: u32 = 2640; @@ -802,20 +813,91 @@ mod tests { ); // and now check that the verifier checks the fee - assert!( + assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &ThisChainAccountId(0), + &Sender::Root, &ThisChainBalance(1), - &*b"test", + &TEST_LANE_ID, &payload, - ) - .is_err(), + ), + Err(source::TOO_LOW_FEE) ); assert!( source::FromThisChainMessageVerifier::::verify_message( - &ThisChainAccountId(0), + &Sender::Root, &ThisChainBalance(1_000_000), - &*b"test", + &TEST_LANE_ID, + &payload, + ) + .is_ok(), + ); + } + + #[test] + fn should_disallow_root_calls_from_regular_accounts() { + // payload of the This -> Bridged chain message + let payload = source::FromThisChainMessagePayload:: { + spec_version: 1, + weight: 100, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, + call: vec![42], + }; + + // and now check that the verifier checks the fee + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &Sender::Signed(ThisChainAccountId(0)), + &ThisChainBalance(1_000_000), + &TEST_LANE_ID, + &payload, + ), + Err(source::BAD_ORIGIN) + ); + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &Sender::None, + &ThisChainBalance(1_000_000), + &TEST_LANE_ID, + &payload, + ), + Err(source::BAD_ORIGIN) + ); + assert!( + source::FromThisChainMessageVerifier::::verify_message( + &Sender::Root, + &ThisChainBalance(1_000_000), + &TEST_LANE_ID, + &payload, + ) + .is_ok(), + ); + } + + #[test] + fn should_verify_source_and_target_origin_matching() { + // payload of the This -> Bridged chain message + let payload = source::FromThisChainMessagePayload:: { + spec_version: 1, + weight: 100, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceAccount(ThisChainAccountId(1)), + call: vec![42], + }; + + // and now check that the verifier checks the fee + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &Sender::Signed(ThisChainAccountId(0)), + &ThisChainBalance(1_000_000), + &TEST_LANE_ID, + &payload, + ), + Err(source::BAD_ORIGIN) + ); + assert!( + source::FromThisChainMessageVerifier::::verify_message( + &Sender::Signed(ThisChainAccountId(1)), + &ThisChainBalance(1_000_000), + &TEST_LANE_ID, &payload, ) .is_ok(), diff --git a/bridges/modules/call-dispatch/src/lib.rs b/bridges/modules/call-dispatch/src/lib.rs index 63d3ff25d5..6e7d26f545 100644 --- a/bridges/modules/call-dispatch/src/lib.rs +++ b/bridges/modules/call-dispatch/src/lib.rs @@ -30,11 +30,12 @@ use codec::{Decode, Encode}; use frame_support::{ decl_event, decl_module, decl_storage, dispatch::{Dispatchable, Parameter}, + ensure, traits::Get, weights::{extract_actual_weight, GetDispatchInfo}, RuntimeDebug, }; -use frame_system::{ensure_root, ensure_signed, RawOrigin}; +use frame_system::RawOrigin; use sp_runtime::{ traits::{BadOrigin, Convert, IdentifyAccount, MaybeDisplay, MaybeSerializeDeserialize, Member, Verify}, DispatchResult, @@ -266,39 +267,31 @@ impl, I: Instance> MessageDispatch for Module { /// For example, if a message is sent from a "regular" account on the source chain it will not be /// allowed to be dispatched as Root on the target chain. This is a useful check to do on the source /// chain _before_ sending a message whose dispatch will be rejected on the target chain. -pub fn verify_message_origin< - SourceChainOuterOrigin, - SourceChainAccountId, - TargetChainAccountPublic, - TargetChainSignature, - Call, ->( - sender_origin: SourceChainOuterOrigin, +pub fn verify_message_origin( + sender_origin: &RawOrigin, message: &MessagePayload, ) -> Result, BadOrigin> where - SourceChainOuterOrigin: Into, SourceChainOuterOrigin>>, - SourceChainAccountId: PartialEq, + SourceChainAccountId: PartialEq + Clone, { match message.origin { CallOrigin::SourceRoot => { - ensure_root(sender_origin)?; + ensure!(sender_origin == &RawOrigin::Root, BadOrigin); Ok(None) } CallOrigin::TargetAccount(ref source_account_id, _, _) => { - let source_chain_signer = ensure_signed(sender_origin)?; - if source_chain_signer != *source_account_id { - return Err(BadOrigin); - } - - Ok(Some(source_chain_signer)) + ensure!( + sender_origin == &RawOrigin::Signed(source_account_id.clone()), + BadOrigin + ); + Ok(Some(source_account_id.clone())) } CallOrigin::SourceAccount(ref source_account_id) => { - let source_chain_signer = ensure_signed(sender_origin)?; - if source_chain_signer != *source_account_id { - return Err(BadOrigin); - } - Ok(Some(source_chain_signer)) + ensure!( + sender_origin == &RawOrigin::Signed(source_account_id.clone()), + BadOrigin + ); + Ok(Some(source_account_id.clone())) } } } @@ -617,14 +610,11 @@ mod tests { let message = prepare_root_message(call); // When message is sent by Root, CallOrigin::SourceRoot is allowed - assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Root), &message), - Ok(None) - )); + assert!(matches!(verify_message_origin(&RawOrigin::Root, &message), Ok(None))); // when message is sent by some real account, CallOrigin::SourceRoot is not allowed assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Signed(1)), &message), + verify_message_origin(&RawOrigin::Signed(1), &message), Err(BadOrigin) )); } @@ -636,20 +626,20 @@ mod tests { // When message is sent by Root, CallOrigin::TargetAccount is not allowed assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Root), &message), + verify_message_origin(&RawOrigin::Root, &message), Err(BadOrigin) )); // When message is sent by some other account, it is rejected assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Signed(2)), &message), + verify_message_origin(&RawOrigin::Signed(2), &message), Err(BadOrigin) )); // When message is sent by a real account, it is allowed to have origin // CallOrigin::TargetAccount assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Signed(1)), &message), + verify_message_origin(&RawOrigin::Signed(1), &message), Ok(Some(1)) )); } @@ -661,19 +651,19 @@ mod tests { // Sending a message from the expected origin account works assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Signed(1)), &message), + verify_message_origin(&RawOrigin::Signed(1), &message), Ok(Some(1)) )); // If we send a message from a different account, it is rejected assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Signed(2)), &message), + verify_message_origin(&RawOrigin::Signed(2), &message), Err(BadOrigin) )); // If we try and send the message from Root, it is also rejected assert!(matches!( - verify_message_origin(Origin::from(RawOrigin::Root), &message), + verify_message_origin(&RawOrigin::Root, &message), Err(BadOrigin) )); } diff --git a/bridges/modules/message-lane/src/instant_payments.rs b/bridges/modules/message-lane/src/instant_payments.rs index efc9795b2e..fe860fdb3f 100644 --- a/bridges/modules/message-lane/src/instant_payments.rs +++ b/bridges/modules/message-lane/src/instant_payments.rs @@ -17,7 +17,7 @@ //! Implementation of `MessageDeliveryAndDispatchPayment` trait on top of `Currency` trait. //! All payments are instant. -use bp_message_lane::source_chain::MessageDeliveryAndDispatchPayment; +use bp_message_lane::source_chain::{MessageDeliveryAndDispatchPayment, Sender}; use codec::Encode; use frame_support::traits::{Currency as CurrencyT, ExistenceRequirement}; use sp_std::fmt::Debug; @@ -37,11 +37,20 @@ where type Error = &'static str; fn pay_delivery_and_dispatch_fee( - submitter: &AccountId, + submitter: &Sender, fee: &Currency::Balance, relayer_fund_account: &AccountId, ) -> Result<(), Self::Error> { - Currency::transfer(submitter, relayer_fund_account, *fee, ExistenceRequirement::AllowDeath).map_err(Into::into) + match submitter { + Sender::Signed(submitter) => { + Currency::transfer(submitter, relayer_fund_account, *fee, ExistenceRequirement::AllowDeath) + .map_err(Into::into) + } + Sender::Root | Sender::None => { + // fixme: we might want to add root account id to this struct. + Err("Root and None account is not allowed to send regular messages.") + } + } } fn pay_relayer_reward( diff --git a/bridges/modules/message-lane/src/lib.rs b/bridges/modules/message-lane/src/lib.rs index 13d4384893..1c8553b8bb 100644 --- a/bridges/modules/message-lane/src/lib.rs +++ b/bridges/modules/message-lane/src/lib.rs @@ -255,18 +255,19 @@ decl_module! { delivery_and_dispatch_fee: T::OutboundMessageFee, ) -> DispatchResult { ensure_operational::()?; - let submitter = ensure_signed(origin)?; + let submitter = origin.into().map_err(|_| BadOrigin)?; // let's first check if message can be delivered to target chain - T::TargetHeaderChain::verify_message(&payload).map_err(|err| { - frame_support::debug::trace!( - "Message to lane {:?} is rejected by target chain: {:?}", - lane_id, - err, - ); + T::TargetHeaderChain::verify_message(&payload) + .map_err(|err| { + frame_support::debug::trace!( + "Message to lane {:?} is rejected by target chain: {:?}", + lane_id, + err, + ); - Error::::MessageRejectedByChainVerifier - })?; + Error::::MessageRejectedByChainVerifier + })?; // now let's enforce any additional lane rules T::LaneMessageVerifier::verify_message( diff --git a/bridges/modules/message-lane/src/mock.rs b/bridges/modules/message-lane/src/mock.rs index 149f27ebcf..cdf29938d8 100644 --- a/bridges/modules/message-lane/src/mock.rs +++ b/bridges/modules/message-lane/src/mock.rs @@ -17,7 +17,7 @@ use crate::Trait; use bp_message_lane::{ - source_chain::{LaneMessageVerifier, MessageDeliveryAndDispatchPayment, TargetHeaderChain}, + source_chain::{LaneMessageVerifier, MessageDeliveryAndDispatchPayment, Sender, TargetHeaderChain}, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, }; @@ -209,7 +209,7 @@ impl LaneMessageVerifier for TestLaneMes type Error = &'static str; fn verify_message( - _submitter: &AccountId, + _submitter: &Sender, delivery_and_dispatch_fee: &TestMessageFee, _lane: &LaneId, _payload: &TestPayload, @@ -234,7 +234,7 @@ impl TestMessageDeliveryAndDispatchPayment { /// Returns true if given fee has been paid by given submitter. pub fn is_fee_paid(submitter: AccountId, fee: TestMessageFee) -> bool { - frame_support::storage::unhashed::get(b":message-fee:") == Some((submitter, fee)) + frame_support::storage::unhashed::get(b":message-fee:") == Some((Sender::Signed(submitter), fee)) } /// Returns true if given relayer has been rewarded with given balance. The reward-paid flag is @@ -249,7 +249,7 @@ impl MessageDeliveryAndDispatchPayment for TestMessag type Error = &'static str; fn pay_delivery_and_dispatch_fee( - submitter: &AccountId, + submitter: &Sender, fee: &TestMessageFee, _relayer_fund_account: &AccountId, ) -> Result<(), Self::Error> { diff --git a/bridges/primitives/message-lane/Cargo.toml b/bridges/primitives/message-lane/Cargo.toml index 63380914e7..87088aed64 100644 --- a/bridges/primitives/message-lane/Cargo.toml +++ b/bridges/primitives/message-lane/Cargo.toml @@ -12,6 +12,7 @@ codec = { package = "parity-scale-codec", version = "1.3.1", default-features = # Substrate Dependencies frame-support = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } +frame-system = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } [features] @@ -19,5 +20,6 @@ default = ["std"] std = [ "codec/std", "frame-support/std", + "frame-system/std", "sp-std/std" ] diff --git a/bridges/primitives/message-lane/src/source_chain.rs b/bridges/primitives/message-lane/src/source_chain.rs index 4bf964c9b0..f854b0356c 100644 --- a/bridges/primitives/message-lane/src/source_chain.rs +++ b/bridges/primitives/message-lane/src/source_chain.rs @@ -21,12 +21,15 @@ use crate::{InboundLaneData, LaneId}; use frame_support::Parameter; use sp_std::fmt::Debug; +/// The sender of the message on the source chain. +pub type Sender = frame_system::RawOrigin; + /// Target chain API. Used by source chain to verify target chain proofs. /// /// All implementations of this trait should only work with finalized data that /// can't change. Wrong implementation may lead to invalid lane states (i.e. lane /// that's stuck) and/or processing messages without paying fees. -pub trait TargetHeaderChain { +pub trait TargetHeaderChain { /// Error type. type Error: Debug + Into<&'static str>; @@ -50,7 +53,7 @@ pub trait TargetHeaderChain { /// Verify messages delivery proof and return lane && nonce of the latest recevied message. fn verify_messages_delivery_proof( proof: Self::MessagesDeliveryProof, - ) -> Result<(LaneId, InboundLaneData), Self::Error>; + ) -> Result<(LaneId, InboundLaneData), Self::Error>; } /// Lane message verifier. @@ -67,7 +70,7 @@ pub trait LaneMessageVerifier { /// Verify message payload and return Ok(()) if message is valid and allowed to be sent over the lane. fn verify_message( - submitter: &Submitter, + submitter: &Sender, delivery_and_dispatch_fee: &Fee, lane: &LaneId, payload: &Payload, @@ -94,7 +97,7 @@ pub trait MessageDeliveryAndDispatchPayment { /// Withhold/write-off delivery_and_dispatch_fee from submitter account to /// some relayers-fund account. fn pay_delivery_and_dispatch_fee( - submitter: &AccountId, + submitter: &Sender, fee: &Balance, relayer_fund_account: &AccountId, ) -> Result<(), Self::Error>;