Confirm delivery detects when more than expected messages are confirmed (#1039)

* confirm delivery detects when more than expected messages are confirmed

* fix benchmarks compilation

bridges/modules/messages/src/benchmarking.rs

@@ -904,7 +904,7 @@ fn confirm_message_delivery<T: Config<I>, I: Instance>(nonce: MessageNonce) {
 		});
 	}
 	assert!(matches!(
-		outbound_lane.confirm_delivery(nonce, &relayers),
+		outbound_lane.confirm_delivery(nonce - latest_received_nonce, nonce, &relayers),
 		ReceivalConfirmationResult::ConfirmedMessages(_),
 	));
 }

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::<T, I>(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::<T, I>::TryingToConfirmMoreMessagesThanExpected);
+				},
 				error => {
 					log::trace!(
 						target: "runtime::bridge-messages",
@@ -660,6 +677,7 @@ decl_module! {
 					fail!(Error::<T, I>::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::<TestRuntime>::receive_messages_delivery_proof(
+					Origin::signed(1),
+					TestMessagesDeliveryProof(Ok((
+						TEST_LANE_ID,
+						InboundLaneData {
+							last_confirmed_nonce: 1,
+							relayers: Default::default(),
+						},
+					))),
+					UnrewardedRelayersState::default(),
+				),
+				Error::<TestRuntime, DefaultInstance>::TryingToConfirmMoreMessagesThanExpected,
+			);
+		});
+	}
 }

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<S: OutboundLaneStorage> OutboundLane<S> {
 	/// Confirm messages delivery.
 	pub fn confirm_delivery<RelayerId>(
 		&mut self,
+		max_allowed_messages: MessageNonce,
 		latest_received_nonce: MessageNonce,
 		relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
 	) -> ReceivalConfirmationResult {
@@ -108,6 +111,16 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
 		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::<TestRuntime, _>(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)),
+			);
+		});
+	}
 }