From acee5580ca9f9402d4771da07e076e8e1e4438e0 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 27 Jan 2021 09:22:17 +0300 Subject: [PATCH] decode call after spec_version check (#663) --- bridges/bin/millau/runtime/src/lib.rs | 3 +- .../bin/millau/runtime/src/rialto_messages.rs | 3 + bridges/bin/rialto/runtime/src/lib.rs | 3 +- .../bin/rialto/runtime/src/millau_messages.rs | 3 + bridges/bin/runtime-common/src/messages.rs | 69 ++++------ bridges/modules/call-dispatch/src/lib.rs | 119 +++++++++++++----- bridges/relays/substrate/src/main.rs | 8 +- 7 files changed, 126 insertions(+), 82 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 90498c7cde..d85be22b24 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/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( - rialto_call: Call, + rialto_call: &Call, millau_account_id: AccountId, rialto_spec_version: SpecVersion, ) -> sp_std::vec::Vec diff --git a/bridges/bin/millau/runtime/src/rialto_messages.rs b/bridges/bin/millau/runtime/src/rialto_messages.rs index 9f2144b563..4ae8d09d64 100644 --- a/bridges/bin/millau/runtime/src/rialto_messages.rs +++ b/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; +/// Encoded Millau Call as it comes from Rialto. +pub type FromRialtoEncodedCall = messages::target::FromBridgedChainEncodedMessageCall; + /// Messages proof for Rialto -> Millau messages. type FromRialtoMessagesProof = messages::target::FromBridgedChainMessagesProof; diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index 313948e6a0..35f16540d4 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/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( - millau_call: Call, + millau_call: &Call, rialto_account_id: AccountId, millau_spec_version: SpecVersion, ) -> sp_std::vec::Vec diff --git a/bridges/bin/rialto/runtime/src/millau_messages.rs b/bridges/bin/rialto/runtime/src/millau_messages.rs index 61a8682d12..222da01f1c 100644 --- a/bridges/bin/rialto/runtime/src/millau_messages.rs +++ b/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; +/// Encoded Rialto Call as it comes from Millau. +pub type FromMillauEncodedCall = messages::target::FromBridgedChainEncodedMessageCall; + /// Call-dispatch based message dispatch for Millau -> Rialto messages. pub type FromMillauMessageDispatch = messages::target::FromBridgedChainMessageDispatch< WithMillauMessageBridge, diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index 924aafd070..dfb1307580 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/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 = pallet_bridge_call_dispatch::MessagePayload< + pub type FromBridgedChainMessagePayload = pallet_bridge_call_dispatch::MessagePayload< AccountIdOf>, SignerOf>, SignatureOf>, - CallOf>, + FromBridgedChainEncodedMessageCall, >; /// Messages proof from bridged chain: @@ -340,33 +340,21 @@ pub mod target { MessageNonce, ); - /// Message payload for Bridged -> This messages. - pub struct FromBridgedChainMessagePayload(pub(crate) FromBridgedChainDecodedMessagePayload); - - impl From> for FromBridgedChainMessagePayload { - fn from(decoded_payload: FromBridgedChainDecodedMessagePayload) -> Self { - Self(decoded_payload) - } + /// Encoded Call of This chain as it is transferred over bridge. + /// + /// Our Call is opaque (`Vec`) 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 { + pub(crate) encoded_call: Vec, + pub(crate) _marker: PhantomData, } - impl Decode for FromBridgedChainMessagePayload { - fn decode(input: &mut I) -> Result { - // for bridged chain our Calls are opaque - they're encoded to Vec 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::::decode(input)?; - let _skipped_length = Compact::::decode(input)?; - let call = CallOf::>::decode(input)?; - - Ok(FromBridgedChainMessagePayload( - pallet_bridge_call_dispatch::MessagePayload { - spec_version, - weight, - origin, - call, - }, - )) + impl From> for Result>, ()> { + fn from(encoded_call: FromBridgedChainEncodedMessageCall) -> Self { + let mut input = &encoded_call.encoded_call[..]; + let _skipped_length = Compact::::decode(&mut input).map_err(drop)?; + CallOf::>::decode(&mut input).map_err(drop) } } @@ -385,22 +373,14 @@ pub mod target { >::Event: From>, pallet_bridge_call_dispatch::Module: - bp_message_dispatch::MessageDispatch< - (LaneId, MessageNonce), - Message = FromBridgedChainDecodedMessagePayload, - >, + bp_message_dispatch::MessageDispatch<(LaneId, MessageNonce), Message = FromBridgedChainMessagePayload>, { type DispatchPayload = FromBridgedChainMessagePayload; fn dispatch_weight( message: &DispatchMessage>>, ) -> 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>>) { @@ -408,7 +388,7 @@ pub mod target { pallet_bridge_call_dispatch::Module::::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::::decode(&mut &message_on_bridged_chain[..]) .unwrap(); assert_eq!( - message_on_this_chain.0, - target::FromBridgedChainDecodedMessagePayload:: { + message_on_this_chain, + target::FromBridgedChainMessagePayload:: { spec_version: 1, weight: 100, origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, - call: ThisChainCall::Transfer, + call: target::FromBridgedChainEncodedMessageCall:: { + encoded_call: ThisChainCall::Transfer.encode(), + _marker: PhantomData::default(), + }, } ); } diff --git a/bridges/modules/call-dispatch/src/lib.rs b/bridges/modules/call-dispatch/src/lib.rs index d60492be34..b68ff3aa5e 100644 --- a/bridges/modules/call-dispatch/src/lib.rs +++ b/bridges/modules/call-dispatch/src/lib.rs @@ -134,6 +134,16 @@ pub trait Config: frame_system::Config { Origin = ::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>::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), } @@ -174,12 +186,8 @@ decl_module! { } impl, I: Instance> MessageDispatch for Module { - type Message = MessagePayload< - T::SourceChainAccountId, - T::TargetChainAccountPublic, - T::TargetChainSignature, - >::Call, - >; + type Message = + MessagePayload; fn dispatch_weight(message: &Self::Message) -> Weight { message.weight @@ -216,28 +224,6 @@ impl, I: Instance> MessageDispatch for Module { 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, I: Instance> MessageDispatch for Module { 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, I: Instance> MessageDispatch for Module { } }; + // 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: 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); + + impl From for Result { + fn from(call: EncodedCall) -> Result { + 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(>::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::::MessageCallDecodeFailed(bridge, id)), + topics: vec![], + }], + ); + }); + } + #[test] fn should_dispatch_bridge_message_from_root_origin() { new_test_ext().execute_with(|| { diff --git a/bridges/relays/substrate/src/main.rs b/bridges/relays/substrate/src/main.rs index c0f0ab2bca..e0d6149870 100644 --- a/bridges/relays/substrate/src/main.rs +++ b/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, );