From a16834871814fe890600516e4a16f57cd8f2e4e2 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 20 Jan 2021 12:11:53 +0300 Subject: [PATCH] Fixed messages count check (#659) * fixed messages count check * explicit check of `messages_count` in the receive_messages_proof * change messages_count to be u32 * Update modules/message-lane/src/lib.rs Co-authored-by: Hernando Castano Co-authored-by: Hernando Castano --- .../bin/millau/runtime/src/rialto_messages.rs | 2 +- .../bin/rialto/runtime/src/millau_messages.rs | 2 +- bridges/bin/runtime-common/src/messages.rs | 53 ++++++++++++++++--- bridges/modules/message-lane/src/lib.rs | 42 +++++++++++++-- bridges/modules/message-lane/src/mock.rs | 2 +- bridges/primitives/message-lane/src/lib.rs | 31 +++++++++-- .../message-lane/src/target_chain.rs | 8 ++- .../src/millau_messages_to_rialto.rs | 2 +- .../src/rialto_messages_to_millau.rs | 2 +- 9 files changed, 121 insertions(+), 23 deletions(-) diff --git a/bridges/bin/millau/runtime/src/rialto_messages.rs b/bridges/bin/millau/runtime/src/rialto_messages.rs index cf3e6511801c..5129276d04f9 100644 --- a/bridges/bin/millau/runtime/src/rialto_messages.rs +++ b/bridges/bin/millau/runtime/src/rialto_messages.rs @@ -192,7 +192,7 @@ impl SourceHeaderChain for Rialto { fn verify_messages_proof( proof: Self::MessagesProof, - messages_count: MessageNonce, + messages_count: u32, ) -> Result>, Self::Error> { messages::target::verify_messages_proof::(proof, messages_count) } diff --git a/bridges/bin/rialto/runtime/src/millau_messages.rs b/bridges/bin/rialto/runtime/src/millau_messages.rs index 85b06c35af34..173bde8cad6e 100644 --- a/bridges/bin/rialto/runtime/src/millau_messages.rs +++ b/bridges/bin/rialto/runtime/src/millau_messages.rs @@ -193,7 +193,7 @@ impl SourceHeaderChain for Millau { fn verify_messages_proof( proof: Self::MessagesProof, - messages_count: MessageNonce, + messages_count: u32, ) -> Result>, Self::Error> { messages::target::verify_messages_proof::(proof, messages_count) } diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index 503dcaeddf77..897de2af6a22 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -396,9 +396,13 @@ pub mod target { } /// Verify proof of Bridged -> This chain messages. + /// + /// The `messages_count` argument verification (sane limits) is supposed to be made + /// outside of this function. This function only verifies that the proof declares exactly + /// `messages_count` messages. pub fn verify_messages_proof( proof: FromBridgedChainMessagesProof, - messages_count: MessageNonce, + messages_count: u32, ) -> Result>>>, &'static str> where ThisRuntime: pallet_substrate_bridge::Config, @@ -487,7 +491,7 @@ pub mod target { /// Verify proof of Bridged -> This chain messages using given message proof parser. pub(crate) fn verify_messages_proof_with_parser( proof: FromBridgedChainMessagesProof, - messages_count: MessageNonce, + messages_count: u32, build_parser: BuildParser, ) -> Result>>>, MessageProofError> where @@ -497,11 +501,18 @@ pub mod target { let (bridged_header_hash, bridged_storage_proof, lane_id, begin, end) = proof; // receiving proofs where end < begin is ok (if proof includes outbound lane state) - // => hence unwrap_or(0) - let messages_in_the_proof = end.checked_sub(begin).and_then(|diff| diff.checked_add(1)).unwrap_or(0); - if messages_in_the_proof != messages_count { - return Err(MessageProofError::MessagesCountMismatch); - } + let messages_in_the_proof = if let Some(nonces_difference) = end.checked_sub(begin) { + // let's check that the user (relayer) has passed correct `messages_count` + // (this bounds maximal capacity of messages vec below) + let messages_in_the_proof = nonces_difference.saturating_add(1); + if messages_in_the_proof != MessageNonce::from(messages_count) { + return Err(MessageProofError::MessagesCountMismatch); + } + + messages_in_the_proof + } else { + 0 + }; let parser = build_parser(bridged_header_hash, bridged_storage_proof)?; @@ -509,7 +520,7 @@ pub mod target { // be in the proof. So any error in `read_value`, or even missing value is fatal. // // Mind that we allow proofs with no messages if outbound lane state is proved. - let mut messages = Vec::with_capacity(end.saturating_sub(begin) as _); + let mut messages = Vec::with_capacity(messages_in_the_proof as _); for nonce in begin..=end { let message_key = MessageKey { lane_id, nonce }; let raw_message_data = parser @@ -1183,4 +1194,30 @@ mod tests { .collect()), ); } + + #[test] + fn verify_messages_proof_with_parser_does_not_panic_if_messages_count_mismatches() { + assert_eq!( + target::verify_messages_proof_with_parser::( + ( + Default::default(), + StorageProof::new(vec![]), + Default::default(), + 0, + u64::MAX + ), + 0, + |_, _| Ok(TestMessageProofParser { + failing: false, + messages: 0..=u64::MAX, + outbound_lane_data: Some(OutboundLaneData { + oldest_unpruned_nonce: 1, + latest_received_nonce: 1, + latest_generated_nonce: 1, + }), + }), + ), + Err(target::MessageProofError::MessagesCountMismatch), + ); + } } diff --git a/bridges/modules/message-lane/src/lib.rs b/bridges/modules/message-lane/src/lib.rs index b667120f101e..b6cf818f5bd9 100644 --- a/bridges/modules/message-lane/src/lib.rs +++ b/bridges/modules/message-lane/src/lib.rs @@ -101,6 +101,10 @@ pub trait Config: frame_system::Config { /// /// There is no point of making this parameter lesser than MaxUnrewardedRelayerEntriesAtInboundLane, /// because then maximal number of relayer entries will be limited by maximal number of messages. + /// + /// This value also represents maximal number of messages in single delivery transaction. Transaction + /// that is declaring more messages than this value, will be rejected. Even if these messages are + /// from different lanes. type MaxUnconfirmedMessagesAtInboundLane: Get; /// Payload type of outbound messages. This payload is dispatched on the bridged chain. @@ -156,6 +160,8 @@ decl_error! { MessageRejectedByLaneVerifier, /// Submitter has failed to pay fee for delivering and dispatching messages. FailedToWithdrawMessageFee, + /// The transaction brings too many messages. + TooManyMessagesInTheProof, /// Invalid messages has been submitted. InvalidMessagesProof, /// Invalid messages dispatch weight has been declared by the relayer. @@ -344,19 +350,25 @@ decl_module! { /// this data in the transaction, so reward confirmations lags should be minimal. #[weight = T::WeightInfo::receive_messages_proof_overhead() .saturating_add(T::WeightInfo::receive_messages_proof_outbound_lane_state_overhead()) - .saturating_add(T::WeightInfo::receive_messages_proof_messages_overhead(*messages_count)) + .saturating_add(T::WeightInfo::receive_messages_proof_messages_overhead(MessageNonce::from(*messages_count))) .saturating_add(*dispatch_weight) ] pub fn receive_messages_proof( origin, relayer_id: T::InboundRelayer, proof: MessagesProofOf, - messages_count: MessageNonce, + messages_count: u32, dispatch_weight: Weight, ) -> DispatchResult { ensure_operational::()?; let _ = ensure_signed(origin)?; + // reject transactions that are declaring too many messages + ensure!( + MessageNonce::from(messages_count) <= T::MaxUnconfirmedMessagesAtInboundLane::get(), + Error::::TooManyMessagesInTheProof + ); + // verify messages proof && convert proof into messages let messages = verify_and_decode_messages_proof::< T::SourceHeaderChain, @@ -457,7 +469,8 @@ decl_module! { // verify that the relayer has declared correct `lane_data::relayers` state // (we only care about total number of entries and messages, because this affects call weight) ensure!( - total_unrewarded_messages(&lane_data.relayers) == relayers_state.total_messages + total_unrewarded_messages(&lane_data.relayers) + .unwrap_or(MessageNonce::MAX) == relayers_state.total_messages && lane_data.relayers.len() as MessageNonce == relayers_state.unrewarded_relayer_entries, Error::::InvalidUnrewardedRelayersState ); @@ -545,7 +558,7 @@ impl, I: Instance> Module { bp_message_lane::UnrewardedRelayersState { unrewarded_relayer_entries: relayers.len() as _, messages_in_oldest_entry: relayers.front().map(|(begin, end, _)| 1 + end - begin).unwrap_or(0), - total_messages: total_unrewarded_messages(&relayers), + total_messages: total_unrewarded_messages(&relayers).unwrap_or(MessageNonce::MAX), } } @@ -733,8 +746,11 @@ impl, I: Instance> OutboundLaneStorage for RuntimeOutboundLaneStora /// Verify messages proof and return proved messages with decoded payload. fn verify_and_decode_messages_proof, Fee, DispatchPayload: Decode>( proof: Chain::MessagesProof, - messages_count: MessageNonce, + messages_count: u32, ) -> Result>, Chain::Error> { + // `receive_messages_proof` weight formula and `MaxUnconfirmedMessagesAtInboundLane` check + // guarantees that the `message_count` is sane and Vec may be allocated. + // (tx with too many messages will either be rejected from the pool, or will fail earlier) Chain::verify_messages_proof(proof, messages_count).map(|messages_by_lane| { messages_by_lane .into_iter() @@ -1073,6 +1089,22 @@ mod tests { }); } + #[test] + fn receive_messages_proof_rejects_proof_with_too_many_messages() { + run_test(|| { + assert_noop!( + Module::::receive_messages_proof( + Origin::signed(1), + TEST_RELAYER_A, + Ok(vec![message(1, REGULAR_PAYLOAD)]).into(), + u32::MAX, + 0, + ), + Error::::TooManyMessagesInTheProof, + ); + }); + } + #[test] fn receive_messages_delivery_proof_works() { run_test(|| { diff --git a/bridges/modules/message-lane/src/mock.rs b/bridges/modules/message-lane/src/mock.rs index 98ae5c6f608d..94f570a84d31 100644 --- a/bridges/modules/message-lane/src/mock.rs +++ b/bridges/modules/message-lane/src/mock.rs @@ -309,7 +309,7 @@ impl SourceHeaderChain for TestSourceHeaderChain { fn verify_messages_proof( proof: Self::MessagesProof, - _messages_count: MessageNonce, + _messages_count: u32, ) -> Result>, Self::Error> { proof .result diff --git a/bridges/primitives/message-lane/src/lib.rs b/bridges/primitives/message-lane/src/lib.rs index 9dba82a71c20..74086cccce84 100644 --- a/bridges/primitives/message-lane/src/lib.rs +++ b/bridges/primitives/message-lane/src/lib.rs @@ -158,11 +158,36 @@ impl Default for OutboundLaneData { } /// Returns total number of messages in the `InboundLaneData::relayers` vector. +/// +/// Returns `None` if there are more messages that `MessageNonce` may fit (i.e. `MessageNonce + 1`). pub fn total_unrewarded_messages( relayers: &VecDeque<(MessageNonce, MessageNonce, RelayerId)>, -) -> MessageNonce { +) -> Option { match (relayers.front(), relayers.back()) { - (Some((begin, _, _)), Some((_, end, _))) => end.checked_sub(*begin).and_then(|d| d.checked_add(1)).unwrap_or(0), - _ => 0, + (Some((begin, _, _)), Some((_, end, _))) => { + if let Some(difference) = end.checked_sub(*begin) { + difference.checked_add(1) + } else { + Some(0) + } + } + _ => Some(0), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn total_unrewarded_messages_does_not_overflow() { + assert_eq!( + total_unrewarded_messages( + &vec![(0, 0, 1), (MessageNonce::MAX, MessageNonce::MAX, 2)] + .into_iter() + .collect() + ), + None, + ); } } diff --git a/bridges/primitives/message-lane/src/target_chain.rs b/bridges/primitives/message-lane/src/target_chain.rs index 04eab55affd8..c79b534156f6 100644 --- a/bridges/primitives/message-lane/src/target_chain.rs +++ b/bridges/primitives/message-lane/src/target_chain.rs @@ -16,7 +16,7 @@ //! Primitives of message lane module, that are used on the target chain. -use crate::{LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData}; +use crate::{LaneId, Message, MessageData, MessageKey, OutboundLaneData}; use codec::{Decode, Encode, Error as CodecError}; use frame_support::{weights::Weight, Parameter, RuntimeDebug}; @@ -72,9 +72,13 @@ pub trait SourceHeaderChain { /// /// Messages vector is required to be sorted by nonce within each lane. Out-of-order /// messages will be rejected. + /// + /// The `messages_count` argument verification (sane limits) is supposed to be made + /// outside of this function. This function only verifies that the proof declares exactly + /// `messages_count` messages. fn verify_messages_proof( proof: Self::MessagesProof, - messages_count: MessageNonce, + messages_count: u32, ) -> Result>, Self::Error>; } diff --git a/bridges/relays/substrate/src/millau_messages_to_rialto.rs b/bridges/relays/substrate/src/millau_messages_to_rialto.rs index b5d1db888e96..1f8088fa7f31 100644 --- a/bridges/relays/substrate/src/millau_messages_to_rialto.rs +++ b/bridges/relays/substrate/src/millau_messages_to_rialto.rs @@ -81,7 +81,7 @@ impl SubstrateMessageLane for MillauMessagesToRialto { let call = rialto_runtime::MessageLaneCall::receive_messages_proof( self.relayer_id_at_source.clone(), proof, - messages_count, + messages_count as _, dispatch_weight, ) .into(); diff --git a/bridges/relays/substrate/src/rialto_messages_to_millau.rs b/bridges/relays/substrate/src/rialto_messages_to_millau.rs index 22e56e312d99..a9ab41b41fe8 100644 --- a/bridges/relays/substrate/src/rialto_messages_to_millau.rs +++ b/bridges/relays/substrate/src/rialto_messages_to_millau.rs @@ -81,7 +81,7 @@ impl SubstrateMessageLane for RialtoMessagesToMillau { let call = millau_runtime::MessageLaneCall::receive_messages_proof( self.relayer_id_at_source.clone(), proof, - messages_count, + messages_count as _, dispatch_weight, ) .into();