Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AWARD: Ark Protocol Claim "5 - Bug Hunters" #705

Closed
taitruong opened this issue Mar 23, 2023 · 31 comments
Closed

AWARD: Ark Protocol Claim "5 - Bug Hunters" #705

taitruong opened this issue Mar 23, 2023 · 31 comments
Labels
award Claim award!

Comments

@taitruong
Copy link
Contributor

Award and Bug claim has been issued here#158. We are splitting claim into 2 parts. This here covers the Bug Hunters Award claim.

Ark Protocol has reported the following bugs during GoN:

In our opinion there are 2 critical bugs:

  1. manipulating/exploiting token data
    As stated in AWARD: Ark Protocol Claim Best Auditor and/or Bug :) #158 all metadata can be exploited based on these data: https://github.com/cosmos/ibc/blob/4bf1dd9a64a7e8bad1f5c1a47ce3cf1ec58ac581/spec/app/ics-721-nft-transfer/README.md?plain=1#L42-L50

like: sender, receiver, class id, uri, token data

This has been mentioned previously in Feb 2023: #158 (comment)

Specifically these can be exploited: https://github.com/cosmos/ibc/blob/4bf1dd9a64a7e8bad1f5c1a47ce3cf1ec58ac581/spec/app/ics-721-nft-transfer/README.md?plain=1#L368

  1. Life-cycle handling in IBC packet flow on sending IBC packet and responding with ACK packet
    See next comment.
@taitruong
Copy link
Contributor Author

  1. Life-cycle handling in IBC packet flow on sending IBC packet and responding with ACK packet

There is another major issue here: https://github.com/cosmos/ibc/blob/ed849c7bacf16204e9509f0f0df325391f3ce25c/spec/app/ics-721-nft-transfer/README.md?plain=1#L335

In case of returning an NFT back from chain B (aka source chain) back to chain A (aka target chain) general flow is this:
a) NFT gets burned on source chain B
b) NFT unescrows/unlocks on target chain A

Conceptionally NFT transfer is an agreement between both chains, where sender (source chain) delievers an NFT, and receiver (target chain) accepts it (responging with ACK success to source) or not (ACK fail).

If - and only if - both parties have agreed on this, NFT is successfully transferred. It is a deal between both, and once both agree on this (aka handshake) then the deal is accepted and transfer has successfully completed.

The other case is, if it fails for any reasons on target chain. Then NFT transactions need to be undone on both side. On target side it requires 2 simple tasks:
i. send ACK fail
ii. error on TX for rolling back NFT on target chain

On source chain though initial transfer is in another tx. So ack fail is send in another tx! In case of ack fail, it needs to undo previous tx (send IBC message/packet to), but in a different tx!

So there are 2 ways of doing this:

a) trusting target chain
source chain burns NFT before and awaits ack fail and undo/remint NFT. Here is the problem: it needs to trust target chain. Besides this: it also needs all required token data being passed by target chain (yet another exploit)!

b) trustless and escrow
source chain can escrow NFT, while sending transfer to target chain, and then burns it, on ack success or unescrow it on ack fail. In this case it only needs to burn on ACK fail, but all data send from target chain is NOT required!

This opens a lot potential exploits:

  • NFT can be easily completely be changed on source chain on ack fail:
    https://github.com/bianjieai/nft-transfer/blob/v1.1.1-beta/keeper/relay.go#L165

  • even without trusting target's token data, there might be another issue: once there will be an IBC exploit, it may be possible to exploit an ack fail and in this case there is a possibility of having 2 active NFTs on both chains. This is currently highly unlikely, but ICS721 transfer shouldn't rely on this, hence escrow it on send and then burns it on ACK fail.

@taitruong
Copy link
Contributor Author

Bottom line on "2. Life-cycle handling in IBC packet flow on sending IBC packet and responding with ACK packet", exploit is here:
https://github.com/bianjieai/nft-transfer/blob/v1.1.1-beta/keeper/relay.go#L165

@taitruong
Copy link
Contributor Author

taitruong commented Mar 23, 2023

Besides bugs mentioned above, Ark Protocol has tested, reviewed and identified a couple of bugs in ICS721 during last and this year. Here are we'd like to mention some (discord group chat with Stargaze, Juno, and IRISnet):

@giansalex
Copy link
Contributor

In our opinion there are 2 critical bugs:

manipulating/exploiting token data
As stated in #158 all metadata can be exploited based on these data: https://github.com/cosmos/ibc/blob/4bf1dd9a64a7e8bad1f5c1a47ce3cf1ec58ac581/spec/app/ics-721-nft-transfer/README.md?plain=1#L42-L50

  • like: sender, receiver, class id, uri, token data*

The remote chain can send any data it wants (PacketData), the destination chain will issue a separate collection for them. This is the same behavior as ICS20.

Now if a native NFT (e.g. IRIS) is sent to the malicious chain, that would be an attack on the user that should not use just any ibc channel (is equivalent to sending the NFT to the attacker's address).

@giansalex
Copy link
Contributor

a) trusting target chain
source chain burns NFT before and awaits ack fail and undo/remint NFT. Here is the problem: it needs to trust target chain. Besides this: it also needs all required token data being passed by target chain (yet another exploit)!

I don't think, ICS20 works the same and has not been exploited, check this:

You can complete the transaction and burn NFT on IBC Acknowledgement event, but then the other chain will send you the same nft but with other data, and what will the destination chain have to do? mint the nft again, trusting the data it gives you.

Is the same case, if it will be minted again when a Timeout or IBC Ack with error occurs, will have to rely on the data received.

@taitruong
Copy link
Contributor Author

a) trusting target chain
source chain burns NFT before and awaits ack fail and undo/remint NFT. Here is the problem: it needs to trust target chain. Besides this: it also needs all required token data being passed by target chain (yet another exploit)!

I don't think, ICS20 works the same and has not been exploited, check this:

Nevertheless there are 2 exploits currently on token data - as stated above (on returning NFT and burn before sending to previous chain) and in #158 (by sending to exploited contract on another chain and returning manipulated NFT back). Thanks on elaborating that it also affects sender in #346. As mentioned in #158 exploited contracts fulfills 2 purposes: a) as a honey pot rugging others and b) sending yourself to exploit and change metadata. Thought it was clear in steps to reproduce with code snippets on exploited contracts by changing receiver and token data.

Regarding your comment: I didn't look at ICS20 closely. But what you have quoted (link?), read closely:

You can complete the transaction and burn NFT on IBC Acknowledgement event, but then the other chain will send you the same nft but with other data, and what will the destination chain have to do? mint the nft again, trusting the data it gives you.

Is the same case, if it will be minted again when a Timeout or IBC Ack with error occurs, will have to rely on the data received.

It refers on ACK: meaning it is on target chain, and ofc there it can and should be burned. Also ofc target should trust the source - since it is source of trust - nomen est omen.

@giansalex
Copy link
Contributor

Thanks on elaborating that it also affects sender in #346.

the issue #346 has nothing to do with modifying the TokenData 😄, is a different scenario and a critical vulnerability. You seem to be looking to get credit...

As mentioned in #158 exploited contracts fulfills 2 purposes: a) as a honey pot rugging others and b) sending yourself to exploit and change metadata.

this is phishing, and cannot be added as a issue/vulnerability, it happens everywhere.

@taitruong
Copy link
Contributor Author

Thanks on elaborating that it also affects sender in #346.

the issue #346 has nothing to do with modifying the TokenData 😄, is a different scenario and a critical vulnerability. You seem to be looking to get credit...

As mentioned in #158 exploited contracts fulfills 2 purposes: a) as a honey pot rugging others and b) sending yourself to exploit and change metadata.

this is phishing, and cannot be added as a issue/vulnerability, it happens everywhere.

All credits goes to @Art3miX as stated in #158

@taitruong
Copy link
Contributor Author

Thanks on elaborating that it also affects sender in #346.

the issue #346 has nothing to do with modifying the TokenData 😄, is a different scenario and a critical vulnerability. You seem to be looking to get credit...

As mentioned in #158 exploited contracts fulfills 2 purposes: a) as a honey pot rugging others and b) sending yourself to exploit and change metadata.

this is phishing, and cannot be added as a issue/vulnerability, it happens everywhere.

Phishing? From your description in #346 stated here:

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

Users sends NFT to attacker's custom contract, takes it over and sends it back. How do you call yours ;)? IRISnet and Stargaze has been informed by us already last month. They are aware of this. I'm trying to be kind on your elaboration.

@giansalex
Copy link
Contributor

Users sends NFT to attacker's custom contract, takes it over and sends it back. How do you call yours ;)? IRISnet and Stargaze has been informed by us already last month. They are aware of this. I'm trying to be kind on your elaboration.

finally you understood, in my case the user does not need to send any NFT. I can take the nft I want via ICS721.

@giansalex
Copy link
Contributor

Proof

Your NFT
image

Now, is now mine http://gon.disperze.com/tokens?cid=alphacollection001&nid=arkalpha012

Did you have to send your NFT to my IBC channel? you know now the difference, and how critical the bug 346 is

@taitruong
Copy link
Contributor Author

taitruong commented Mar 24, 2023

Proof

Your NFT image

Now, is now mine http://gon.disperze.com/tokens?cid=alphacollection001&nid=arkalpha012

Did you have to send your NFT to my IBC channel? you know now the difference, and how critical the bug 346 is

Again. Great, excellent job on this! Honestly from our side we weren't aware of how deep this exploit could go.

As stated on description above and in #158 basically all data can be manipulated in token (packet) data:
https://github.com/cosmos/ibc/blob/4bf1dd9a64a7e8bad1f5c1a47ce3cf1ec58ac581/spec/app/ics-721-nft-transfer/README.md?plain=1#L42-L50

#346 demos in detail how vulnerable nft module is. If you look closer what token packet contains: it covers not only token data for a single NFT, but for ALL token data for a specific collection. This way it allows to take ownership of ALL NFTs!!!!

Maybe you can extend your demo a bit? It should easily show that it can take ownership of more than one NFT!

In addition it can even change all token data on EACH NFT!!!

@taitruong
Copy link
Contributor Author

More on #346 and #158: so basically it is possible taking over on all collections even up to each OG collection where it is intially created by nft module.

Above we mention another exploit on burning while sending and reminting on ack fail:
#705 (comment)

It will at least remint NFT. Depending on how it handles list of class ids,token data and token uris, it may have same severe outcome as stated in #346.

Even if OG collection is not originated on nft module-based but wasm-based chains, outcome is highly severe:

All NFTs on module-based chain are UNESCROWED like the ones on wasm chains.

@giansalex
Copy link
Contributor

Maybe you can extend your demo a bit? It should easily show that it can take ownership of more than one NFT!

the issue says that attacker can "take control of any NFT", it does not say that only one.

In addition it can even change all token data on EACH NFT!!!

tokenData is only one field of the NFT. so far I am still waiting for a demonstration that you can change the tokenUri field.

@dreamer-zq
Copy link

dreamer-zq commented Mar 24, 2023

a) trusting target chain source chain burns NFT before and awaits ack fail and undo/remint NFT. Here is the problem: it needs to trust target chain. Besides this: it also needs all required token data being passed by target chain (yet another exploit)!

This is not a problem, because the ibc protocol has recorded the hash of the packet during the transfer, so in the ack, if the packet is changed, the verification will fail

@taramakage taramakage added the award Claim award! label Mar 24, 2023
@taitruong
Copy link
Contributor Author

a) trusting target chain source chain burns NFT before and awaits ack fail and undo/remint NFT. Here is the problem: it needs to trust target chain. Besides this: it also needs all required token data being passed by target chain (yet another exploit)!

This is not a problem, because the ibc protocol has recorded the hash of the packet during the transfer, so in the ack, if the packet is changed, the verification will fail

It is not about validation of ACK packet being send by target chain. It has sent it. But with changed data. Can you confirm source is taking packet data from target for re-minting it?

@taitruong
Copy link
Contributor Author

Maybe you can extend your demo a bit? It should easily show that it can take ownership of more than one NFT!

the issue says that attacker can "take control of any NFT", it does not say that only one.

In addition it can even change all token data on EACH NFT!!!

tokenData is only one field of the NFT. so far I am still waiting for a demonstration that you can change the tokenUri field.

Yes, tokendata one field of NFT but in receive and ack, nft module sends and receive token packet(!!!) data. Have a look at it again, token packet has a list of data for each NFT in one single packet with class ids, token data (for each nft) and token uris (not class uris).

I havent had the time checking in what detail token packet data can be exploited. I demoed in general in #158 it can be used for exploits.

@giansalex
Copy link
Contributor

Yes, tokendata one field of NFT but in receive and ack, nft module sends and receive token packet(!!!) data. Have a look at it again, token packet has a list of data for each NFT in one single packet with class ids, token data (for each nft) and token uris (not class uris).

Yes, the packet has many fields, but these are not used when it comes to native NFT. only the tokenData field can be exploited

I havent had the time checking in what detail token packet data can be exploited. I demoed in general in #158 it can be used for exploits.

if you claim an award, you have to show that it works, and upload the evidence. Otherwise you should abstain.

@taitruong
Copy link
Contributor Author

Exploit for stealing an escrowed NFT

There is a fix which fixes to steal an NFT from a not-yet transferred/not escrowed NFT: bianjieai/nft-transfer#12

But still it will be possible to steal an escrowed NFT. See my comment in PR and also here: #713 (comment)

@taitruong
Copy link
Contributor Author

Proof

Your NFT image

Now, is now mine http://gon.disperze.com/tokens?cid=alphacollection001&nid=arkalpha012

Did you have to send your NFT to my IBC channel? you know now the difference, and how critical the bug 346 is

Some of the NFTs are still in process of getting scored... better transfer them back. Like ark018!

@giansalex
Copy link
Contributor

Some of the NFTs are still in process of getting scored... better transfer them back. Like ark018!

ark018 is in your wallet, I just sent alphacollection001/arkalpha012

@taitruong
Copy link
Contributor Author

Here a full example of what data may be changed as stated in #158:

    let ibc_message = NonFungibleTokenPacketData {
        class_id: class.id.clone(), // this can be also modified for taking over ANY collection
        class_uri: Some("https://stolen-class.uri".into()),
        class_data: Some(to_binary("{\"token\": \"stolen\"}")?),

        token_ids: vec![token_id.clone()],
        token_uris: Some("https://stolen-token.uri".into()).map(|uri| vec![uri]),
        token_data: Some(vec![to_binary("{\"token\": \"stolen\"}")?]),

        sender: "stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc".into(),
        receiver: "iaa1g0krhq56pmxmaz8lnj8h6jf55kcsq7x07srg8c".into(),
        memo: Some("stolen memo".into()),
    };
    let ibc_message = IbcMsg::SendPacket {
        channel_id: msg.channel_id.clone(),
        data: to_binary(&ibc_message)?,
        timeout: msg.timeout,
    };

Above example requires only to send a simple IBC message to destination chain where collection is as proven in #346 for unescrowed/not-yet-transferred NFTs. There is a fix for this here: bianjieai/nft-transfer#12

With above example there remains 2 exploits: stealing escrowed NFTs and modifying NFTs. For simplicty above code snippet is modified using ICS721 wasm contract, for showcasing both 2 exploits.

  1. Modifying NFT
    In this case exploiter can just transfer an existing NFT from IRISnet over to exploited contrract, Then exploiter sends it back.

This is how it looked initially:

{
    "id": "ark103",
    "name": "",
    "uri": "https://token.uri",
    "data": "{\"token\": \"data\"}",
    "owner": "iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f",
    "uri_hash": ""
},

This is how it looks after backtransfer:

{
    "id": "ark103",
    "name": "",
    "uri": "https://token.uri",
    "data": "\"{\\\"token\\\": \\\"stolen\\\"}\"",
    "owner": "iaa1g0krhq56pmxmaz8lnj8h6jf55kcsq7x07srg8c",
    "uri_hash": ""
},

What has been changed here is data and owner.

  1. Taking owner any NFT on any collection

Though PR mentioned above fixed exploiting not-yet-send NFTs - by checking whether NFT is escrowed, now escrowed NFT can be easily taken over, by changing class_id in above custom contract:

        class_id: ClassId::new("originalPort/originalChannel/originalCollectionId"),

Again, a simple IBC message can be send from any other chain and NFT can get exploited, since it only checks whether NFT is escrowed, but not whether source channel from incoming packet is from same channel it has been send to at the time it got escrowed. See full explanation in PR and also here: #713 (comment)

Custom contract:
exploit.zip

@taitruong
Copy link
Contributor Author

Yes, tokendata one field of NFT but in receive and ack, nft module sends and receive token packet(!!!) data. Have a look at it again, token packet has a list of data for each NFT in one single packet with class ids, token data (for each nft) and token uris (not class uris).

Yes, the packet has many fields, but these are not used when it comes to native NFT. only the tokenData field can be exploited

I havent had the time checking in what detail token packet data can be exploited. I demoed in general in #158 it can be used for exploits.

if you claim an award, you have to show that it works, and upload the evidence. Otherwise you should abstain.

See my comment above. token data and owner can be changed. Even with fix in PR for #346, it can still be stolen.

@taitruong
Copy link
Contributor Author

See my comment above. token data and owner can be changed. Even with fix in PR for #346, it can still be stolen.

Latest changes in PR12 looks good! Should all be fine afaik! Thx @dreamer-zq @giansalex

bianjieai/nft-transfer#12

@taitruong
Copy link
Contributor Author

Only thing that is left is about token data that can be changed. Question is: "who is allowed to change token data"? It is normally the creator owning a collection, being allowed to changed it.

Like I created this collection: iris query nft denom arkprotocol003 --output json

{
  "id": "arkprotocol003",
  "name": "Ark Protocol - building multichain utilities",
  "schema": "",
  "creator": "iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f",
  "symbol": "arkprotocol_symbol",
  "mint_restricted": true,
  "update_restricted": true,
  "description": "Ark Protocol's mission: build multi-chain NFT utilities, allowing NFTs to move between chains & enabling utilities across multiple chains.",
  "uri": "https://arkprotocol.io",
  "uri_hash": "",
  "data": "{\"github_username\": \"taitruong\", \"discord_handle\": \"mr_t|Ark Protocol#2337\", \"team_name\": \"Ark Protocol\", \"community\": \"All Cosmos chains ;)\"}"
}

As you can see for creator address, it is my wallet. But when doing a back transfer, nft module is changing this data. So nft module behaves like an admin and could possibly change anything in collection - even though it doesn't own this collection!

@taitruong
Copy link
Contributor Author

Question is who is the owner? Creator? nft module? Or both, creator and nft module?

Normally when it comes to collections and NFTs, it is the creator of the collection having sole ownership. Holder/collector: only owns NFT, where NFT is assigned to its wallet address.

Example:

  1. me as a creator, create a collection. creator = owning collection
  2. holder transfer NFT from chain A to B
  3. holder transfer back NFT from chain B to A

For 3 holder can pass token data. NFT module does 2 things: a) transfer NFT to recipient and b) changes token data on collection which is owned by creator!

@taitruong
Copy link
Contributor Author

If creators know that with nft module, users can change token data, question is whether they'd like it or not?

@taitruong
Copy link
Contributor Author

As discussed with @dreamer-zq:

MintRestricted: If set to true, only the owner of the class can mint nft under the class
UpdateRestricted: If set to true, no one can modify nft

In current implementation when UpdateRestricted=true it changes token data. There are internal discussions about how to deal with it and has not been decided.

From my understanding if UpdateRestricted=true, creators and collectors expect nothing can be updated - only owner/creator of collection should.

As a consequence UpdateRestricted=false should be able to update all data, not only token data, but also token uri, class uri, and class data.

This makes it clear and precise for creatos when creating a collection how to set MintRestricted and UpdateRestricted.

@taitruong
Copy link
Contributor Author

In any case, we need to follow SoC / seperation of concerns on responsibilities:

  1. responsibility of collection by nft module and CW721 wasm contracts: here it defines what is allowed for an NFT and collection or not
  2. responsibility of interchain transfer by nft-transfer module and ICS721 wasm contracts: here it defines who can and how transfer works between 2 chains. Everything apart, like internal transfers is not its responsibility and needs to be delegated by collection (nft module or CW721 wasm contract).

@taitruong
Copy link
Contributor Author

A summary on Truth-of-Source where we pointed bug #158 and the importance of validation of token data: https://twitter.com/arkprotocol/status/1641756940962283520?s=20

@dreamer-zq
Copy link

a) trusting target chain source chain burns NFT before and awaits ack fail and undo/remint NFT. Here is the problem: it needs to trust target chain. Besides this: it also needs all required token data being passed by target chain (yet another exploit)!

This is not a problem, because the ibc protocol has recorded the hash of the packet during the transfer, so in the ack, if the packet is changed, the verification will fail

It is not about validation of ACK packet being send by target chain. It has sent it. But with changed data. Can you confirm source is taking packet data from target for re-minting it?

What I mean is that in a cross-chain cycle, the content of the nft destroyed on the target chain is consistent with the content of the re-mint on the target chain. This is guaranteed by the ibc protocol and has nothing to do with our app layer. If The original link failed to receive nft, the attacker changed the content of the data packet in the ack, and accepting this data packet on the target chain will be intercepted by ibc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
award Claim award!
Projects
None yet
Development

No branches or pull requests

4 participants