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

[yrong]

Resolves:
https://linear.app/snowfork/issue/SNO-1243
https://linear.app/snowfork/issue/SNO-1242

[acatangiu]

I think it's wrong to enforce this rule at the router level:

• maybe some other chain wants to use this router primitive with another bridge (unlikely)
• maybe we change the router in the AH runtime and the new one doesn't validate this bridge-specific rule (likely)
• XCM goes to bridge on Bridge Hub through another path than this router and this rule is not enforced (potentially dangerous)

This rule should definitely be enforced by the bridge itself on Bridge Hub. We can have the router also enforce it in order to fail early, fail at AH instead of failing on the next hop on BH. If this is the case, then the impl details comments below apply.

[yrong]

We can have the router also enforce it to fail early at AH instead of failing on the next hop on BH.

IIUC EthereumBlobExporter is the only entry point where users can send arbitrary messages to OutboundQueue. Meanwhile, since we've already patched with polkadot-fellows/runtimes#506 to disable the agent/channel, I'd assume it's safe for now, so maybe we can improve that in a separate PR?

[bkontur]

I think it's wrong to enforce this rule at the router level:

* maybe some other chain wants to use this router primitive with another bridge (unlikely)

* maybe we change the router in the AH runtime and the new one doesn't validate this bridge-specific rule (likely)

* XCM goes to bridge on Bridge Hub through another path than this router and this rule is not enforced (potentially dangerous)

This rule should definitely be enforced by the bridge itself on Bridge Hub. We can have the router also enforce it in order to fail early, fail at AH instead of failing on the next hop on BH. If this is the case, then the impl details comments below apply.

I agree, and I think changing the EthereumBlobExporter to filter the AssetHub paraId is not necessary. On the Bridge Hub, the EthereumBlobExporter is triggered only when the ExportMessage instruction is received.
A simpler and better solution would be to add a custom Barrier to forbid the ExportMessage for Ethereum if the origin is not AssetHubLocation here.

This solution should be better and safer because it prevents the execution of any XCM from starting. As a result, it will fail fast without executing other instructions before ExportMessage (so no withdrawals and so on). Do we want/need to trap any assets in this case? I would say no.

Something like this:

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
		(
			DenyReserveTransferToRelayChain,
			// TODO: here just add this `Deny` filter
			DenyFirstExportMessageFrom<
				EverythingBut<Equals<AssetHubLocation>,
		                Equals<EthereumNetwork>
			>
		(
			// Allow local users to buy weight credit.
			TakeWeightCredit,

pub struct DenyFirstExportMessageFrom<FromOrigin, ToGlobalConsensus>(PhantomData<(FromOrigin, ToGlobalConsensus)>);
impl<FromOrigin: Contains<Location>, ToGlobalConsensus: Contains<NetworkId>> ShouldExecute for DenyFirstExportMessageFrom<(FromOrigin, ToGlobalConsensus)> {
	fn should_execute<RuntimeCall>(...

[yrong]

As a result, it will fail fast without executing other instructions before ExportMessage (so no withdrawals and so on). Do we want/need to trap any assets in this case? I would say no.

@bkontur That makes sense. I've made some changes accordingly please check.

[bkontur]

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.

[bkontur]

@yrong actually, let me prepare that issue

[bkontur]

#7148

[yrong]

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.

[yrong]

@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.

[bkontur]

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.

[yrong]

3a66cdb

[yrong]

I'll rebase this PR on top of #7169, so please don't merge.