decode call after spec_version check (#663)

bridges/bin/millau/runtime/src/lib.rs

@@ -215,6 +215,7 @@ impl pallet_bridge_call_dispatch::Config for Runtime {
 	type Event = Event;
 	type MessageId = (bp_message_lane::LaneId, bp_message_lane::MessageNonce);
 	type Call = Call;
+	type EncodedCall = crate::rialto_messages::FromRialtoEncodedCall;
 	type SourceChainAccountId = bp_rialto::AccountId;
 	type TargetChainAccountPublic = MultiSigner;
 	type TargetChainSignature = MultiSignature;
@@ -602,7 +603,7 @@ impl_runtime_apis! {
 /// This way, the owner of `millau_account_id` on Millau proves that the Rialto account private key
 /// is also under his control.
 pub fn rialto_account_ownership_digest<Call, AccountId, SpecVersion>(
-	rialto_call: Call,
+	rialto_call: &Call,
 	millau_account_id: AccountId,
 	rialto_spec_version: SpecVersion,
 ) -> sp_std::vec::Vec<u8>

bridges/bin/millau/runtime/src/rialto_messages.rs

@@ -63,6 +63,9 @@ pub type ToRialtoMessageVerifier = messages::source::FromThisChainMessageVerifie
 /// Message payload for Rialto -> Millau messages.
 pub type FromRialtoMessagePayload = messages::target::FromBridgedChainMessagePayload<WithRialtoMessageBridge>;
 
+/// Encoded Millau Call as it comes from Rialto.
+pub type FromRialtoEncodedCall = messages::target::FromBridgedChainEncodedMessageCall<WithRialtoMessageBridge>;
+
 /// Messages proof for Rialto -> Millau messages.
 type FromRialtoMessagesProof = messages::target::FromBridgedChainMessagesProof<WithRialtoMessageBridge>;
 

bridges/bin/rialto/runtime/src/lib.rs

@@ -266,6 +266,7 @@ impl pallet_bridge_call_dispatch::Config for Runtime {
 	type Event = Event;
 	type MessageId = (bp_message_lane::LaneId, bp_message_lane::MessageNonce);
 	type Call = Call;
+	type EncodedCall = crate::millau_messages::FromMillauEncodedCall;
 	type SourceChainAccountId = bp_millau::AccountId;
 	type TargetChainAccountPublic = MultiSigner;
 	type TargetChainSignature = MultiSignature;
@@ -991,7 +992,7 @@ impl_runtime_apis! {
 /// This way, the owner of `rialto_account_id` on Rialto proves that the 'millau' account private key
 /// is also under his control.
 pub fn millau_account_ownership_digest<Call, AccountId, SpecVersion>(
-	millau_call: Call,
+	millau_call: &Call,
 	rialto_account_id: AccountId,
 	millau_spec_version: SpecVersion,
 ) -> sp_std::vec::Vec<u8>

bridges/bin/rialto/runtime/src/millau_messages.rs

@@ -63,6 +63,9 @@ pub type ToMillauMessageVerifier = messages::source::FromThisChainMessageVerifie
 /// Message payload for Millau -> Rialto messages.
 pub type FromMillauMessagePayload = messages::target::FromBridgedChainMessagePayload<WithMillauMessageBridge>;
 
+/// Encoded Rialto Call as it comes from Millau.
+pub type FromMillauEncodedCall = messages::target::FromBridgedChainEncodedMessageCall<WithMillauMessageBridge>;
+
 /// Call-dispatch based message dispatch for Millau -> Rialto messages.
 pub type FromMillauMessageDispatch = messages::target::FromBridgedChainMessageDispatch<
 	WithMillauMessageBridge,

bridges/bin/runtime-common/src/messages.rs

@@ -27,7 +27,7 @@ use bp_message_lane::{
 	InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData,
 };
 use bp_runtime::InstanceId;
-use codec::{Compact, Decode, Encode, Input};
+use codec::{Compact, Decode, Encode};
 use frame_support::{traits::Instance, weights::Weight, RuntimeDebug};
 use hash_db::Hasher;
 use pallet_substrate_bridge::StorageProofChecker;
@@ -319,11 +319,11 @@ pub mod target {
 	>;
 
 	/// Decoded Bridged -> This message payload.
-	pub type FromBridgedChainDecodedMessagePayload<B> = pallet_bridge_call_dispatch::MessagePayload<
+	pub type FromBridgedChainMessagePayload<B> = pallet_bridge_call_dispatch::MessagePayload<
 		AccountIdOf<BridgedChain<B>>,
 		SignerOf<ThisChain<B>>,
 		SignatureOf<ThisChain<B>>,
-		CallOf<ThisChain<B>>,
+		FromBridgedChainEncodedMessageCall<B>,
 	>;
 
 	/// Messages proof from bridged chain:
@@ -340,33 +340,21 @@ pub mod target {
 		MessageNonce,
 	);
 
-	/// Message payload for Bridged -> This messages.
-	pub struct FromBridgedChainMessagePayload<B: MessageBridge>(pub(crate) FromBridgedChainDecodedMessagePayload<B>);
-
-	impl<B: MessageBridge> From<FromBridgedChainDecodedMessagePayload<B>> for FromBridgedChainMessagePayload<B> {
-		fn from(decoded_payload: FromBridgedChainDecodedMessagePayload<B>) -> Self {
-			Self(decoded_payload)
-		}
+	/// Encoded Call of This chain as it is transferred over bridge.
+	///
+	/// Our Call is opaque (`Vec<u8>`) for Bridged chain. So it is encoded, prefixed with
+	/// vector length. Custom decode implementation here is exactly to deal with this.
+	#[derive(Decode, Encode, RuntimeDebug, PartialEq)]
+	pub struct FromBridgedChainEncodedMessageCall<B> {
+		pub(crate) encoded_call: Vec<u8>,
+		pub(crate) _marker: PhantomData<B>,
 	}
 
-	impl<B: MessageBridge> Decode for FromBridgedChainMessagePayload<B> {
-		fn decode<I: Input>(input: &mut I) -> Result<Self, codec::Error> {
-			// for bridged chain our Calls are opaque - they're encoded to Vec<u8> by submitter
-			// => skip encoded vec length here before decoding Call
-			let spec_version = pallet_bridge_call_dispatch::SpecVersion::decode(input)?;
-			let weight = frame_support::weights::Weight::decode(input)?;
-			let origin = FromBridgedChainMessageCallOrigin::<B>::decode(input)?;
-			let _skipped_length = Compact::<u32>::decode(input)?;
-			let call = CallOf::<ThisChain<B>>::decode(input)?;
-
-			Ok(FromBridgedChainMessagePayload(
-				pallet_bridge_call_dispatch::MessagePayload {
-					spec_version,
-					weight,
-					origin,
-					call,
-				},
-			))
+	impl<B: MessageBridge> From<FromBridgedChainEncodedMessageCall<B>> for Result<CallOf<ThisChain<B>>, ()> {
+		fn from(encoded_call: FromBridgedChainEncodedMessageCall<B>) -> Self {
+			let mut input = &encoded_call.encoded_call[..];
+			let _skipped_length = Compact::<u32>::decode(&mut input).map_err(drop)?;
+			CallOf::<ThisChain<B>>::decode(&mut input).map_err(drop)
 		}
 	}
 
@@ -385,22 +373,14 @@ pub mod target {
 		<ThisRuntime as pallet_bridge_call_dispatch::Config<ThisCallDispatchInstance>>::Event:
 			From<pallet_bridge_call_dispatch::RawEvent<(LaneId, MessageNonce), ThisCallDispatchInstance>>,
 		pallet_bridge_call_dispatch::Module<ThisRuntime, ThisCallDispatchInstance>:
-			bp_message_dispatch::MessageDispatch<
-				(LaneId, MessageNonce),
-				Message = FromBridgedChainDecodedMessagePayload<B>,
-			>,
+			bp_message_dispatch::MessageDispatch<(LaneId, MessageNonce), Message = FromBridgedChainMessagePayload<B>>,
 	{
 		type DispatchPayload = FromBridgedChainMessagePayload<B>;
 
 		fn dispatch_weight(
 			message: &DispatchMessage<Self::DispatchPayload, BalanceOf<BridgedChain<B>>>,
 		) -> frame_support::weights::Weight {
-			message
-				.data
-				.payload
-				.as_ref()
-				.map(|payload| payload.0.weight)
-				.unwrap_or(0)
+			message.data.payload.as_ref().map(|payload| payload.weight).unwrap_or(0)
 		}
 
 		fn dispatch(message: DispatchMessage<Self::DispatchPayload, BalanceOf<BridgedChain<B>>>) {
@@ -408,7 +388,7 @@ pub mod target {
 			pallet_bridge_call_dispatch::Module::<ThisRuntime, ThisCallDispatchInstance>::dispatch(
 				B::INSTANCE,
 				message_id,
-				message.data.payload.map_err(drop).map(|payload| payload.0),
+				message.data.payload.map_err(drop),
 			);
 		}
 	}
@@ -595,6 +575,7 @@ mod tests {
 	const BRIDGED_CHAIN_MAX_EXTRINSIC_SIZE: u32 = 1024;
 
 	/// Bridge that is deployed on ThisChain and allows sending/receiving messages to/from BridgedChain;
+	#[derive(Debug, PartialEq)]
 	struct OnThisChainBridge;
 
 	impl MessageBridge for OnThisChainBridge {
@@ -635,6 +616,7 @@ mod tests {
 	}
 
 	/// Bridge that is deployed on BridgedChain and allows sending/receiving messages to/from ThisChain;
+	#[derive(Debug, PartialEq)]
 	struct OnBridgedChainBridge;
 
 	impl MessageBridge for OnBridgedChainBridge {
@@ -804,12 +786,15 @@ mod tests {
 			target::FromBridgedChainMessagePayload::<OnThisChainBridge>::decode(&mut &message_on_bridged_chain[..])
 				.unwrap();
 		assert_eq!(
-			message_on_this_chain.0,
-			target::FromBridgedChainDecodedMessagePayload::<OnThisChainBridge> {
+			message_on_this_chain,
+			target::FromBridgedChainMessagePayload::<OnThisChainBridge> {
 				spec_version: 1,
 				weight: 100,
 				origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot,
-				call: ThisChainCall::Transfer,
+				call: target::FromBridgedChainEncodedMessageCall::<OnThisChainBridge> {
+					encoded_call: ThisChainCall::Transfer.encode(),
+					_marker: PhantomData::default(),
+				},
 			}
 		);
 	}

bridges/modules/call-dispatch/src/lib.rs

@@ -134,6 +134,16 @@ pub trait Config<I = DefaultInstance>: frame_system::Config {
 			Origin = <Self as frame_system::Config>::Origin,
 			PostInfo = frame_support::dispatch::PostDispatchInfo,
 		>;
+	/// The type that is used to wrap the `Self::Call` when it is moved over bridge.
+	///
+	/// The idea behind this is to avoid `Call` conversion/decoding until we'll be sure
+	/// that all other stuff (like `spec_version`) is ok. If we would try to decode
+	/// `Call` which has been encoded using previous `spec_version`, then we might end
+	/// up with decoding error, instead of `MessageVersionSpecMismatch`.
+	///
+	/// The `Encode` implementation should match `Encode` implementation of the actual
+	/// `Call`, that (may) have been used to produce signature for `CallOrigin::TargetAccount`.
+	type EncodedCall: Decode + Encode + Into<Result<<Self as Config<I>>::Call, ()>>;
 	/// A type which can be turned into an AccountId from a 256-bit hash.
 	///
 	/// Used when deriving target chain AccountIds from source chain AccountIds.
@@ -160,6 +170,8 @@ decl_event!(
 		MessageSignatureMismatch(InstanceId, MessageId),
 		/// Message has been dispatched with given result.
 		MessageDispatched(InstanceId, MessageId, DispatchResult),
+		/// We have failed to decode Call from the message.
+		MessageCallDecodeFailed(InstanceId, MessageId),
 		/// Phantom member, never used. Needed to handle multiple pallet instances.
 		_Dummy(PhantomData<I>),
 	}
@@ -174,12 +186,8 @@ decl_module! {
 }
 
 impl<T: Config<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
-	type Message = MessagePayload<
-		T::SourceChainAccountId,
-		T::TargetChainAccountPublic,
-		T::TargetChainSignature,
-		<T as Config<I>>::Call,
-	>;
+	type Message =
+		MessagePayload<T::SourceChainAccountId, T::TargetChainAccountPublic, T::TargetChainSignature, T::EncodedCall>;
 
 	fn dispatch_weight(message: &Self::Message) -> Weight {
 		message.weight
@@ -216,28 +224,6 @@ impl<T: Config<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
 			return;
 		}
 
-		// verify weight
-		// (we want passed weight to be at least equal to pre-dispatch weight of the call
-		// because otherwise Calls may be dispatched at lower price)
-		let dispatch_info = message.call.get_dispatch_info();
-		let expected_weight = dispatch_info.weight;
-		if message.weight < expected_weight {
-			frame_support::debug::trace!(
-				"Message {:?}/{:?}: passed weight is too low. Expected at least {:?}, got {:?}",
-				bridge,
-				id,
-				expected_weight,
-				message.weight,
-			);
-			Self::deposit_event(RawEvent::MessageWeightMismatch(
-				bridge,
-				id,
-				expected_weight,
-				message.weight,
-			));
-			return;
-		}
-
 		// prepare dispatch origin
 		let origin_account = match message.origin {
 			CallOrigin::SourceRoot => {
@@ -247,8 +233,7 @@ impl<T: Config<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
 				target_id
 			}
 			CallOrigin::TargetAccount(source_account_id, target_public, target_signature) => {
-				let digest =
-					account_ownership_digest(message.call.clone(), source_account_id, message.spec_version, bridge);
+				let digest = account_ownership_digest(&message.call, source_account_id, message.spec_version, bridge);
 
 				let target_account = target_public.into_account();
 				if !target_signature.verify(&digest[..], &target_account) {
@@ -274,11 +259,43 @@ impl<T: Config<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
 			}
 		};
 
+		// now that we have everything checked, let's decode the call
+		let call = match message.call.into() {
+			Ok(call) => call,
+			Err(_) => {
+				frame_support::debug::trace!("Failed to decode Call from message {:?}/{:?}", bridge, id,);
+				Self::deposit_event(RawEvent::MessageCallDecodeFailed(bridge, id));
+				return;
+			}
+		};
+
+		// verify weight
+		// (we want passed weight to be at least equal to pre-dispatch weight of the call
+		// because otherwise Calls may be dispatched at lower price)
+		let dispatch_info = call.get_dispatch_info();
+		let expected_weight = dispatch_info.weight;
+		if message.weight < expected_weight {
+			frame_support::debug::trace!(
+				"Message {:?}/{:?}: passed weight is too low. Expected at least {:?}, got {:?}",
+				bridge,
+				id,
+				expected_weight,
+				message.weight,
+			);
+			Self::deposit_event(RawEvent::MessageWeightMismatch(
+				bridge,
+				id,
+				expected_weight,
+				message.weight,
+			));
+			return;
+		}
+
 		// finally dispatch message
 		let origin = RawOrigin::Signed(origin_account).into();
 
-		frame_support::debug::trace!("Message being dispatched is: {:?}", &message.call);
-		let dispatch_result = message.call.dispatch(origin);
+		frame_support::debug::trace!("Message being dispatched is: {:?}", &call);
+		let dispatch_result = call.dispatch(origin);
 		let actual_call_weight = extract_actual_weight(&dispatch_result, &dispatch_info);
 
 		frame_support::debug::trace!(
@@ -339,7 +356,7 @@ where
 /// private key. This way, the owner of `source_account_id` on the source chain proves that
 /// the target chain account private key is also under his control.
 pub fn account_ownership_digest<Call, AccountId, SpecVersion, BridgeId>(
-	call: Call,
+	call: &Call,
 	source_account_id: AccountId,
 	target_spec_version: SpecVersion,
 	source_instance_id: BridgeId,
@@ -471,9 +488,19 @@ mod tests {
 		type TargetChainAccountPublic = TestAccountPublic;
 		type TargetChainSignature = TestSignature;
 		type Call = Call;
+		type EncodedCall = EncodedCall;
 		type AccountIdConverter = AccountIdConverter;
 	}
 
+	#[derive(Decode, Encode)]
+	pub struct EncodedCall(Vec<u8>);
+
+	impl From<EncodedCall> for Result<Call, ()> {
+		fn from(call: EncodedCall) -> Result<Call, ()> {
+			Call::decode(&mut &call.0[..]).map_err(drop)
+		}
+	}
+
 	const TEST_SPEC_VERSION: SpecVersion = 0;
 	const TEST_WEIGHT: Weight = 1_000_000_000;
 
@@ -492,7 +519,7 @@ mod tests {
 			spec_version: TEST_SPEC_VERSION,
 			weight: TEST_WEIGHT,
 			origin,
-			call,
+			call: EncodedCall(call.encode()),
 		}
 	}
 
@@ -617,6 +644,30 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn should_fail_on_call_decode() {
+		new_test_ext().execute_with(|| {
+			let bridge = b"ethb".to_owned();
+			let id = [0; 4];
+
+			let mut message =
+				prepare_root_message(Call::System(<frame_system::Call<TestRuntime>>::remark(vec![1, 2, 3])));
+			message.call.0 = vec![];
+
+			System::set_block_number(1);
+			CallDispatch::dispatch(bridge, id, Ok(message));
+
+			assert_eq!(
+				System::events(),
+				vec![EventRecord {
+					phase: Phase::Initialization,
+					event: TestEvent::call_dispatch(Event::<TestRuntime>::MessageCallDecodeFailed(bridge, id)),
+					topics: vec![],
+				}],
+			);
+		});
+	}
+
 	#[test]
 	fn should_dispatch_bridge_message_from_root_origin() {
 		new_test_ext().execute_with(|| {

bridges/relays/substrate/src/main.rs

@@ -298,7 +298,7 @@ async fn run_command(command: cli::Command) -> Result<(), String> {
 				},
 				cli::Origins::Target => {
 					let digest = millau_runtime::rialto_account_ownership_digest(
-						rialto_call.clone(),
+						&rialto_call,
 						millau_account_id.clone(),
 						rialto_runtime::VERSION.spec_version,
 					);
@@ -449,7 +449,7 @@ async fn run_command(command: cli::Command) -> Result<(), String> {
 				},
 				cli::Origins::Target => {
 					let digest = rialto_runtime::millau_account_ownership_digest(
-						millau_call.clone(),
+						&millau_call,
 						rialto_account_id.clone(),
 						millau_runtime::VERSION.spec_version,
 					);
@@ -539,7 +539,7 @@ mod tests {
 		let millau_account_id: bp_millau::AccountId = millau_public.into_account();
 
 		let digest = millau_runtime::rialto_account_ownership_digest(
-			call,
+			&call,
 			millau_account_id,
 			rialto_runtime::VERSION.spec_version,
 		);
@@ -560,7 +560,7 @@ mod tests {
 		let rialto_account_id: bp_rialto::AccountId = rialto_public.into_account();
 
 		let digest = rialto_runtime::millau_account_ownership_digest(
-			call,
+			&call,
 			rialto_account_id,
 			millau_runtime::VERSION.spec_version,
 		);