Inconsistencies of XCM NonFungible adapters. And an easy-to-introduce vulnerability

[mrshiposha]

Description of bug

Note: this issue describes an easy-to-introduce vulnerability. It's not an actual "live" vulnerability. That's why it is expressed publicly.

1. Different behavior of T and (T,)

Both the NonFungiblesAdapter and the NonFungibleAdapter implement the transfer_asset function of the TransactAsset trait. This leads to the inconsistency between the implementations of TransactAsset for T and for a tuple (T,).
Why this happens:

• The implementation for a NonFungible adapter doesn't implement the internal_transfer_asset function, so the default implementation is used that returns the Err(XcmError::Unimplemented). The non-fungible adapters implement the transfer_asset directly instead. The transfer_asset function has a non-trivial default implementation which will attempt to call the internal_transfer_asset first, and if failed, will resort to the combination of the withdraw_asset + deposit_asset.
  In NonFungible adapters, this default logic is substituted. So, if we call the transfer_asset directly on an adapter type, the adapter's implementation will be used.
• The implementation for tuples implements the internal_transfer_asset leaving the default behavior of the transfer_asset untouched. So, if we call the transfer_asset on a tuple of an adapter type (e.g., (T,)), the withdraw_asset + deposit_asset combination will be used (because the adapter's internal_transfer_asset will return an error).
  The actual implementation of the transfer by the adapter is completely ignored.

Different behavior of T and (T,) is bad by itself, but also the NonFungible adapters' TransactAsset implementations contradict the Fungible adapters that implement the internal_transfer_asset

• FungiblesAdapter impl
• FungibleAdapter impl

WARNING: Implementing internal_transfer_asset instead of the transfer_asset in the NFT adapters is NOT enough, as it will strengthen the adapters' vulnerability described below. Moreover, fixing this will INTRODUCE the vulnerability to AssetHub.

2. Both NFT adapters don't check the from account in the transfer_asset

In the implementation of the transfer_asset, neither of the adapters checks the from account. They transfer the asset to the to account without any permission checks.

• NonFungiblesAdapter impl
• NonFungibleAdapter impl

If the transfer_asset of the NFT adapters is used unwary in a chain's runtime, an NFT could be stolen.
This vulnerability is relatively easy to introduce (see the reasons below).

3. The conjunction of the above issues

Suppose a chain has the following setting in its XCM config: type AssetTransactors = NonFungiblesAdapter<...>;. Notice that the AssetTransactors is NOT a tuple but just a NonFungiblesAdapter.
Then, suppose the pallet-xcm's config allows a user to execute an arbitrary XCM message (as the AssetHub does). In that case, a user might execute a simple program containing just the TransferAsset XCM instruction (via the pallet-xcm.execute extrinsic).
This will trigger the adapter's transfer_asset implementation, which doesn't include the ownership checks.
IF a NonFungible adapter is used directly, not in a tuple, it produces the described vulnerability.
Fortunately, at the moment, the NonFungible adapters are never used alone, as in this example, so the AssetHubs of Westend and Rococo aren't vulnerable to this (checked locally for Westend; see steps to reproduce for the details).
Polkadot's and Kusama's AssetHub don't include NonFungible adapters.

Steps to reproduce

Exposing NonFungible adapters vulnerability

1. Change the AssetTransactors in Westend to the following:

pub type AssetTransactors = UniquesTransactor;

2. Compile the polkadot-parachain-bin package
3. Run the Relay and the modified AssetHub locally (with XCM trace logs)
4. Create two different accounts with enough funds on AssetHub (let's call them Alice and Bob)
5. Create an NFT collection via uniques.create by Alice. Set the collection's admin to Alice, too. Set COLLECTION_ID = 42.
6. Mint an NFT within the created collection using uniques.mint. Make Alice the owner of this NFT. Use NFT_ID = 256.
7. Check that the owner of the NFT is Alice: uniques.asset(42, 256)
8. Use the following encoded polkadotXcm.execute

0x1f03040404040002043305a801010104000101006e8e27f729d2ce33a6db48a49c2653843d81bd0f9fc6a55e2b5fa5ccf7686e34028c8647017d

Submit it using Bob's account. After it executes, recheck the storage: uniques.asset(42, 256). The owner will be the following account: 5EZfMTyug6vyWCAcSBdpBVA8zkErY2veBZyXg7yzG97unHZw.
Bob doesn't have any right to transfer Alice's NFT elsewhere. However, it can do this if the chain uses the adapter as in the first step.

Proving that the actual Westend AssetHub isn't vulnerable

1. Revert the changes to the AssetTransactors made above
2. Repeat the steps 2-7 from the previous section
3. Repeat the 8th step from the previous section and observe the node's logs. The extrinsic will fail. Also, notice that the internal_transfer_asset fallbacks to withdraw_asset + deposit_asset, and Bob can't withdraw the NFT owned by Alice.
   Search for the following message in the logs: xcm::TransactAsset::internal_transfer_asset: [Parachain] did not transfer asset and examine the surroundings.

[mrshiposha]

Hi @franciscoaguirre! Could you look into this please?

I'd propose an ad-hoc fix. We can check the NFT's owner via the Inspect trait in the NonFungible adapters before executing the Assets::transfer. It is ad-hoc since, using this approach, we don't consider any custom transfer approval logic.
However, I believe it should be enough to prevent potential misuse of the NonFungible adapters.

As for the custom things, I believe new abstract NFT traits are needed. Decoupled from the metadata things and more granular. This will make them more general (currently, they repeat the pallet-uniques and pallet-nfts interface).
Also, creating more abstract NFT and metadata traits will make them more XCM-friendly. I could make such a PR.

[acatangiu]

@paritytech/frame-coders PTAL

[ggwpez]

cc @muharem

[franciscoaguirre]

I'm looking into this

[franciscoaguirre]

@mrshiposha Nice catch! Seems weird that the nonfungible trait doesn't take in the source location in the transfer function. I think the solution you described is good. We can do that one first and then look into better nonfungible traits.

[franciscoaguirre]

There's also the nonfungible v2 traits, which I don't know if they improve the situation in any way

[mrshiposha]

There's also the nonfungible v2 traits, which I don't know if they improve the situation in any way

They have the same flaw as v1. Also, v2 traits repeat the interface of pallet-nfts (like v1 traits repeat the interface of pallet-uniques), so they aren't general enough. For instance, they coupled with the notion of metadata defined in pallet-nfts. There are also inconsistencies in these traits (if we view them as a general interface, not as an interface of a concrete pallet).

I'd be happy to describe all the concerns and an alternative set of general NFT / Asset Metadata traits in a draft PR. I could add such a trait set (not replacing the existing traits so that I won't break anything existing). Would you be interested in this?

Note: I view these traits not only as an interface for use in XCM but also as an interface to use in pallet configs. NFTs (or their classes) may represent functional objects (like CoreTime Region), and these objects can be utilized by pallets in a decoupled way, so having a good granular interface is important for such things as well

[franciscoaguirre]

I think a truly granular set of traits for NFTs would be greatly appreciated. I'll review the draft PR when it's open.
I think separating the NFT from the metadata would be useful, since some NFTs are coretime regions and other images.