Snowbridge V2 Audit Fixes

bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs

@@ -185,7 +185,7 @@ pub mod pallet {
 	/// StorageMap used for encoding a SparseBitmapImpl that tracks whether a specific nonce has
 	/// been processed or not. Message nonces are unique and never repeated.
 	#[pallet::storage]
-	pub type NonceBitmap<T: Config> = StorageMap<_, Twox64Concat, u128, u128, ValueQuery>;
+	pub type NonceBitmap<T: Config> = StorageMap<_, Twox64Concat, u64, u128, ValueQuery>;
 
 	/// The current operating mode of the pallet.
 	#[pallet::storage]
@@ -233,13 +233,17 @@ pub mod pallet {
 			let (nonce, relayer_fee) = (message.nonce, message.relayer_fee);
 
 			// Verify the message has not been processed
-			ensure!(!Nonce::<T>::get(nonce.into()), Error::<T>::InvalidNonce);
+			ensure!(!Nonce::<T>::get(nonce), Error::<T>::InvalidNonce);
 
 			let xcm =
 				T::MessageConverter::convert(message).map_err(|error| Error::<T>::from(error))?;
 
 			// Forward XCM to AH
 			let dest = Location::new(1, [Parachain(T::AssetHubParaId::get())]);
+
+			// Mark message as received
+			Nonce::<T>::set(nonce);
+
 			let message_id =
 				Self::send_xcm(dest.clone(), &relayer, xcm.clone()).map_err(|error| {
 					tracing::error!(target: LOG_TARGET, ?error, ?dest, ?xcm, "XCM send failed with error");
@@ -255,9 +259,6 @@ pub mod pallet {
 				);
 			}
 
-			// Mark message as received
-			Nonce::<T>::set(nonce.into());
-
 			Self::deposit_event(Event::MessageReceived { nonce, message_id });
 
 			Ok(())

bridges/snowbridge/pallets/inbound-queue-v2/src/mock.rs

@@ -101,6 +101,7 @@ parameter_types! {
 	pub UniversalLocation: InteriorLocation =
 		[GlobalConsensus(ByGenesis(WESTEND_GENESIS_HASH)), Parachain(1002)].into();
 	pub AssetHubFromEthereum: Location = Location::new(1,[GlobalConsensus(ByGenesis(WESTEND_GENESIS_HASH)),Parachain(1000)]);
+	pub AssetHubUniversalLocation: InteriorLocation = [GlobalConsensus(ByGenesis(WESTEND_GENESIS_HASH)), Parachain(1000)].into();
 	pub SnowbridgeReward: BridgeReward = BridgeReward::Snowbridge;
 	pub const CreateAssetCall: [u8;2] = [53, 0];
 	pub const CreateAssetDeposit: u128 = 10_000_000_000u128;
@@ -155,6 +156,8 @@ impl inbound_queue_v2::Config for Test {
 		GatewayAddress,
 		UniversalLocation,
 		AssetHubFromEthereum,
+		AssetHubUniversalLocation,
+		AccountId,
 	>;
 	#[cfg(feature = "runtime-benchmarks")]
 	type Helper = Test;

bridges/snowbridge/pallets/outbound-queue-v2/src/benchmarking.rs

@@ -17,6 +17,7 @@ use crate::Pallet as OutboundQueue;
 #[benchmarks(
 	where
 		<T as Config>::MaxMessagePayloadSize: Get<u32>,
+		<T as frame_system::Config>::AccountId: From<[u8; 32]>,
 )]
 mod benchmarks {
 	use super::*;

bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs

@@ -276,13 +276,19 @@ pub mod pallet {
 	}
 
 	#[pallet::call]
-	impl<T: Config> Pallet<T> {
+	impl<T: Config> Pallet<T>
+	where
+		<T as frame_system::Config>::AccountId: From<[u8; 32]>,
+	{
 		#[pallet::call_index(1)]
 		#[pallet::weight(T::WeightInfo::submit_delivery_receipt())]
 		pub fn submit_delivery_receipt(
 			origin: OriginFor<T>,
 			event: Box<EventProof>,
-		) -> DispatchResult {
+		) -> DispatchResult
+		where
+			<T as frame_system::Config>::AccountId: From<[u8; 32]>,
+		{
 			let relayer = ensure_signed(origin)?;
 
 			// submit message to verifier for verification
@@ -315,7 +321,8 @@ pub mod pallet {
 			Self::deposit_event(Event::MessagesCommitted { root, count });
 		}
 
-		/// Process a message delivered by the MessageQueue pallet
+		/// Process a message delivered by the MessageQueue pallet.
+		/// IMPORTANT!! This method does not roll back storage changes on error.
 		pub(crate) fn do_process_message(
 			_: ProcessMessageOriginOf<T>,
 			mut message: &[u8],
@@ -333,8 +340,6 @@ pub mod pallet {
 				return Err(Yield);
 			}
 
-			let nonce = Nonce::<T>::get();
-
 			// Decode bytes into Message
 			let Message { origin, id, fee, commands } =
 				Message::decode(&mut message).map_err(|_| {
@@ -355,6 +360,16 @@ pub mod pallet {
 					payload: command.abi_encode(),
 				})
 				.collect();
+
+			let nonce = <Nonce<T>>::get().checked_add(1).ok_or_else(|| {
+				Self::deposit_event(Event::MessageRejected {
+					id: None,
+					payload: message.to_vec(),
+					error: Unsupported,
+				});
+				Unsupported
+			})?;
+
 			let outbound_message = OutboundMessage {
 				origin,
 				nonce,
@@ -403,14 +418,7 @@ pub mod pallet {
 			};
 			<PendingOrders<T>>::insert(nonce, order);
 
-			Nonce::<T>::set(nonce.checked_add(1).ok_or_else(|| {
-				Self::deposit_event(Event::MessageRejected {
-					id: Some(id),
-					payload: message.to_vec(),
-					error: Unsupported,
-				});
-				Unsupported
-			})?);
+			<Nonce<T>>::set(nonce);
 
 			Self::deposit_event(Event::MessageAccepted { id, nonce });
 
@@ -421,17 +429,30 @@ pub mod pallet {
 		pub fn process_delivery_receipt(
 			relayer: <T as frame_system::Config>::AccountId,
 			receipt: DeliveryReceipt,
-		) -> DispatchResult {
+		) -> DispatchResult
+		where
+			<T as frame_system::Config>::AccountId: From<[u8; 32]>,
+		{
 			// Verify that the message was submitted from the known Gateway contract
 			ensure!(T::GatewayAddress::get() == receipt.gateway, Error::<T>::InvalidGateway);
 
+			let reward_account = if receipt.reward_address == [0u8; 32] {
+				relayer
+			} else {
+				receipt.reward_address.into()
+			};
+
 			let nonce = receipt.nonce;
 
 			let order = <PendingOrders<T>>::get(nonce).ok_or(Error::<T>::InvalidPendingNonce)?;
 
 			if order.fee > 0 {
 				// Pay relayer reward
-				T::RewardPayment::register_reward(&relayer, T::DefaultRewardKind::get(), order.fee);
+				T::RewardPayment::register_reward(
+					&reward_account,
+					T::DefaultRewardKind::get(),
+					order.fee,
+				);
 			}
 
 			<PendingOrders<T>>::remove(nonce);

bridges/snowbridge/pallets/outbound-queue-v2/src/test.rs

@@ -133,15 +133,10 @@ fn process_message_fails_on_max_nonce_reached() {
 
 		match last_event {
 			mock::RuntimeEvent::OutboundQueue(Event::MessageRejected {
-				id: Some(id),
+				id: None,
 				payload: _,
 				error: ProcessMessageError::Unsupported,
-			}) => {
-				assert_eq!(
-					id,
-					hex!("0000000000000000000000000000000000000000000000000000000000000001").into()
-				);
-			},
+			}) => {},
 			_ => {
 				panic!("Expected Event::MessageRejected(Unsupported) but got {:?}", last_event);
 			},

bridges/snowbridge/pallets/system-frontend/src/lib.rs

@@ -54,7 +54,7 @@ pub enum BridgeHubRuntime {
 /// Call indices for dispatchables within `snowbridge-pallet-system-v2`
 #[derive(Encode, Decode, Debug, PartialEq, Clone, TypeInfo)]
 pub enum EthereumSystemCall {
-	#[codec(index = 0)]
+	#[codec(index = 2)]
 	RegisterToken {
 		sender: Box<VersionedLocation>,
 		asset_id: Box<VersionedLocation>,

bridges/snowbridge/pallets/system-v2/src/lib.rs

@@ -126,7 +126,7 @@ pub mod pallet {
 		/// - `impl_address`: The address of the implementation contract.
 		/// - `impl_code_hash`: The codehash of the implementation contract.
 		/// - `initializer`: Optionally call an initializer on the implementation contract.
-		#[pallet::call_index(3)]
+		#[pallet::call_index(0)]
 		#[pallet::weight((<T as pallet::Config>::WeightInfo::upgrade(), DispatchClass::Operational))]
 		pub fn upgrade(
 			origin: OriginFor<T>,
@@ -160,7 +160,7 @@ pub mod pallet {
 		/// Fee required: No
 		///
 		/// - `origin`: Must be `GovernanceOrigin`
-		#[pallet::call_index(4)]
+		#[pallet::call_index(1)]
 		#[pallet::weight((<T as pallet::Config>::WeightInfo::set_operating_mode(), DispatchClass::Operational))]
 		pub fn set_operating_mode(origin: OriginFor<T>, mode: OperatingMode) -> DispatchResult {
 			let origin_location = T::GovernanceOrigin::ensure_origin(origin)?;
@@ -180,8 +180,7 @@ pub mod pallet {
 		/// - `sender`: The original sender initiating the call on AH
 		/// - `asset_id`: Location of the asset (relative to this chain)
 		/// - `metadata`: Metadata to include in the instantiated ERC20 contract on Ethereum
-		/// - `fee`: Ether to pay for the execution cost on Ethereum
-		#[pallet::call_index(0)]
+		#[pallet::call_index(2)]
 		#[pallet::weight(<T as pallet::Config>::WeightInfo::register_token())]
 		pub fn register_token(
 			origin: OriginFor<T>,
@@ -227,14 +226,12 @@ pub mod pallet {
 	impl<T: Config> Pallet<T> {
 		/// Send `command` to the Gateway from a specific origin/agent
 		fn send(origin: H256, command: Command, fee: u128) -> DispatchResult {
-			let mut message = Message {
+			let message = Message {
 				origin,
-				id: Default::default(),
+				id: frame_system::unique((origin, &command, fee)).into(),
 				fee,
 				commands: BoundedVec::try_from(vec![command]).unwrap(),
 			};
-			let hash = sp_io::hashing::blake2_256(&message.encode());
-			message.id = hash.into();
 
 			let ticket = <T as pallet::Config>::OutboundQueue::validate(&message)
 				.map_err(|err| Error::<T>::Send(err))?;