[XCM] Investigate better support for filtering XCM programs with Barrier

[bkontur]

Description

Imagine we have a barrier like the one below. For example, we want to add new rules:

1. Discard/deny any XCM program that contains LockAsset (no real scenario, just for testing purposes to showcase XCM filtering).
2. Discard/deny any XCM program that contains ExportMessage from a particular origin xyz.

Actually, we can use DenyThenTry e.g.:

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
                // `Deny` filter tuple
                (
			DenyLockAsset,
			DenyExportMessage,
                ),
                //  then `Allow` filter tuple
		(
			TakeWeightCredit,
			AllowKnownQueryResponses<PolkadotXcm>,
			WithComputedOrigin<
				(
					AllowTopLevelPaidExecutionFrom<Everything>,
					AllowExplicitUnpaidExecutionFrom<(
						ParentOrParentsPlurality,
						Equals<RelayTreasuryLocation>,
						Equals<SiblingPeople>,
					)>,
					AllowSubscriptionsFrom<ParentRelayOrSiblingParachains>,
					AllowHrmpNotificationsFromRelayChain,
				),
				UniversalLocation,
				ConstU32<8>,
			>,
		),
	>,
>;

But this DenyAndTry has some problems mentioned below.

Problem 1 - ShouldExecute tuple implementation and Deny filter tuple

The tuple implementation for ShouldExecute returns Ok(()) immediately when one of the tuple items passes Tuple::should_execute().

Relates to comment.

This is problematic for the Deny filter because Deny requires all items in the tuple to be checked. For example, if DenyCase1 is okay (meaning it does not deny execution), the tuple will return Ok(()) [here](https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-executor/src/traits/should_execute.rs#L63-L74). However, DenyCase2 and DenyCase3 will be skipped, which is not the desired behavior.

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

Solution

#7169

Problem 2 - Barrier vs nested XCM validation

Instructions like SetErrorHandler(xcm), SetAppendix(xcm), and ExecuteWithOrigin { xcm, .. } are meant to be executed on the local chain, but their inner xcm is not actually checked against the Barrier.

We do not have specific barriers for these instructions, which means they could potentially bypass the Barrier.

DenyReserveTransferToRelayChain is just one example (below). We may encounter different cases in the future. For instance, Snowbridge already has a specific case: #6838.

Example:

For example, all our system parachain barriers are structured like this barrier below.

Ok, we can deny the instruction TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } using DenyReserveTransferToRelayChain on the incoming XCM for system parachains. However, nothing prevents us from wrapping TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } with SetAppendix or ExecuteWithOrigin or SetErrorHandler.

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
		DenyReserveTransferToRelayChain,
		(
			TakeWeightCredit,
			AllowKnownQueryResponses<PolkadotXcm>,
			WithComputedOrigin<
				(
					AllowTopLevelPaidExecutionFrom<Everything>,
					AllowExplicitUnpaidExecutionFrom<(
						ParentOrParentsPlurality,
						Equals<RelayTreasuryLocation>,
						Equals<bridging::SiblingBridgeHub>,
					)>,
					AllowSubscriptionsFrom<ParentRelayOrSiblingParachains>,
					AllowHrmpNotificationsFromRelayChain,
				),
				UniversalLocation,
				ConstU32<8>,
			>,
		),
	>,
>;

Possible solution - be explicit

Adding new DenyInstructionsWithXcmFor barrier which checks nested XCM for SetErrorHandler(xcm), SetAppendix(xcm), and ExecuteWithOrigin { xcm, .. } instruction.

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
                (
                               // Deny for top level XCM program 
                               DenyReserveTransferToRelayChain,
                               // Dedicated barrier for nested XCM programs
                               DenyInstructionsWithXcmFor<
                                                // Repeat all Deny filters here 
                                                DenyReserveTransferToRelayChain,
                                >
                ),
		(
		                ...
				TakeWeightCredit
		                ...
		)

See PoC for DenyInstructionsWithXcmFor

This will also require #7169 to be fixed/merged.

Solution

#7200

Related another potential problem in xcm-executor

I think another potential problem could be in the xcm-executor, where we recursively trigger self.process(..).

1. We use Barrier only once during XcmExecutor::execute(message) here.
2. If it passes, we trigger let result = vm.process(message); here.
3. Once processed (either Ok or Err), we take the XCM from SetAppendix or SetErrorHandler and again trigger let result = vm.process(message);.
4. A similar use case exists for ExecuteWithOrigin, where we again trigger self.process(message).

An easy solution seems to be to trigger Config::Barrier::should_execute(nested_xcm) inside or before self.process(message) or vm.process(message). However, this won't work because, as seen in the example Barrier above, it would require every nested XCM to contain UnpaidExecution or BuyExecution/PayFees.

Therefore, we may need another Barrier/Filter specifically for nested XCM in the XcmExecutor, or we need to adjust the existing Barrier logic.

Scenario Test

#7200 should fix this potential issue, please check the scenario test.

[yrong]

#7169 for the fix

[bkontur]

I found a very old similar issue: #837