358 handle delegation of brz token to automation pallet in xcm config

[gianfra-t]

A custom MultiCurrencyAdapter that is used as AssetTransactor is re-defined in runtime-common which then can be used to specify on the xcm config of the runtimes the asset and beneficiary_location that we want to "intercept", corresponding to the ReserveAssetDeposited's asset an DepositAsset's beneficiary instruction on the XCM message.

Detailed Explanation

Any transfer from the XTokens pallet in moonbeam corresponding to the transfer of a native XC-20 token, will trigger the following instructions:
ReserveAssetDeposited, ClearOrigin, BuyExecution, DepositAsset

See for instance this event which corresponds to the transfer of a local XC 20 with account key 0x931715fee2d06333043d11f658c8ce934ac61d0c.
The corresponding XCM message for this event can be found here where the ReserveAssetDeposited instructions reflects the multilocation of the XC-20 asset, and DepositAsset that of the destination defined in the XTokens::transfer.

The call to the deposit_asset function will happen with the asset defined in ReserveAssetDeposited and the beneficiary location being that of the automation pallet.

[ebma]

It looks good overall, great job @gianfra-t 👍
I think it would be better if we don't add the same barrier on all runtimes though. For now I would only add it to the pendulum runtime and revert the changes for Foucoco and Amplitude. The reason is that the BRZ location only is correct like this on Pendulum. For testing purposes we will probably add a very similar barrier to Foucoco as well (but defining a different contract location instead of BRZ because we need to be the issuer of that contract for testing), so it's great that the barrier is implemented in the common directory and re-usable. Adding the changes for Foucoco should be part of a different ticket once we know the details though.

Thus, I'd suggest we only focus on Pendulum for now and also change the added integration test to only target Pendulum and not the other runtimes.

[TorstenStueber]

Nice PR. I have some general remarks.

• Is the Barrier really the right point to execute side effects? Just the name of the trait ShouldExecute indicates that this is just a pure function that returns Yes or No. Do we have examples from other parachains?
• The xcm message can fail at a later point and we execute the side effect anyway (or am I wrong here?)
• The whole trait MatcherConfig feels overly generic to me. Let's please always just implement the minimum required and add features later if we need them, particularly:
  • why a fee system?
  • why a vector of matcher pairs?
  • why does is the MatcherPair a struct of functions instead of just a struct with an expected multilocation and, well, the DepositAssetMatcher can probably not be simplified that much

[gianfra-t]

To address some of the remarks (as far as I can!):

Is the Barrier really the right point to execute side effects? Just the name of the trait ShouldExecute indicates that this is just a pure function that returns Yes or No. Do we have examples from other parachains?

At first I actually chose the FungiblesAdapter to hold our custom logic, but at that point in the execution we don't have the origin and the ReserveAssetDeposited instruction which we need to confirm that the asset is the BRZ XC-20 token. I agree that the barrier is not the most elegant solution, but is the only place I could think at the moment that had the full set of instruction of the XCM message, and that is simple enough to define custom logic. I could not find any example in other parachains with a similar case, and to be honest don't know how to approach that search.

The xcm message can fail at a later point and we execute the side effect anyway (or am I wrong here?)

I don't think it can fail really, since it is not going to be executed by the xcm_executor anyway. Once we match the special case in this custom barrier, you can see that actually the barrier denies the message, because we don't want it processed in the "normal route", and we have already processed the call to the automation pallet.

The whole trait MatcherConfig feels overly generic to me. Let's please always just implement the minimum required and add features later if we need them

This is true, it is not the "simplest" solution. The idea of using a vector is to allow for easily handling a new XC-20 token like BRZ that will go the same route (to the automation pallet). If this is not needed then we can simplify a bit. But fees was something that was discussed before and since the message will never reach the BuyExecution part, we needed some way to define how to handle fees.

[TorstenStueber]

Wouldn't the AssetTransactor config of xcm-executor be the right place? This is exactly the place where the assets are supposed to be moved into our special multilocation. The deposit_asset function of AssetTransactor will be called in DepositAsset – this is where the funds in the holding accounts are moved to the destination multilocation.

I think that this is better and more flexible then reading the complete xcm message and trying to interpret what it does. Instead let the xcm executor execute the whole message and insert our special code only at the place where the real transaction happens.

I don't think it can fail really, since it is not going to be executed by the xcm_executor anyway. Once we match the special case in this custom barrier, you can see that actually the barrier denies the message, because we don't want it processed in the "normal route", and we have already processed the call to the automation pallet.

I missed that and I would definitely advise against this. We put too many assumptions here on how messages sent by other parachains will look like in the future. I think we should definitely not skip the execution of the xcm executor pallet, particularly all the safety mechanisms built into it (and, e.g., fee payments, etc.)

But fees was something that was discussed before and since the message will never reach the BuyExecution part, we needed some way to define how to handle fees.

If this is not a definition in the issue description, then we should use the normal fee mechanism of the xcm-executor and not start to build a custom solution here. The decision and discussion should happen in the issue instead.

[gianfra-t]

You are right that the AssetTransactor's deposit_asset is the most logical place, something like this. But the problem with that solution I see is that we don't have the ReserveAssetDeposited or more precisely it's assets, which are the ones which were withdrawn in moonbeam, since the instruction DepositAsset is the one that calls the deposit_asset of the transactor, with it's parameters that look like:

DepositAsset { assets: Wild(AllCounted(1)), beneficiary: MultiLocation { parents: 0, interior: X2(PalletInstance(99), GeneralKey { length ....

In this case, how could we prevent that another XC-20 token (BRZ_2) calls transfer with the same destination? The automation pallet would execute anyway.

Instead let the xcm executor execute the whole message and insert our special code only at the place where the real transaction happens.

This I agree until the fee extraction, after that (the deposit instruction left) will fail anyway since we will not map this special destination into a token.

Update:
So I went back to check this option and realize that the original BRZ moonbeam asset may be in fact the "what" of the deposit_asset function, but the other problem is that we could not filter by origin (moonbeam chain) since the ClearOrigin instruction is executed before, so in the context of the AssetTransactor the origin is set to None.

[TorstenStueber]

Yes, it is exactly the _what part of deposit_asset. I think this is an important discussion: I consider it not important where the token is coming from. I see our special multilocation as a black hole that would automatically trigger our automation logic. Any BRZ token that arrives in that black hole – from wherever – would then be forwarded to our automation logic.

Am I naive here? Could this be exploited? If that could be exploited, that means that another parachain would be able to send us an XCM message that we execute blindly that contains the DepositAsset instruction in a "malicious way". Is that even a scenario? Could such a hostile chain exist that just sends malicious DepositAsset instructions to us freely? I think that this is exactly why we have the Barrier, to check that xcm messages are well formed.

But even if we can ensure that the xcm message originates at Moonbeam and is well formed: we do not know whether Moonbeam actually locked the respective amount of BRZ tokens or whether Moonbeam just sent this xcm message to us without any BRZ tokens being locked. We need to inherently trust that Moonbeam works correctly and not maliciously here (i.e., acting on XCM messages is not a trustless process).

[gianfra-t]

Then assuming we trust moonbeam and the other connected parachains which is probably a good assumption, and that we do not check for the parachain origin, the only dangerous scenario I see is another parachain that also implements pallet contracts or similar, where some user would be able to replicate this specific set of instructions.

But even if we can ensure that the xcm message originates at Moonbeam and is well formed: we do not know whether Moonbeam actually locked a BRZ token or whether Moonbeam just sent this xcm message to us without any BRZ tokens being sent via XCM.

In this case I think we do because it is reserved asset transfer as the ones here right?

[TorstenStueber]

Then assuming we trust moonbeam and the other connected parachains which is probably a good assumption, and that we do not check for the parachain origin, the only dangerous scenario I see is another parachain that also implements pallet contracts or similar, where some user would be able to replicate this specific set of instructions.

What do you mean, can you elaborate? I don't think that users are themselves able to generate these instructions. Users are not able to send arbitrary XCM messages to us from another parachain.

In this case I think we do because it is reserved asset transfer as the ones here right?

Sure, this is a reserved asset transfer. But just in principle: if Moonbeam implements malicious logic, they could just send us an XCM message that looks exactly like the one that results from a reserve asset transfer.

Futhermore, I think that we need to use the IsReserve configuration of xcm-executor to decide whether a specific origin is really the source of a specific asset and that is where we check that the BRZ token is really coming from Moonbeam (this is used in the first xcm instruction ReserveAssetDeposited: https://github.com/paritytech/polkadot/blob/52209dcfe546ff39cc031b92d64e787e7e8264d4/xcm/xcm-executor/src/lib.rs#L493-L496). The way it is currently implemented should work already: it requires that the asset originates from the origin parachain.

[gianfra-t]

I don't think that users are themselves able to generate these instructions. Users are not able to send arbitrary XCM messages to us from another parachain.

For sure, I was thinking more about a parachain having a pallet contracts or equivalent that allowed their users to create tokens (like the XC-20) and send to a desired destination. I honestly don't know how it would look like, and it may be too far fetched.

I also wasn't aware of the checking in ReserveAssetDeposited. So we can say that by checking the _what we are already checking the origin.
In any case I will rollback to using a custom AssetTransactor as the place for our custom instructions.

[gianfra-t]

I have greatly simplified the implementation as we discussed, also removing the generic capability of allowing multiple "pairs" and only defining one desired asset to intercept (in this case BRZ). We can later put back this capability easily once we are aware of how other asset may be bridged this way.

[ebma]

I don't think that users are themselves able to generate these instructions. Users are not able to send arbitrary XCM messages to us from another parachain.

Maybe it misses the point and you meant something else, but users actually are able to send arbitrary XCM messages to us from other parachains using the send() extrinsic of the xcm-pallet @TorstenStueber.

[gianfra-t]

users actually are able to send arbitrary XCM messages to us from other parachains using the send() extrinsic of the xcm-pallet

But I think one can control that with SendXcmOrigin trait. So I guess we should be aware of what other chains connected to us have in this configuration.

[TorstenStueber]

@ebma I was not aware. I assume that in this case the origin MultiLocation will be different and not just { parent: 1, interior: X1(Parachain(...)) }, correct?

[ebma]

I assume that in this case the origin MultiLocation will be different and not just { parent: 1, interior: X1(Parachain(...)) }, correct?

No, I think it will be the correct MultiLocation of the sending chain. For example, you could use the polkadotXcm::send() extrinsic on Moonbeam to build an XCM transaction with the same instructions that would be created as a result of a call to xtokens::transfer() pallet. I don't think we would be able to tell the difference on our receiving chain.

[gianfra-t]

@ebma I think you are correct that anyone can send a DepositAsset instruction with the form we are intercepting, but I don't think it will execute.
If we look at the DepositAsset code, it first attempts to retrieve the assets from the holding. And the assets may only arrive before when either WithdrawAsset, ReserveAssetDeposited or ReceiveTeleportedAsset execute subsume_asset. So if someone sends that instruction there will be no assets in the holding, and deposit_asset will not be called.
Otherwise this would be a problem for all other xcm transactions.

[TorstenStueber]

@gianfra-t the argument is that anyone could send exactly the same xcm message to us that would originate from a legitimate reserve asset transfer.

But I doubt this. If everyone can send an xcm message to us from Moonbeam and we are not able to tell the difference, then this would be a clear flaw: the whole purpose of a reserve asset transfer is that such an xcm message only arrives at Pendulum when Moonbeam locked an amount of tokens before – like for every bridge. When we execute the xcm message, then we mint an equivalent amount of wrapped tokens. This should of course never happen when Moonbeam did not lock tokens before – otherwise this would be a flawed bridge design.

[TorstenStueber]

I just checked this:

• the send extrinsic will execute the send_xcm function in pallet-xcm where interior is X1(AccountId32({network: Polkadot, id: <the account id of the caller>})) (see here)
• then send_xcm will add a new instruction DescendOrigin(interior) as the first instruction of the xcm message (see here)
• on our side the DescendOrigin instruction will then be executed first and ensure that the origin register of the xcm VM will be changed from {parent: 1, X1(Parachain(...)} to {parent: 1, X2(Parachain(...), AccountId32(...))} (see here)
• when we then execute the next xcm instruction ReserveAssetDeposited, our xcm VM will check whether the origin register is just the parachain where the asset that is sent to us originates (see here)
• which will not hold and the result is that our VM execution will fail with XcmError::UntrustedReserveLocation

One more reason why we should not skip the normal VM execution (as you did originally @gianfra-t).

[ebma]

• when we then execute the next xcm instruction ReserveAssetDeposited, our xcm VM will check whether the origin register is just the parachain where the asset that is sent to us originates (see here)
• which will not hold and the result is that our VM execution will fail with XcmError::UntrustedReserveLocation

If I understand correctly this does not hold because the origin of this polkadotXcm::send() message will be a Junction with a specific account while when using the xtokens pallet the origin will be a Junction that has the parachain itself as the origin?

[TorstenStueber]

I did not check what the xtokens pallet does. I only checked what pallet-xcm does when executing the reserve_transfer_assets extrinsic.

I assume that xtokens works similarly but it might depends on the concrete extrinsic. What extrinsic do you have in mind?

[ebma]

I was just curious as to why I wouldn't be able to execute a reserve transfer of assets with polkadotXcm::send() and I assume the answer is that as you pointed out here, the origin will be set to {parent: 1, X2(Parachain(...), AccountId32(...))} but for the reserve transfer to work when being processed on the target chain, the origin would need to be just {parent: 1, X1(Parachain(...)}? Or am I wrong?

I checked the xtokens pallet and when doing a transfer() eventually it will create an XCM message similar to what you wrote down in the Notion page, see here.

[TorstenStueber]

the origin would need to be just {parent: 1, X1(Parachain(...)}? Or am I wrong?

XCM is so complex and there are many different origins involved here:

• the origin multilocation used when the source xcm message is executed in the source chain (see below, it is the xcm message only containing the TransferReserveAsset instruction) – for this the multilocation {parents: 0, interior: X1(AccountId32 { ... }) } is used
• the initial origin multilocation used when the target xcm message is executed in the target chain – this is just {parents: 1, interior: X1(Parachain(...)) } when the source is another parachain (and {parents: 1, interior: Here } if it is the relay chain)
• the actual origin multilocation used for further processing of the target xcm message after the first DescendOrigin instruction will be executed; in this case the origin multilocation {parents: 1, interior: X2(Parachain(...), AccountId32 {...}) }

The difference between sending an xcm message directly is that the there will always be this extra initial DescendOrigin instruction be added to your message so that the target parachain will always execute it first. If you use any of the extrinsics for reserve transfers, then this instruction will not occur in the target xcm message.

I find it confusing that there is pallet-xcm and xtokens, seems like there is a big overlap in their purpose.

So it seems like the following two extrinsics basically have the same outcome:

• pallet-xcm -> limited_reserve_transfer_assets
• xtokens -> transfer_multiasset

Both of these extrinsics will ultimately call the function execute_xcm_in_credit in the xcm-executor pallet and will provide the following parameters:

• origin: the multilocation {parents: 0, interior: X1(AccountId32 { ... }) }
• message:

  TransferReserveAsset {
    assets: ... // the vector of assets to be sent,
    dest: ... // the multilocation of the target parachain,
    xcm: Xcm(vec![
      BuyExecution {
        fee: ... // the fee multiasset reanchored to the target parachain
        weight_limit: ... // some weight limit
      },
      DepositAsset {
        assets: Wild(AllCounted(/* number of assets to send */)), 
        beneficiary: ... // the recipient multilocation
      },
    ]),
  }
  

So this xcm message with this one instruction will be executed on the source chain.

[ebma]

Nice, looks good to me now 👍

What do you think @TorstenStueber?

[TorstenStueber]

Looks good so far. I want to check a few things and will give my final opinion.

[TorstenStueber]

Great job @gianfra-t! Thanks.