diff --git a/bridges/modules/messages/src/benchmarking.rs b/bridges/modules/messages/src/benchmarking.rs index 54cb7c26cd..cab2704f40 100644 --- a/bridges/modules/messages/src/benchmarking.rs +++ b/bridges/modules/messages/src/benchmarking.rs @@ -904,7 +904,7 @@ fn confirm_message_delivery, I: Instance>(nonce: MessageNonce) { }); } assert!(matches!( - outbound_lane.confirm_delivery(nonce, &relayers), + outbound_lane.confirm_delivery(nonce - latest_received_nonce, nonce, &relayers), ReceivalConfirmationResult::ConfirmedMessages(_), )); } diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index c1d4fa223a..a8071e9922 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -197,7 +197,10 @@ decl_error! { /// The message someone is trying to work with (i.e. increase fee) is already-delivered. MessageIsAlreadyDelivered, /// The message someone is trying to work with (i.e. increase fee) is not yet sent. - MessageIsNotYetSent + MessageIsNotYetSent, + /// The number of actually confirmed messages is going to be larger than the number of messages in the proof. + /// This may mean that this or bridged chain storage is corrupted. + TryingToConfirmMoreMessagesThanExpected, } } @@ -647,9 +650,23 @@ decl_module! { let mut lane = outbound_lane::(lane_id); let mut relayers_rewards: RelayersRewards<_, T::OutboundMessageFee> = RelayersRewards::new(); let last_delivered_nonce = lane_data.last_delivered_nonce(); - let confirmed_messages = match lane.confirm_delivery(last_delivered_nonce, &lane_data.relayers) { + let confirmed_messages = match lane.confirm_delivery( + relayers_state.total_messages, + last_delivered_nonce, + &lane_data.relayers, + ) { ReceivalConfirmationResult::ConfirmedMessages(confirmed_messages) => Some(confirmed_messages), ReceivalConfirmationResult::NoNewConfirmations => None, + ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(to_confirm_messages_count) => { + log::trace!( + target: "runtime::bridge-messages", + "Messages delivery proof contains too many messages to confirm: {} vs declared {}", + to_confirm_messages_count, + relayers_state.total_messages, + ); + + fail!(Error::::TryingToConfirmMoreMessagesThanExpected); + }, error => { log::trace!( target: "runtime::bridge-messages", @@ -660,6 +677,7 @@ decl_module! { fail!(Error::::InvalidUnrewardedRelayers); }, }; + if let Some(confirmed_messages) = confirmed_messages { // handle messages delivery confirmation let preliminary_callback_overhead = relayers_state.total_messages.saturating_mul( @@ -2051,4 +2069,33 @@ mod tests { confirm_3_messages_delivery() }); } + + #[test] + fn receive_messages_delivery_proof_rejects_proof_if_trying_to_confirm_more_messages_than_expected() { + run_test(|| { + // send message first to be able to check that delivery_proof fails later + send_regular_message(); + + // 1) InboundLaneData declares that the `last_confirmed_nonce` is 1; + // 2) InboundLaneData has no entries => `InboundLaneData::last_delivered_nonce()` + // returns `last_confirmed_nonce`; + // 3) it means that we're going to confirm delivery of messages 1..=1; + // 4) so the number of declared messages (see `UnrewardedRelayersState`) is `0` and + // numer of actually confirmed messages is `1`. + assert_noop!( + Pallet::::receive_messages_delivery_proof( + Origin::signed(1), + TestMessagesDeliveryProof(Ok(( + TEST_LANE_ID, + InboundLaneData { + last_confirmed_nonce: 1, + relayers: Default::default(), + }, + ))), + UnrewardedRelayersState::default(), + ), + Error::::TryingToConfirmMoreMessagesThanExpected, + ); + }); + } } diff --git a/bridges/modules/messages/src/outbound_lane.rs b/bridges/modules/messages/src/outbound_lane.rs index 44061d984e..7130bd1bb5 100644 --- a/bridges/modules/messages/src/outbound_lane.rs +++ b/bridges/modules/messages/src/outbound_lane.rs @@ -63,6 +63,8 @@ pub enum ReceivalConfirmationResult { /// The unrewarded relayers vec contains entry with mismatched number of dispatch results. May be /// a result of invalid bridged chain storage. InvalidNumberOfDispatchResults, + /// The chain has more messages that need to be confirmed than there is in the proof. + TryingToConfirmMoreMessagesThanExpected(MessageNonce), } /// Outbound messages lane. @@ -98,6 +100,7 @@ impl OutboundLane { /// Confirm messages delivery. pub fn confirm_delivery( &mut self, + max_allowed_messages: MessageNonce, latest_received_nonce: MessageNonce, relayers: &VecDeque>, ) -> ReceivalConfirmationResult { @@ -108,6 +111,16 @@ impl OutboundLane { if latest_received_nonce > data.latest_generated_nonce { return ReceivalConfirmationResult::FailedToConfirmFutureMessages; } + if latest_received_nonce - data.latest_received_nonce > max_allowed_messages { + // that the relayer has declared correct number of messages that the proof contains (it is + // checked outside of the function). But it may happen (but only if this/bridged chain storage is + // corrupted, though) that the actual number of confirmed messages if larger than declared. + // This would mean that 'reward loop' will take more time than the weight formula accounts, + // so we can't allow that. + return ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected( + latest_received_nonce - data.latest_received_nonce, + ); + } let dispatch_results = match extract_dispatch_results(data.latest_received_nonce, latest_received_nonce, relayers) { @@ -245,7 +258,7 @@ mod tests { lane.send_message(message_data(REGULAR_PAYLOAD)); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); - let result = lane.confirm_delivery(latest_received_nonce, relayers); + let result = lane.confirm_delivery(3, latest_received_nonce, relayers); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); result @@ -273,7 +286,7 @@ mod tests { assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( - lane.confirm_delivery(3, &unrewarded_relayers(1..=3)), + lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)), ); assert_eq!(lane.storage.data().latest_generated_nonce, 3); @@ -291,18 +304,18 @@ mod tests { assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( - lane.confirm_delivery(3, &unrewarded_relayers(1..=3)), + lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)), ); assert_eq!( - lane.confirm_delivery(3, &unrewarded_relayers(1..=3)), + lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), ReceivalConfirmationResult::NoNewConfirmations, ); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 3); assert_eq!( - lane.confirm_delivery(2, &unrewarded_relayers(1..=1)), + lane.confirm_delivery(1, 2, &unrewarded_relayers(1..=1)), ReceivalConfirmationResult::NoNewConfirmations, ); assert_eq!(lane.storage.data().latest_generated_nonce, 3); @@ -393,18 +406,40 @@ mod tests { assert_eq!(lane.storage.data().oldest_unpruned_nonce, 1); // after confirmation, some messages are received assert_eq!( - lane.confirm_delivery(2, &unrewarded_relayers(1..=2)), + lane.confirm_delivery(2, 2, &unrewarded_relayers(1..=2)), ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=2)), ); assert_eq!(lane.prune_messages(100), 2); assert_eq!(lane.storage.data().oldest_unpruned_nonce, 3); // after last message is confirmed, everything is pruned assert_eq!( - lane.confirm_delivery(3, &unrewarded_relayers(3..=3)), + lane.confirm_delivery(1, 3, &unrewarded_relayers(3..=3)), ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(3..=3)), ); assert_eq!(lane.prune_messages(100), 1); assert_eq!(lane.storage.data().oldest_unpruned_nonce, 4); }); } + + #[test] + fn confirm_delivery_detects_when_more_than_expected_messages_are_confirmed() { + run_test(|| { + let mut lane = outbound_lane::(TEST_LANE_ID); + lane.send_message(message_data(REGULAR_PAYLOAD)); + lane.send_message(message_data(REGULAR_PAYLOAD)); + lane.send_message(message_data(REGULAR_PAYLOAD)); + assert_eq!( + lane.confirm_delivery(0, 3, &unrewarded_relayers(1..=3)), + ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(3), + ); + assert_eq!( + lane.confirm_delivery(2, 3, &unrewarded_relayers(1..=3)), + ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(3), + ); + assert_eq!( + lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), + ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)), + ); + }); + } }