Verify that channel exists when validating outbound message (#1067)

* Check existence of channel when validating a message

* Check that channel exists when validating outbound message

* Use Contains rather than StaticLookup

* refactor test utils

* fix runtime tests

* fix more tests

* update polkadot-sdk

---------

Co-authored-by: Alistair Singh <alistair.singh7@gmail.com>

parachain/pallets/outbound-queue/src/lib.rs

@@ -94,7 +94,7 @@ use bridge_hub_common::{AggregateMessageOrigin, CustomDigestItem};
 use codec::Decode;
 use frame_support::{
 	storage::StorageStreamIter,
-	traits::{tokens::Balance, Defensive, EnqueueMessage, Get, ProcessMessageError},
+	traits::{tokens::Balance, Contains, Defensive, EnqueueMessage, Get, ProcessMessageError},
 	weights::{Weight, WeightToFee},
 };
 use snowbridge_core::{
@@ -150,6 +150,9 @@ pub mod pallet {
 		#[pallet::constant]
 		type MaxMessagesPerBlock: Get<u32>;
 
+		/// Check whether a channel exists
+		type Channels: Contains<ChannelId>;
+
 		type PricingParameters: Get<PricingParameters<Self::Balance>>;
 
 		/// Convert a weight value into a deductible fee based.

parachain/pallets/outbound-queue/src/mock.rs

@@ -103,6 +103,7 @@ impl crate::Config for Test {
 	type GasMeter = ConstantGasMeter;
 	type Balance = u128;
 	type PricingParameters = Parameters;
+	type Channels = Everything;
 	type WeightToFee = IdentityFee<u128>;
 	type WeightInfo = ();
 }

parachain/pallets/outbound-queue/src/send_message_impl.rs

@@ -43,12 +43,14 @@ where
 	) -> Result<(Self::Ticket, Fee<<Self as SendMessageFeeProvider>::Balance>), SendError> {
 		// The inner payload should not be too large
 		let payload = message.command.abi_encode();
-
 		ensure!(
 			payload.len() < T::MaxMessagePayloadSize::get() as usize,
 			SendError::MessageTooLarge
 		);
 
+		// Ensure there is a registered channel we can transmit this message on
+		ensure!(T::Channels::contains(&message.channel_id), SendError::InvalidChannel);
+
 		// Generate a unique message id unless one is provided
 		let message_id: H256 = message
 			.id

parachain/pallets/system/src/benchmarking.rs

@@ -126,6 +126,7 @@ mod benchmarks {
 		let origin = T::Helper::make_xcm_origin(origin_location);
 		fund_sovereign_account::<T>(origin_para_id.into())?;
 		SnowbridgeControl::<T>::create_agent(origin.clone())?;
+		SnowbridgeControl::<T>::create_channel(origin.clone(), OperatingMode::Normal)?;
 
 		#[extrinsic_call]
 		_(origin as T::RuntimeOrigin, H160::default(), 1);

parachain/pallets/system/src/lib.rs

@@ -58,7 +58,7 @@ use frame_support::{
 	traits::{
 		fungible::{Inspect, Mutate},
 		tokens::Preservation,
-		EnsureOrigin,
+		Contains, EnsureOrigin,
 	},
 };
 use frame_system::pallet_prelude::*;
@@ -618,18 +618,8 @@ pub mod pallet {
 			Ok(())
 		}
 
-		/// Checks if the pallet has been initialized.
-		pub(crate) fn is_initialized() -> bool {
-			let primary_exists = Channels::<T>::contains_key(PRIMARY_GOVERNANCE_CHANNEL);
-			let secondary_exists = Channels::<T>::contains_key(SECONDARY_GOVERNANCE_CHANNEL);
-			primary_exists && secondary_exists
-		}
-
 		/// Initializes agents and channels.
-		pub(crate) fn initialize(
-			para_id: ParaId,
-			asset_hub_para_id: ParaId,
-		) -> Result<(), DispatchError> {
+		pub fn initialize(para_id: ParaId, asset_hub_para_id: ParaId) -> Result<(), DispatchError> {
 			// Asset Hub
 			let asset_hub_location: MultiLocation =
 				ParentThen(X1(Parachain(asset_hub_para_id.into()))).into();
@@ -660,6 +650,13 @@ pub mod pallet {
 
 			Ok(())
 		}
+
+		/// Checks if the pallet has been initialized.
+		pub(crate) fn is_initialized() -> bool {
+			let primary_exists = Channels::<T>::contains_key(PRIMARY_GOVERNANCE_CHANNEL);
+			let secondary_exists = Channels::<T>::contains_key(SECONDARY_GOVERNANCE_CHANNEL);
+			primary_exists && secondary_exists
+		}
 	}
 
 	impl<T: Config> StaticLookup for Pallet<T> {
@@ -670,6 +667,12 @@ pub mod pallet {
 		}
 	}
 
+	impl<T: Config> Contains<ChannelId> for Pallet<T> {
+		fn contains(channel_id: &ChannelId) -> bool {
+			Channels::<T>::get(channel_id).is_some()
+		}
+	}
+
 	impl<T: Config> Get<PricingParametersOf<T>> for Pallet<T> {
 		fn get() -> PricingParametersOf<T> {
 			PricingParameters::<T>::get()

parachain/pallets/system/src/mock.rs

@@ -178,6 +178,7 @@ impl snowbridge_outbound_queue::Config for Test {
 	type GasMeter = ConstantGasMeter;
 	type Balance = u128;
 	type PricingParameters = EthereumSystem;
+	type Channels = EthereumSystem;
 	type WeightToFee = IdentityFee<u128>;
 	type WeightInfo = ();
 }

parachain/pallets/system/src/tests.rs

@@ -314,8 +314,8 @@ fn update_channel() {
 
 		// First create the channel
 		let _ = Balances::mint_into(&sovereign_account, 10000);
-		EthereumSystem::create_agent(origin.clone()).unwrap();
-		EthereumSystem::create_channel(origin.clone(), OperatingMode::Normal).unwrap();
+		assert_ok!(EthereumSystem::create_agent(origin.clone()));
+		assert_ok!(EthereumSystem::create_channel(origin.clone(), OperatingMode::Normal));
 
 		// Now try to update it
 		assert_ok!(EthereumSystem::update_channel(origin, OperatingMode::Normal));
@@ -405,8 +405,8 @@ fn force_update_channel() {
 
 		// First create the channel
 		let _ = Balances::mint_into(&sovereign_account, 10000);
-		EthereumSystem::create_agent(origin.clone()).unwrap();
-		EthereumSystem::create_channel(origin.clone(), OperatingMode::Normal).unwrap();
+		assert_ok!(EthereumSystem::create_agent(origin.clone()));
+		assert_ok!(EthereumSystem::create_channel(origin.clone(), OperatingMode::Normal));
 
 		// Now try to force update it
 		let force_origin = RuntimeOrigin::root();
@@ -444,11 +444,13 @@ fn force_update_channel_bad_origin() {
 fn transfer_native_from_agent() {
 	new_test_ext(true).execute_with(|| {
 		let origin_location = MultiLocation { parents: 1, interior: X1(Parachain(2000)) };
+		let origin = make_xcm_origin(origin_location);
 		let recipient: H160 = [27u8; 20].into();
 		let amount = 103435;
 
-		// First create the agent
-		Agents::<Test>::insert(make_agent_id(origin_location), ());
+		// First create the agent and channel
+		assert_ok!(EthereumSystem::create_agent(origin.clone()));
+		assert_ok!(EthereumSystem::create_channel(origin, OperatingMode::Normal));
 
 		let origin = make_xcm_origin(origin_location);
 		assert_ok!(EthereumSystem::transfer_native_from_agent(origin, recipient, amount),);
@@ -549,23 +551,28 @@ fn charge_fee_for_create_agent() {
 		let sovereign_account = sibling_sovereign_account::<Test>(para_id.into());
 		let (_, agent_id) = ensure_sibling::<Test>(&origin_location).unwrap();
 
+		let initial_sovereign_balance = Balances::balance(&sovereign_account);
 		assert_ok!(EthereumSystem::create_agent(origin.clone()));
+		let fee_charged = initial_sovereign_balance - Balances::balance(&sovereign_account);
+
+		assert_ok!(EthereumSystem::create_channel(origin, OperatingMode::Normal));
+
 		// assert sovereign_balance decreased by (fee.base_fee + fee.delivery_fee)
 		let message = Message {
 			id: None,
 			channel_id: ParaId::from(para_id).into(),
 			command: Command::CreateAgent { agent_id },
 		};
 		let (_, fee) = OutboundQueue::validate(&message).unwrap();
-		let sovereign_balance = Balances::balance(&sovereign_account);
-		assert_eq!(sovereign_balance + fee.local + fee.remote, InitialFunding::get());
+		assert_eq!(fee.local + fee.remote, fee_charged);
 
 		// and treasury_balance increased
 		let treasury_balance = Balances::balance(&TreasuryAccount::get());
 		assert!(treasury_balance > InitialFunding::get());
 
+		let final_sovereign_balance = Balances::balance(&sovereign_account);
 		// (sovereign_balance + treasury_balance) keeps the same
-		assert_eq!(sovereign_balance + treasury_balance, { InitialFunding::get() * 2 });
+		assert_eq!(final_sovereign_balance + treasury_balance, { InitialFunding::get() * 2 });
 	});
 }
 

parachain/runtime/tests/src/lib.rs

@@ -8,6 +8,7 @@ mod test_cases;
 use asset_hub_rococo_runtime::xcm_config::bridging::to_ethereum::DefaultBridgeHubEthereumBaseFee;
 use bridge_hub_rococo_runtime::{xcm_config::XcmConfig, Runtime, RuntimeEvent, SessionKeys};
 use codec::Decode;
+use cumulus_primitives_core::XcmError::{FailedToTransactAsset, NotHoldingFees};
 use parachains_common::{AccountId, AuraId};
 use sp_core::H160;
 use sp_keyring::AccountKeyring::Alice;
@@ -51,25 +52,29 @@ pub fn unpaid_transfer_token_to_ethereum_fails_with_barrier() {
 
 #[test]
 pub fn transfer_token_to_ethereum_fee_not_enough() {
-	test_cases::send_transfer_token_message_fee_not_enough::<Runtime, XcmConfig>(
+	test_cases::send_transfer_token_message_failure::<Runtime, XcmConfig>(
 		collator_session_keys(),
 		1013,
 		1000,
+		DefaultBridgeHubEthereumBaseFee::get() + 1_000_000_000,
 		H160::random(),
 		H160::random(),
 		// fee not enough
 		1_000_000_000,
+		NotHoldingFees,
 	)
 }
 
 #[test]
 pub fn transfer_token_to_ethereum_insufficient_fund() {
-	test_cases::send_transfer_token_message_insufficient_fund::<Runtime, XcmConfig>(
+	test_cases::send_transfer_token_message_failure::<Runtime, XcmConfig>(
 		collator_session_keys(),
 		1013,
 		1000,
+		1_000_000_000,
 		H160::random(),
 		H160::random(),
 		DefaultBridgeHubEthereumBaseFee::get(),
+		FailedToTransactAsset("InsufficientBalance"),
 	)
 }

parachain/runtime/tests/src/test_cases.rs

@@ -4,6 +4,7 @@
 //! Module contains predefined test-case scenarios for `Runtime` with bridging capabilities.
 
 use asset_hub_rococo_runtime::xcm_config::bridging::to_ethereum::DefaultBridgeHubEthereumBaseFee;
+use bridge_hub_rococo_runtime::EthereumSystem;
 use codec::Encode;
 use frame_support::{assert_err, assert_ok, traits::fungible::Mutate};
 use parachains_runtimes_test_utils::{
@@ -15,7 +16,7 @@ use xcm::latest::prelude::*;
 use xcm_executor::XcmExecutor;
 // Re-export test_case from `parachains-runtimes-test-utils`
 pub use parachains_runtimes_test_utils::test_cases::change_storage_constant_by_governance_works;
-use xcm::v3::Error::{Barrier, FailedToTransactAsset, NotHoldingFees};
+use xcm::v3::Error::{self, Barrier};
 
 type RuntimeHelper<Runtime, AllPalletsWithoutSystem = ()> =
 	parachains_runtimes_test_utils::RuntimeHelper<Runtime, AllPalletsWithoutSystem>;
@@ -119,7 +120,8 @@ pub fn send_transfer_token_message_success<Runtime, XcmConfig>(
 		+ parachain_info::Config
 		+ pallet_collator_selection::Config
 		+ cumulus_pallet_parachain_system::Config
-		+ snowbridge_outbound_queue::Config,
+		+ snowbridge_outbound_queue::Config
+		+ snowbridge_system::Config,
 	XcmConfig: xcm_executor::Config,
 	ValidatorIdOf<Runtime>: From<AccountIdOf<Runtime>>,
 {
@@ -130,6 +132,9 @@ pub fn send_transfer_token_message_success<Runtime, XcmConfig>(
 		.with_tracing()
 		.build()
 		.execute_with(|| {
+			EthereumSystem::initialize(runtime_para_id.into(), assethub_parachain_id.into())
+				.unwrap();
+
 			// fund asset hub sovereign account enough so it can pay fees
 			initial_fund::<Runtime>(
 				assethub_parachain_id,
@@ -240,13 +245,15 @@ pub fn send_unpaid_transfer_token_message<Runtime, XcmConfig>(
 		});
 }
 
-pub fn send_transfer_token_message_fee_not_enough<Runtime, XcmConfig>(
+pub fn send_transfer_token_message_failure<Runtime, XcmConfig>(
 	collator_session_key: CollatorSessionKeys<Runtime>,
 	runtime_para_id: u32,
 	assethub_parachain_id: u32,
+	initial_amount: u128,
 	weth_contract_address: H160,
 	destination_address: H160,
 	fee_amount: u128,
+	expected_error: Error,
 ) where
 	Runtime: frame_system::Config
 		+ pallet_balances::Config
@@ -255,7 +262,8 @@ pub fn send_transfer_token_message_fee_not_enough<Runtime, XcmConfig>(
 		+ parachain_info::Config
 		+ pallet_collator_selection::Config
 		+ cumulus_pallet_parachain_system::Config
-		+ snowbridge_outbound_queue::Config,
+		+ snowbridge_outbound_queue::Config
+		+ snowbridge_system::Config,
 	XcmConfig: xcm_executor::Config,
 	ValidatorIdOf<Runtime>: From<AccountIdOf<Runtime>>,
 {
@@ -266,11 +274,11 @@ pub fn send_transfer_token_message_fee_not_enough<Runtime, XcmConfig>(
 		.with_tracing()
 		.build()
 		.execute_with(|| {
+			EthereumSystem::initialize(runtime_para_id.into(), assethub_parachain_id.into())
+				.unwrap();
+
 			// fund asset hub sovereign account enough so it can pay fees
-			initial_fund::<Runtime>(
-				assethub_parachain_id,
-				DefaultBridgeHubEthereumBaseFee::get() + 1_000_000_000,
-			);
+			initial_fund::<Runtime>(assethub_parachain_id, initial_amount);
 
 			let outcome = send_transfer_token_message::<Runtime, XcmConfig>(
 				assethub_parachain_id,
@@ -279,46 +287,6 @@ pub fn send_transfer_token_message_fee_not_enough<Runtime, XcmConfig>(
 				fee_amount,
 			);
 			// check err is NotHoldingFees
-			assert_err!(outcome.ensure_complete(), NotHoldingFees);
-		});
-}
-
-pub fn send_transfer_token_message_insufficient_fund<Runtime, XcmConfig>(
-	collator_session_key: CollatorSessionKeys<Runtime>,
-	runtime_para_id: u32,
-	assethub_parachain_id: u32,
-	weth_contract_address: H160,
-	destination_address: H160,
-	fee_amount: u128,
-) where
-	Runtime: frame_system::Config
-		+ pallet_balances::Config
-		+ pallet_session::Config
-		+ pallet_xcm::Config
-		+ parachain_info::Config
-		+ pallet_collator_selection::Config
-		+ cumulus_pallet_parachain_system::Config
-		+ snowbridge_outbound_queue::Config,
-	XcmConfig: xcm_executor::Config,
-	ValidatorIdOf<Runtime>: From<AccountIdOf<Runtime>>,
-{
-	ExtBuilder::<Runtime>::default()
-		.with_collators(collator_session_key.collators())
-		.with_session_keys(collator_session_key.session_keys())
-		.with_para_id(runtime_para_id.into())
-		.with_tracing()
-		.build()
-		.execute_with(|| {
-			// initial fund not enough
-			initial_fund::<Runtime>(assethub_parachain_id, 1_000_000_000);
-
-			let outcome = send_transfer_token_message::<Runtime, XcmConfig>(
-				assethub_parachain_id,
-				weth_contract_address,
-				destination_address,
-				fee_amount,
-			);
-			// check err is FailedToTransactAsset("InsufficientBalance")
-			assert_err!(outcome.ensure_complete(), FailedToTransactAsset("InsufficientBalance"));
+			assert_err!(outcome.ensure_complete(), expected_error);
 		});
 }