Skip to content

Commit

Permalink
Fixed messages count check (paritytech#659)
Browse files Browse the repository at this point in the history
* 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 <HCastano@users.noreply.github.com>

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
  • Loading branch information
2 people authored and serban300 committed Apr 9, 2024
1 parent d7b536b commit 4457f1f
Show file tree
Hide file tree
Showing 9 changed files with 121 additions and 23 deletions.
2 changes: 1 addition & 1 deletion bridges/bin/millau/runtime/src/rialto_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ impl SourceHeaderChain<bp_rialto::Balance> for Rialto {

fn verify_messages_proof(
proof: Self::MessagesProof,
messages_count: MessageNonce,
messages_count: u32,
) -> Result<ProvedMessages<Message<bp_rialto::Balance>>, Self::Error> {
messages::target::verify_messages_proof::<WithRialtoMessageBridge, Runtime>(proof, messages_count)
}
Expand Down
2 changes: 1 addition & 1 deletion bridges/bin/rialto/runtime/src/millau_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ impl SourceHeaderChain<bp_millau::Balance> for Millau {

fn verify_messages_proof(
proof: Self::MessagesProof,
messages_count: MessageNonce,
messages_count: u32,
) -> Result<ProvedMessages<Message<bp_millau::Balance>>, Self::Error> {
messages::target::verify_messages_proof::<WithMillauMessageBridge, Runtime>(proof, messages_count)
}
Expand Down
53 changes: 45 additions & 8 deletions bridges/bin/runtime-common/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<B: MessageBridge, ThisRuntime>(
proof: FromBridgedChainMessagesProof<B>,
messages_count: MessageNonce,
messages_count: u32,
) -> Result<ProvedMessages<Message<BalanceOf<BridgedChain<B>>>>, &'static str>
where
ThisRuntime: pallet_substrate_bridge::Config,
Expand Down Expand Up @@ -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<B: MessageBridge, BuildParser, Parser>(
proof: FromBridgedChainMessagesProof<B>,
messages_count: MessageNonce,
messages_count: u32,
build_parser: BuildParser,
) -> Result<ProvedMessages<Message<BalanceOf<BridgedChain<B>>>>, MessageProofError>
where
Expand All @@ -497,19 +501,26 @@ 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)?;

// Read messages first. All messages that are claimed to be in the proof must
// 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
Expand Down Expand Up @@ -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::<OnThisChainBridge, _, _>(
(
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),
);
}
}
42 changes: 37 additions & 5 deletions bridges/modules/message-lane/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ pub trait Config<I = DefaultInstance>: 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<MessageNonce>;

/// Payload type of outbound messages. This payload is dispatched on the bridged chain.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<T, I>,
messages_count: MessageNonce,
messages_count: u32,
dispatch_weight: Weight,
) -> DispatchResult {
ensure_operational::<T, I>()?;
let _ = ensure_signed(origin)?;

// reject transactions that are declaring too many messages
ensure!(
MessageNonce::from(messages_count) <= T::MaxUnconfirmedMessagesAtInboundLane::get(),
Error::<T, I>::TooManyMessagesInTheProof
);

// verify messages proof && convert proof into messages
let messages = verify_and_decode_messages_proof::<
T::SourceHeaderChain,
Expand Down Expand Up @@ -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::<T, I>::InvalidUnrewardedRelayersState
);
Expand Down Expand Up @@ -545,7 +558,7 @@ impl<T: Config<I>, I: Instance> Module<T, I> {
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),
}
}

Expand Down Expand Up @@ -733,8 +746,11 @@ impl<T: Config<I>, I: Instance> OutboundLaneStorage for RuntimeOutboundLaneStora
/// Verify messages proof and return proved messages with decoded payload.
fn verify_and_decode_messages_proof<Chain: SourceHeaderChain<Fee>, Fee, DispatchPayload: Decode>(
proof: Chain::MessagesProof,
messages_count: MessageNonce,
messages_count: u32,
) -> Result<ProvedMessages<DispatchMessage<DispatchPayload, Fee>>, Chain::Error> {
// `receive_messages_proof` weight formula and `MaxUnconfirmedMessagesAtInboundLane` check
// guarantees that the `message_count` is sane and Vec<Message> 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()
Expand Down Expand Up @@ -1073,6 +1089,22 @@ mod tests {
});
}

#[test]
fn receive_messages_proof_rejects_proof_with_too_many_messages() {
run_test(|| {
assert_noop!(
Module::<TestRuntime, DefaultInstance>::receive_messages_proof(
Origin::signed(1),
TEST_RELAYER_A,
Ok(vec![message(1, REGULAR_PAYLOAD)]).into(),
u32::MAX,
0,
),
Error::<TestRuntime, DefaultInstance>::TooManyMessagesInTheProof,
);
});
}

#[test]
fn receive_messages_delivery_proof_works() {
run_test(|| {
Expand Down
2 changes: 1 addition & 1 deletion bridges/modules/message-lane/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ impl SourceHeaderChain<TestMessageFee> for TestSourceHeaderChain {

fn verify_messages_proof(
proof: Self::MessagesProof,
_messages_count: MessageNonce,
_messages_count: u32,
) -> Result<ProvedMessages<Message<TestMessageFee>>, Self::Error> {
proof
.result
Expand Down
31 changes: 28 additions & 3 deletions bridges/primitives/message-lane/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<RelayerId>(
relayers: &VecDeque<(MessageNonce, MessageNonce, RelayerId)>,
) -> MessageNonce {
) -> Option<MessageNonce> {
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,
);
}
}
8 changes: 6 additions & 2 deletions bridges/primitives/message-lane/src/target_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -72,9 +72,13 @@ pub trait SourceHeaderChain<Fee> {
///
/// 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<ProvedMessages<Message<Fee>>, Self::Error>;
}

Expand Down
2 changes: 1 addition & 1 deletion bridges/relays/substrate/src/millau_messages_to_rialto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion bridges/relays/substrate/src/rialto_messages_to_millau.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 4457f1f

Please sign in to comment.