BUG: Critical vulnerability allows attacker to take control of any NFT

[giansalex]

Summary of Bug

Chains using nft-transfer module are affected.

An attacker can create a custom ICS721 implementation on a remote chain, and send packets with a classId that matches the class-trace path of any NFT on the origin chain, because the nft-transfer module does not verify the owner of the NFT in the destination chain, the attacker can steal any NFT, even if the NFT has never been used in an IBC nft-transfer.

Environment

• iris@1.4.1-gon-testnet
• github.com/bianjieai/nft-transfer@v1.1.1-beta

Steps to Reproduce

• Add custom code to ics721 implementation to allow sending packets with a custom classId,
  e.g {port}/{channel}/{remote-class-id}, where {remote-class-id} is the NFT class to attack.

malicious packet sent:

{
  "classId": "wasm.juno14svms48zknd2583kunct22haqf26tsmzd8z0n50zgfj9mkfygdvqxjhjdj/channel-113/bitvalidator01",
  "classUri": null,
  "classData": null,
  "tokenIds": [
    "bitnftA4"
  ],
  "tokenUris": null,
  "tokenData": null,
  "sender": "juno1zxtlyk7l6n3t46yqse84sza7ar39m8ul3gdfye",
  "receiver": "iaa1zxtlyk7l6n3t46yqse84sza7ar39m8uljcwrp5",
  "memo": "hack"
}

Expected and Actual Behavior

• Expected: ACK with Error: Unauthorized NFT owner
• Actual: attacker receives the NFT

Additional Context

• the attack has been executed on chains using nft-transfer module
  demo: JUNO <> IRIS
  Tx Transfer: D4AD024E4972D7FED3FB9364953D33525313F089C8780386056D17F16BAE50E4
  IBC Recv: 5467BC97AD6CF7A667BF9CF5D3F4519310AA0F341FF2CE3EC59BECC7431626BF

• NFT transferred to my IRIS wallet
  

• Custom ICS721 contract
  https://blueprints.juno.giansalex.dev/#/contracts/juno14svms48zknd2583kunct22haqf26tsmzd8z0n50zgfj9mkfygdvqxjhjdj
  source code: https://github.com/disperze/ics721/tree/gon-attack

[taramakage]

Hey @giansalex, thank you for pointing this out.

So, before the attack, was the owner of the NFT a user with the address iaa1cj6pz6ly4kfxddhc844rf6lkvqze0fv96w47qh?

After the attack, the owner of the NFT became the attacker with the address iaa1zxtlyk7l6n3t46yqse84sza7ar39m8uljcwrp5, even though the real owner never did an IBC NFT transfer?

If I understood correctly, the nft-transfer module should have checked the owner of that NFT but it didn't. In this example, the owner should have been the escrow account iaa1ul6gkpnpd5lzllxc42d0fcd0z889tvxxmjlftc. As a result, the NFT was stolen.

> iris q nft-transfer escrow-address channel-39 nft-transfer 
iaa1ul6gkpnpd5lzllxc42d0fcd0z889tvxxmjlftc

[giansalex]

@taramakage yes, any NFT in the chain can be stolen, even if it is not owned by IBC NFT escrow-address

[taitruong]

This has been pointed out in #158. Where ALL token data can be manipulated, since target takes all token data from source and overrides NFT.

[giansalex]

This has been pointed out in #158. Where ALL token data can be manipulated, since target takes all token data from source and overrides NFT.

??
this bug is not about manipulating the data, it's about stealing the NFTs.

---

Only tokenData can modified as I see in the code (referring to a native NFT on IRIS/Uptick/Omniflix)
check my comment #158 (comment)

[taitruong]

This has been pointed out in #158. Where ALL token data can be manipulated, since target takes all token data from source and overrides NFT.

?? this bug is not about manipulating the data, it's about stealing the NFTs.

Only tokenData can modified as I see in the code (referring to a native NFT on IRIS/Uptick/Omniflix) check my comment #158 (comment)

I leave it to others judging on what manipulating or stealing is ;). As noted in my description in #158 I have provided 2 code snippets for:

1. taken owner ship (overriding recipient) and

    let rugged_receiver = "stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc";
    let receiver = deps.api.addr_validate(rugged_receiver)?;

2. changing token data (example demos just "exploit", but all props in token data can be changed)

token_data: Some(vec![to_binary("{\"exploit\": \"gotcha\"}")?]),

Now as an exploiter, taking ownership, ofc I send it back to my sender wallet including manipulated metadata.

[giansalex]

1. taken owner ship (overriding recipient) and

    let rugged_receiver = "stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc";
    let receiver = deps.api.addr_validate(rugged_receiver)?;

The receiver is not part of the NFT properties that you can change.
You are confusing the fact that a novice user sends the NFT to your IBC-channel, in that case you appropriate only that NFT (a type of Phishing).
The scenario that I pose does not involve cheating the user, you can steal any NFT from the chain, even one that has never been used IBC nft-transfer.

2. changing token data (example demos just "exploit", but all props in token data can be changed)

token_data: Some(vec![to_binary("{\"exploit\": \"gotcha\"}")?]),

Now as an exploiter, taking ownership, ofc I send it back to my sender wallet including manipulated metadata.

That's what I'm saying, you can only change the TokenData, but you can't change the TokenUri as you claim in this comment.

[giansalex]

@taitruong with this demo #705 (comment) , is it clear what the difference is?

[taitruong]

@taitruong with this demo #705 (comment) , is it clear what the difference is?

Oh yes and pretty cool. Token data and uri was intial discussion. Please note in token data, there are ALL relevant data:

https://github.com/cosmos/ibc/blob/4bf1dd9a64a7e8bad1f5c1a47ce3cf1ec58ac581/spec/app/ics-721-nft-transfer/README.md?plain=1#L42-L50

It really includes all data:

• class uri
• a lost of ALL class ids, token data, and token uris
• and Sender and receiver

As I said: thx for elaborating. With ONE single call you will be able to not ONLY rug one NFT but ALL NFTs on this collection. So we all, art3mix, you and I have been identifying vulnerables and exploits on many use cases: phishing, manipulating, stealing and burning NFTs and reminting NFTs.

Great job. Again. We all contributed on this.

[taitruong]

I mean a 'list of ALL class ids'and not a 'lost of....' :)

[giansalex]

As I said: thx for elaborating. With ONE single call you will be able to not ONLY rug one NFT but ALL NFTs on this collection. So we all, art3mix, you and I have been identifying vulnerables and exploits on many use cases: phishing, manipulating, stealing and burning NFTs and reminting NFTs.

yes. that is possible, but it does not change the consequence: any NFT stolen

[taramakage]

This bug has been fixed in nft-transfer#12