Verify Source origin in TargetChainVerifier (#558)

* Make sure to verify sender's origin.

* Make sure to use dispatch verification.

* Add tests.

* cargo fmt --all

* Remove superfluous lifetime.

* Move the check to MessageLanVerifier.

* cargo fmt --all

* Fix docs.

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<B>(PhantomData<B>);
 
-	impl<B: MessageBridge>
-		LaneMessageVerifier<AccountIdOf<ThisChain<B>>, FromThisChainMessagePayload<B>, BalanceOf<ThisChain<B>>>
+	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<B> LaneMessageVerifier<AccountIdOf<ThisChain<B>>, FromThisChainMessagePayload<B>, BalanceOf<ThisChain<B>>>
 		for FromThisChainMessageVerifier<B>
+	where
+		B: MessageBridge,
+		AccountIdOf<ThisChain<B>>: PartialEq + Clone,
 	{
 		type Error = &'static str;
 
 		fn verify_message(
-			_submitter: &AccountIdOf<ThisChain<B>>,
+			submitter: &Sender<AccountIdOf<ThisChain<B>>>,
 			delivery_and_dispatch_fee: &BalanceOf<ThisChain<B>>,
 			_lane: &LaneId,
 			payload: &FromThisChainMessagePayload<B>,
 		) -> 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::<B>(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::<OnThisChainBridge>::verify_message(
-				&ThisChainAccountId(0),
+				&Sender::Root,
 				&ThisChainBalance(1),
-				&*b"test",
+				&TEST_LANE_ID,
 				&payload,
-			)
-			.is_err(),
+			),
+			Err(source::TOO_LOW_FEE)
 		);
 		assert!(
 			source::FromThisChainMessageVerifier::<OnThisChainBridge>::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::<OnThisChainBridge> {
+			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::<OnThisChainBridge>::verify_message(
+				&Sender::Signed(ThisChainAccountId(0)),
+				&ThisChainBalance(1_000_000),
+				&TEST_LANE_ID,
+				&payload,
+			),
+			Err(source::BAD_ORIGIN)
+		);
+		assert_eq!(
+			source::FromThisChainMessageVerifier::<OnThisChainBridge>::verify_message(
+				&Sender::None,
+				&ThisChainBalance(1_000_000),
+				&TEST_LANE_ID,
+				&payload,
+			),
+			Err(source::BAD_ORIGIN)
+		);
+		assert!(
+			source::FromThisChainMessageVerifier::<OnThisChainBridge>::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::<OnThisChainBridge> {
+			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::<OnThisChainBridge>::verify_message(
+				&Sender::Signed(ThisChainAccountId(0)),
+				&ThisChainBalance(1_000_000),
+				&TEST_LANE_ID,
+				&payload,
+			),
+			Err(source::BAD_ORIGIN)
+		);
+		assert!(
+			source::FromThisChainMessageVerifier::<OnThisChainBridge>::verify_message(
+				&Sender::Signed(ThisChainAccountId(1)),
+				&ThisChainBalance(1_000_000),
+				&TEST_LANE_ID,
 				&payload,
 			)
 			.is_ok(),

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<T: Trait<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
 /// 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<SourceChainAccountId, TargetChainAccountPublic, TargetChainSignature, Call>(
+	sender_origin: &RawOrigin<SourceChainAccountId>,
 	message: &MessagePayload<SourceChainAccountId, TargetChainAccountPublic, TargetChainSignature, Call>,
 ) -> Result<Option<SourceChainAccountId>, BadOrigin>
 where
-	SourceChainOuterOrigin: Into<Result<RawOrigin<SourceChainAccountId>, 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)
 		));
 	}

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<AccountId>,
 		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(

bridges/modules/message-lane/src/lib.rs

@@ -255,18 +255,19 @@ decl_module! {
 			delivery_and_dispatch_fee: T::OutboundMessageFee,
 		) -> DispatchResult {
 			ensure_operational::<T, I>()?;
-			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::<T, I>::MessageRejectedByChainVerifier
-			})?;
+					Error::<T, I>::MessageRejectedByChainVerifier
+				})?;
 
 			// now let's enforce any additional lane rules
 			T::LaneMessageVerifier::verify_message(

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<AccountId, TestPayload, TestMessageFee> for TestLaneMes
 	type Error = &'static str;
 
 	fn verify_message(
-		_submitter: &AccountId,
+		_submitter: &Sender<AccountId>,
 		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<AccountId, TestMessageFee> for TestMessag
 	type Error = &'static str;
 
 	fn pay_delivery_and_dispatch_fee(
-		submitter: &AccountId,
+		submitter: &Sender<AccountId>,
 		fee: &TestMessageFee,
 		_relayer_fund_account: &AccountId,
 	) -> Result<(), Self::Error> {

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"
 ]

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<AccountId> = frame_system::RawOrigin<AccountId>;
+
 /// 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<Payload, RelayerId> {
+pub trait TargetHeaderChain<Payload, AccountId> {
 	/// Error type.
 	type Error: Debug + Into<&'static str>;
 
@@ -50,7 +53,7 @@ pub trait TargetHeaderChain<Payload, RelayerId> {
 	/// Verify messages delivery proof and return lane && nonce of the latest recevied message.
 	fn verify_messages_delivery_proof(
 		proof: Self::MessagesDeliveryProof,
-	) -> Result<(LaneId, InboundLaneData<RelayerId>), Self::Error>;
+	) -> Result<(LaneId, InboundLaneData<AccountId>), Self::Error>;
 }
 
 /// Lane message verifier.
@@ -67,7 +70,7 @@ pub trait LaneMessageVerifier<Submitter, Payload, Fee> {
 
 	/// 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<Submitter>,
 		delivery_and_dispatch_fee: &Fee,
 		lane: &LaneId,
 		payload: &Payload,
@@ -94,7 +97,7 @@ pub trait MessageDeliveryAndDispatchPayment<AccountId, Balance> {
 	/// 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<AccountId>,
 		fee: &Balance,
 		relayer_fund_account: &AccountId,
 	) -> Result<(), Self::Error>;