Skip to content
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.

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

[Snowbridge]: Ensure source always from AH for exported message #6838

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
8a88a8b
Ensure source always from AH
yrong Dec 11, 2024
53ee95e
More tests for edge cases
yrong Dec 11, 2024
525e6a6
Update bridges/snowbridge/primitives/router/src/outbound/mod.rs
yrong Dec 13, 2024
c3b8b88
Update bridges/snowbridge/primitives/router/src/outbound/mod.rs
yrong Dec 13, 2024
a7cb93d
Update bridges/snowbridge/primitives/router/src/outbound/mod.rs
yrong Dec 13, 2024
277d51b
Load AssetHub's ParaId from runtime
yrong Dec 13, 2024
c2a8ba9
More tests
yrong Dec 13, 2024
7192a5a
Add prdoc
yrong Dec 13, 2024
f7b6f3a
Merge branch 'master' into check-souce-from-ah
yrong Dec 13, 2024
62f629b
Fix fmt
yrong Dec 13, 2024
eff2ef2
Merge branch 'check-souce-from-ah' of https://github.com/yrong/polkad…
yrong Dec 13, 2024
f84cc0a
Merge branch 'master' into check-souce-from-ah
yrong Dec 13, 2024
df1e673
Fix the comment
yrong Dec 14, 2024
11f8d57
Merge branch 'master' into check-souce-from-ah
yrong Dec 18, 2024
2bf5bb1
Rename as WhitelistedParaId
yrong Jan 7, 2025
73961f4
Merge branch 'check-souce-from-ah' of https://github.com/yrong/polkad…
yrong Jan 7, 2025
b5d4369
Merge branch 'master' into check-souce-from-ah
yrong Jan 7, 2025
0a51fec
Revert change
yrong Jan 13, 2025
e2e0228
Barrier DenyFirstExportMessageFrom
yrong Jan 13, 2025
7102219
Merge branch 'master' into check-souce-from-ah
yrong Jan 13, 2025
7f529ed
Rename
yrong Jan 14, 2025
ccf9a28
Fix xcm config
yrong Jan 14, 2025
5226eed
Cleanup
yrong Jan 14, 2025
e779308
Update prdoc/pr_6838.prdoc
franciscoaguirre Jan 14, 2025
3a66cdb
More tests
yrong Jan 14, 2025
5f9ea7f
Merge branch 'check-souce-from-ah' of https://github.com/yrong/polkad…
yrong Jan 14, 2025
15ae2b3
Update cumulus/parachains/integration-tests/emulated/tests/bridges/br…
yrong Jan 14, 2025
e792f7d
Cleanup
yrong Jan 14, 2025
d0027ea
Update bridges/snowbridge/primitives/router/src/outbound/barriers.rs
yrong Jan 14, 2025
fcadbd7
Update bridges/snowbridge/primitives/router/src/outbound/barriers.rs
yrong Jan 14, 2025
4b51b60
Update bridges/snowbridge/primitives/router/src/outbound/barriers.rs
yrong Jan 14, 2025
6701841
Update bridges/snowbridge/primitives/router/src/outbound/barriers.rs
yrong Jan 14, 2025
efa5114
Fix format
yrong Jan 14, 2025
f53964b
Merge branch 'master' into check-souce-from-ah
yrong Jan 14, 2025
421b4ef
Fix clippy
yrong Jan 14, 2025
ef72cdf
Merge branch 'master' into check-souce-from-ah
yrong Jan 14, 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions bridges/snowbridge/primitives/router/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ sp-runtime = { workspace = true }
sp-std = { workspace = true }

xcm = { workspace = true }
xcm-builder = { workspace = true }
xcm-executor = { workspace = true }

snowbridge-core = { workspace = true }
Expand All @@ -43,13 +44,15 @@ std = [
"sp-io/std",
"sp-runtime/std",
"sp-std/std",
"xcm-builder/std",
"xcm-executor/std",
"xcm/std",
]
runtime-benchmarks = [
"frame-support/runtime-benchmarks",
"snowbridge-core/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
"xcm-builder/runtime-benchmarks",
"xcm-executor/runtime-benchmarks",
"xcm/runtime-benchmarks",
]
126 changes: 126 additions & 0 deletions bridges/snowbridge/primitives/router/src/outbound/barriers.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// SPDX-License-Identifier: Apache-2.0
// SPDX-FileCopyrightText: 2023 Snowfork <[email protected]>

use core::{marker::PhantomData, ops::ControlFlow};
use frame_support::traits::{Contains, ProcessMessageError};
use xcm::prelude::{ExportMessage, Instruction, Location, NetworkId, Weight};
use xcm_builder::{CreateMatcher, MatchXcm};
use xcm_executor::traits::{Properties, ShouldExecute};

pub struct DenyFirstExportMessageFrom<FromOrigin, ToGlobalConsensus>(
PhantomData<(FromOrigin, ToGlobalConsensus)>,
);

impl<FromOrigin, ToGlobalConsensus> ShouldExecute
for DenyFirstExportMessageFrom<FromOrigin, ToGlobalConsensus>
where
FromOrigin: Contains<Location>,
ToGlobalConsensus: Contains<NetworkId>,
{
fn should_execute<RuntimeCall>(
origin: &Location,
message: &mut [Instruction<RuntimeCall>],
_max_weight: Weight,
_properties: &mut Properties,
) -> Result<(), ProcessMessageError> {
message.matcher().match_next_inst_while(
|_| true,
|inst| match inst {
ExportMessage { network, .. } =>
if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) {
return Err(ProcessMessageError::Unsupported)
} else {
Ok(ControlFlow::Continue(()))
},
_ => Ok(ControlFlow::Continue(())),
},
)?;
Ok(())
}
}

#[cfg(test)]
mod tests {
use super::*;
use frame_support::{
assert_err, parameter_types,
traits::{Equals, Everything, EverythingBut},
};
use xcm::prelude::*;
use xcm_builder::{DenyReserveTransferToRelayChain, DenyThenTry, TakeWeightCredit};

parameter_types! {
pub AssetHubLocation: Location = Location::new(1, Parachain(1000));
pub EthereumNetwork: NetworkId = Ethereum { chain_id: 1 };
}

#[test]
fn deny_export_message_from_source() {
yrong marked this conversation as resolved.
Show resolved Hide resolved
let mut xcm: Vec<Instruction<()>> =
vec![ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() }];
yrong marked this conversation as resolved.
Show resolved Hide resolved

let result = DenyFirstExportMessageFrom::<
EverythingBut<Equals<AssetHubLocation>>,
Everything,
>::should_execute(
&Location::parent(),
&mut xcm,
Weight::zero(),
&mut Properties { weight_credit: Weight::zero(), message_id: None },
);
assert_err!(result, ProcessMessageError::Unsupported);
}

#[test]
fn allow_export_message_from_source() {
yrong marked this conversation as resolved.
Show resolved Hide resolved
let mut xcm: Vec<Instruction<()>> =
vec![ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() }];
yrong marked this conversation as resolved.
Show resolved Hide resolved

let result = DenyFirstExportMessageFrom::<
EverythingBut<Equals<AssetHubLocation>>,
Everything,
>::should_execute(
&AssetHubLocation::get(),
&mut xcm,
Weight::zero(),
&mut Properties { weight_credit: Weight::zero(), message_id: None },
);
assert!(result.is_ok());
}

#[test]
fn allow_xcm_without_export_message() {
let mut xcm: Vec<Instruction<()>> = vec![ClearOrigin];

let result = DenyFirstExportMessageFrom::<Everything, Everything>::should_execute(
&Location::parent(),
&mut xcm,
Weight::zero(),
&mut Properties { weight_credit: Weight::zero(), message_id: None },
);
assert!(result.is_ok());
}

#[test]
fn deny_with_reserve_transfer_to_relay_chain() {
let mut xcm: Vec<Instruction<()>> = vec![DepositReserveAsset {
assets: Wild(All),
dest: Location { parents: 1, interior: Here },
xcm: Default::default(),
}];

let result = DenyThenTry::<
DenyFirstExportMessageFrom<
EverythingBut<Equals<AssetHubLocation>>,
Equals<EthereumNetwork>,
>,
DenyThenTry<DenyReserveTransferToRelayChain, TakeWeightCredit>,
>::should_execute(
&AssetHubLocation::get(),
&mut xcm,
Weight::zero(),
&mut Properties { weight_credit: Weight::zero(), message_id: None },
);
assert_err!(result, ProcessMessageError::Unsupported);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please, do not test other barriers here:

Suggested change
#[test]
fn deny_with_reserve_transfer_to_relay_chain() {
let mut xcm: Vec<Instruction<()>> = vec![DepositReserveAsset {
assets: Wild(All),
dest: Location { parents: 1, interior: Here },
xcm: Default::default(),
}];
let result = DenyThenTry::<
DenyFirstExportMessageFrom<
EverythingBut<Equals<AssetHubLocation>>,
Equals<EthereumNetwork>,
>,
DenyThenTry<DenyReserveTransferToRelayChain, TakeWeightCredit>,
>::should_execute(
&AssetHubLocation::get(),
&mut xcm,
Weight::zero(),
&mut Properties { weight_credit: Weight::zero(), message_id: None },
);
assert_err!(result, ProcessMessageError::Unsupported);
}

But definitely, I would be very happy if you move this test for DenyThenTry and DenyReserveTransferToRelayChain to the https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-builder/src/tests/barriers.rs where we have tests for other barriers defined in xcm-builder.

But please do this as a separate PR/issue, because I have a feeling that DenyThenTry does not work for Deny tuple, does it? That's why you used chaining of DenyThenTry instead of tuple?

So for example one test should be dedicated just for DenyThenTry to check all branches, e.g if DenyCase1 returns Ok() does it also checks DenyCase2 and DenyCase3?

DenyThenTry<
    (
        DenyCase1,     
        DenyCase2,     
        DenyCase3,     
    ),
    (
        AllowCase1,     
        AllowCase2,     
        AllowCase3,     
    )
>

If there is such a problem with DenyThenTry, we should at least update documentation (that for Deny you cannot use tuple, but just chaining) and/or find better fix and/or deprecated DenyThenTry. Alternatively, we could go with some dedicated Allow filtering wrapper barrier with EverythingBut like behavior.


/// Deny executing the XCM if it matches any of the Deny filter regardless of anything else.
/// If it passes the Deny, and matches one of the Allow cases then it is let through.
pub struct DenyThenTry<Deny, Allow>(PhantomData<Deny>, PhantomData<Allow>)

I am worried here about: /// Deny executing the XCM if it matches any of the Deny filter regardless of anything else.,
if it passes (does not match/ignore XCM case) DenyCase1 which returns Ok(()) then the tuple return Ok(()), but does not check DenyCase2 and DenyCase3, which could cause a potential problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yrong actually, let me prepare that issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling that DenyThenTry does not work for Deny tuple, does it? That's why you used chaining of DenyThenTry instead of tuple?

Exactly, the tuple does not work as expected. That's why I added the tests for the DenyThenTry here.

I will address it in a separate PR.

Copy link
Contributor Author

@yrong yrong Jan 14, 2025

Choose a reason for hiding this comment

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

@bkontur Though I revert the change in EthereumBlobExporter with 0a51fec#diff-0bd5b75b88aee6992e3ddba0de8bd08d2ab07ecd38fc745c7d76daff56bba52

Do you think it may be better to keep? so we have the double-check first in Barrier and then in EthereumBlobExporter.

This PR is mainly for addressing one security issue recently found and I feel a bit more secure with both of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine as it is, just please remove this test deny_with_reserve_transfer_to_relay_chain from here as a part of that #7148, it is not important here.


This PR is mainly for addressing one security issue recently found and I feel a bit more secure with both of that.

The test covering the security issue is more important at the runtime level (not the primitives level). If you want to be sure, simply write a unit test directly in the BridgeHubWestend runtime - e.g. trigger the xcm-executor with a malicious XCM program (ExportMessage(Eth) and origin other than AssetHubLocation), where the runtime uses the real type Barrier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
2 changes: 2 additions & 0 deletions bridges/snowbridge/primitives/router/src/outbound/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use sp_std::{iter::Peekable, marker::PhantomData, prelude::*};
use xcm::prelude::*;
use xcm_executor::traits::{ConvertLocation, ExportXcm};

pub mod barriers;

pub struct EthereumBlobExporter<
UniversalLocation,
EthereumNetwork,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ mod claim_assets;
mod register_bridged_assets;
mod send_xcm;
mod snowbridge;
mod snowbridge_edge_case;
mod teleport;
mod transact;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use xcm_executor::traits::ConvertLocation;
const INITIAL_FUND: u128 = 5_000_000_000_000;
pub const CHAIN_ID: u64 = 11155111;
pub const WETH: [u8; 20] = hex!("87d1f7fdfEe7f651FaBc8bFCB6E086C278b77A7d");
const ETHEREUM_DESTINATION_ADDRESS: [u8; 20] = hex!("44a57ee2f2FCcb85FDa2B0B18EBD0D8D2333700e");
pub const ETHEREUM_DESTINATION_ADDRESS: [u8; 20] = hex!("44a57ee2f2FCcb85FDa2B0B18EBD0D8D2333700e");
const XCM_FEE: u128 = 100_000_000_000;
const TOKEN_AMOUNT: u128 = 100_000_000_000;

Expand Down
Loading
Loading