Skip to content

Audit Issue 25: Use ExternalConsensusLocationsConverterFor in Inbound Queue v2 #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
claravanstaden merged 2 commits into from
Apr 14, 2025
Merged

Audit Issue 25: Use ExternalConsensusLocationsConverterFor in Inbound Queue v2 #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7d2835
use ExternalConsensusLocationsConverterFor instead of EthereumLocatio…
claravanstaden Apr 9, 2025
fe67940
use AccountId
claravanstaden Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 49 additions & 20 deletions bridges/snowbridge/primitives/inbound-queue/src/v2/converter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! Converts messages from Solidity ABI-encoding to XCM

use super::{message::*, traits::*};
use crate::{v2::LOG_TARGET, CallIndex, EthereumLocationsConverterFor};
use crate::{v2::LOG_TARGET, CallIndex};
use codec::{Decode, DecodeLimit, Encode};
use core::marker::PhantomData;
use frame_support::ensure;
Expand All @@ -16,6 +16,8 @@ use xcm::{
prelude::{Junction::*, *},
MAX_XCM_DECODE_DEPTH,
};
use xcm_builder::ExternalConsensusLocationsConverterFor;
use xcm_executor::traits::ConvertLocation;

const MINIMUM_DEPOSIT: u128 = 1;

Expand Down Expand Up @@ -55,6 +57,8 @@ pub struct MessageToXcm<
GatewayProxyAddress,
EthereumUniversalLocation,
GlobalAssetHubLocation,
AssetHubUniversalLocation,
AccountId,
Copy link
Copy Markdown

@vgeddes vgeddes Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unused since you're configuring ExternalConsensusLocationsConverterFor to directly convert to [u8; 32].

Suggested change
AccountId,

Copy link
Copy Markdown
Owner Author

@claravanstaden claravanstaden Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to use it, added in fe67940.

> {
_phantom: PhantomData<(
CreateAssetCall,
Expand All @@ -65,6 +69,8 @@ pub struct MessageToXcm<
GatewayProxyAddress,
EthereumUniversalLocation,
GlobalAssetHubLocation,
AssetHubUniversalLocation,
AccountId,
)>,
}

Expand All @@ -77,6 +83,8 @@ impl<
GatewayProxyAddress,
EthereumUniversalLocation,
GlobalAssetHubLocation,
AssetHubUniversalLocation,
AccountId,
>
MessageToXcm<
CreateAssetCall,
Expand All @@ -87,6 +95,8 @@ impl<
GatewayProxyAddress,
EthereumUniversalLocation,
GlobalAssetHubLocation,
AssetHubUniversalLocation,
AccountId,
>
where
CreateAssetCall: Get<CallIndex>,
Expand All @@ -97,6 +107,8 @@ where
GatewayProxyAddress: Get<H160>,
EthereumUniversalLocation: Get<InteriorLocation>,
GlobalAssetHubLocation: Get<Location>,
AssetHubUniversalLocation: Get<InteriorLocation>,
AccountId: Into<[u8; 32]> + From<[u8; 32]> + Clone,
{
/// Parse the message into an intermediate form, with all fields decoded
/// and prepared.
Expand All @@ -110,7 +122,7 @@ where
// Get the claimer from the message,
.and_then(|claimer_bytes| Location::decode(&mut claimer_bytes.as_ref()).ok())
// or use the Snowbridge sovereign on AH as the fallback claimer.
.unwrap_or_else(|| Location::new(0, [AccountId32 { network: None, id: bridge_owner }]));
.unwrap_or_else(|| Location::new(0, [AccountId32 { network: None, id: bridge_owner.clone().into() }]));

let mut remote_xcm: Xcm<()> = match &message.xcm {
XcmPayload::Raw(raw) => Self::decode_raw_xcm(raw),
Expand Down Expand Up @@ -182,12 +194,14 @@ where

/// Get the bridge owner account ID from the current Ethereum network chain ID.
/// Returns an error if the network is not Ethereum.
fn bridge_owner() -> Result<[u8; 32], ConvertMessageError> {
let chain_id = match EthereumNetwork::get() {
NetworkId::Ethereum { chain_id } => chain_id,
_ => return Err(ConvertMessageError::InvalidNetwork),
};
Ok(EthereumLocationsConverterFor::<[u8; 32]>::from_chain_id(&chain_id))
fn bridge_owner() -> Result<AccountId, ConvertMessageError> {
let account =
ExternalConsensusLocationsConverterFor::<AssetHubUniversalLocation, AccountId>::convert_location(
&Location::new(2, [GlobalConsensus(EthereumNetwork::get())]),
)
.ok_or(ConvertMessageError::CannotReanchor)?;

Ok(account)
}

/// Construct the remote XCM needed to create a new asset in the `ForeignAssets` pallet
Expand All @@ -196,7 +210,7 @@ where
token: &H160,
network: super::message::Network,
eth_value: u128,
bridge_owner: [u8; 32],
bridge_owner: AccountId,
claimer: Location,
) -> Result<Xcm<()>, ConvertMessageError> {
let dot_asset = Location::new(1, Here);
Expand Down Expand Up @@ -231,11 +245,12 @@ where
fn make_create_asset_xcm_for_polkadot(
create_call_index: [u8; 2],
asset_id: Location,
bridge_owner: [u8; 32],
bridge_owner: AccountId,
dot_fee_asset: xcm::prelude::Asset,
eth_asset: xcm::prelude::Asset,
claimer: Location,
) -> Xcm<()> {
let bridge_owner_bytes: [u8; 32] = bridge_owner.into();
vec![
// Exchange eth for dot to pay the asset creation deposit.
ExchangeAsset {
Expand All @@ -245,15 +260,15 @@ where
},
// Deposit the dot deposit into the bridge sovereign account (where the asset
// creation fee will be deducted from).
DepositAsset { assets: dot_fee_asset.clone().into(), beneficiary: bridge_owner.into() },
DepositAsset { assets: dot_fee_asset.clone().into(), beneficiary: bridge_owner_bytes.into() },
// Call to create the asset.
Transact {
origin_kind: OriginKind::Xcm,
fallback_max_weight: None,
call: (
create_call_index,
asset_id.clone(),
MultiAddress::<[u8; 32], ()>::Id(bridge_owner.into()),
MultiAddress::<[u8; 32], ()>::Id(bridge_owner_bytes.into()),
MINIMUM_DEPOSIT,
)
.encode()
Expand Down Expand Up @@ -293,6 +308,8 @@ impl<
GatewayProxyAddress,
EthereumUniversalLocation,
GlobalAssetHubLocation,
AssetHubUniversalLocation,
AccountId,
> ConvertMessage
for MessageToXcm<
CreateAssetCall,
Expand All @@ -303,6 +320,8 @@ impl<
GatewayProxyAddress,
EthereumUniversalLocation,
GlobalAssetHubLocation,
AssetHubUniversalLocation,
AccountId,
>
where
CreateAssetCall: Get<CallIndex>,
Expand All @@ -313,6 +332,8 @@ where
GatewayProxyAddress: Get<H160>,
EthereumUniversalLocation: Get<InteriorLocation>,
GlobalAssetHubLocation: Get<Location>,
AssetHubUniversalLocation: Get<InteriorLocation>,
AccountId: Into<[u8; 32]> + From<[u8; 32]> + Clone,
{
fn convert(message: Message) -> Result<Xcm<()>, ConvertMessageError> {
let message = Self::prepare(message)?;
Expand Down Expand Up @@ -385,9 +406,10 @@ mod tests {
pub const EthereumNetwork: xcm::v5::NetworkId = xcm::v5::NetworkId::Ethereum { chain_id: 11155111 };
pub const GatewayAddress: H160 = H160(GATEWAY_ADDRESS);
pub InboundQueueLocation: InteriorLocation = [PalletInstance(84)].into();
pub UniversalLocation: InteriorLocation =
pub EthereumUniversalLocation: 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 const CreateAssetCall: [u8;2] = [53, 0];
pub const CreateAssetDeposit: u128 = 10_000_000_000u128;
}
Expand Down Expand Up @@ -419,8 +441,10 @@ mod tests {
InboundQueueLocation,
MockTokenIdConvert,
GatewayAddress,
UniversalLocation,
EthereumUniversalLocation,
AssetHubFromEthereum,
AssetHubUniversalLocation,
[u8; 32],
>;

type ConverterFailing = MessageToXcm<
Expand All @@ -430,8 +454,10 @@ mod tests {
InboundQueueLocation,
MockFailedTokenConvert,
GatewayAddress,
UniversalLocation,
EthereumUniversalLocation,
AssetHubFromEthereum,
AssetHubUniversalLocation,
[u8; 32],
>;

#[test]
Expand Down Expand Up @@ -725,11 +751,14 @@ mod tests {
}

// actual claimer should default to Snowbridge sovereign account
let chain_id = match EthereumNetwork::get() {
NetworkId::Ethereum { chain_id } => chain_id,
_ => 0,
};
let bridge_owner = EthereumLocationsConverterFor::<[u8; 32]>::from_chain_id(&chain_id);
let bridge_owner = ExternalConsensusLocationsConverterFor::<
AssetHubUniversalLocation,
[u8; 32],
>::convert_location(&Location::new(
2,
[GlobalConsensus(EthereumNetwork::get())],
))
.unwrap();
assert_eq!(
actual_claimer,
Some(Location::new(0, [AccountId32 { network: None, id: bridge_owner }]))
Expand Down
3 changes: 3 additions & 0 deletions cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/bridge_to_ethereum_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ parameter_types! {
multiplier: FixedU128::from_rational(1, 1),
};
pub AssetHubFromEthereum: Location = Location::new(1, [GlobalConsensus(RelayNetwork::get()), Parachain(ASSET_HUB_ID)]);
pub AssetHubUniversalLocation: InteriorLocation = [GlobalConsensus(RelayNetwork::get()), Parachain(ASSET_HUB_ID)].into();
pub AssetHubLocation: Location = Location::new(1, [Parachain(ASSET_HUB_ID)]);
pub EthereumUniversalLocation: InteriorLocation = [GlobalConsensus(EthereumNetwork::get())].into();
pub InboundQueueV2Location: InteriorLocation = [PalletInstance(INBOUND_QUEUE_PALLET_INDEX_V2)].into();
Expand Down Expand Up @@ -144,6 +145,8 @@ impl snowbridge_pallet_inbound_queue_v2::Config for Runtime {
EthereumGatewayAddress,
EthereumUniversalLocation,
AssetHubFromEthereum,
AssetHubUniversalLocation,
AccountId,
>;
type AccountToLocation = xcm_builder::AliasesIntoAccountId32<
xcm_config::RelayNetwork,
Expand Down
Loading