AWARD: Ark Protocol Claim "1 - Best Auditor"

[taitruong]

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

Audits and reviews on NFT module and ICS721 wasm contract:

1. audit and bugs identified
   i. 6 bugs during GoN and at least 2 before GoN - see here: AWARD: Ark Protocol Claim "5 - Bug Hunters" #705
   ii. both 2 critical bugs have been identified by team members of Ark Protocol:
   A. exploit token data on transfer (IBC receive/send packet) from source to target chain
   a) exploit and example here: AWARD: Ark Protocol Claim Best Auditor and/or Bug :) #158
   b) please also note the steps to reproduce in AWARD: Ark Protocol Claim Best Auditor and/or Bug :) #158. especiall this code snippet:

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

This exploit is a honeypot, allowing exploiter to take ownership of NFT by overriding receiver address. In return exploiter can just send NFT back to 'official' collection and take ownership by passing any recipient of course - by doing a normal ICS (back) transfer.
Even if a collector would not fall into this honeypot trap. Exploiter can send itself to exploited contract on another chain, change metadata and send it back with modified token data - like with manipulated rare, legendaray traits for selling a former cheap NFT to a higher price.

B. exploit token data on response (IBC ACK packet) with ACK fail on back transfer from target back to source chain
  a) exploit explanation here: https://github.com/game-of-nfts/gon-evidence/issues/705#issuecomment-1481335082

2. Spec reviews and contributions

• outcome in spec after talk with Stargaze, Juno and IRISnet: cosmos/ibc@20dd2b4
• main reviews with @dreamer-zq, @Art3miX @0xekez @humanalgorithm from IRISnet, Stargaze, Juno and team members from Ark Protocol on various discord group chats ("ICS721" and "Ark Protocol <> IRISnet")

More reviews:

• wrong cw721 code id: In case regular (non-mintable) cw721 is used, transfer should cause ACK fail public-awesome/cw-ics721#38
• better error output: proper contract error on cw721 instantiation public-awesome/cw-ics721#47
• test reviews: Ibc3 feature failing tests public-awesome/cw-ics721#36
• using optimizer: Upgraded optimizer version and added optional timeout  public-awesome/cw-ics721#28
• timeout proposal: Added timeout to take number of seconds public-awesome/cw-ics721#29
• upgrade using ibc3: Add ibc3 feature public-awesome/cw-ics721#37
• adding class and token data after review with Stargaze, Juno and IRISnet: Added handling class data and token data public-awesome/cw-ics721#43
• metadata and tests: Art3mix/metadata public-awesome/cw-ics721#45
• integration tests: Feature/2 integration tests public-awesome/cw-ics721#32

[taitruong]

There is a fix for avoiding stealing NFTs. The fix only checks whether NFT is escrowed. So exploiting an not send/not escrowed NFTs is not possible anymore. Nevertheless it is possible to steal an escrowed, from another custom ICS721 contract with a fake class id. The solution is: when receiving a back transfer dest channel must be checked against (outgoing) channel, at the time, when NFT was transferred to specific channel. This way it guarantees that during receival, nft module only accepts back transfer when source channel is the same, as the outgoing channel where it has previously been send to. See my comment here:

bianjieai/nft-transfer#12 (comment)

[taitruong]

As mentioned together with IRISnet, Ark Protocol, has reviewed, supported and tested transfers between ICS721 wasm contract and nft module since Nov. 2022. Like these 2 issues have been identified and fixed:

1. NonFungibleTokenPacketData uses camel case json encoding
   NonFungibleTokenPacketData uses camel case json encoding bianjieai/nft-transfer#2

2. Fix: NFT package JSON encoding
   Fix: NFT package JSON encoding bianjieai/nft-transfer#7

[taitruong]

There is a fix for avoiding stealing NFTs. The fix only checks whether NFT is escrowed. So exploiting an not send/not escrowed NFTs is not possible anymore. Nevertheless it is possible to steal an escrowed, from another custom ICS721 contract with a fake class id. The solution is: when receiving a back transfer dest channel must be checked against (outgoing) channel, at the time, when NFT was transferred to specific channel. This way it guarantees that during receival, nft module only accepts back transfer when source channel is the same, as the outgoing channel where it has previously been send to. See my comment here:

bianjieai/nft-transfer#12 (comment)

See my custom contract and comment in #705

[taitruong]

Also check our review on nft module is able to change token data - even though on origin chain the creator has ownership of this collection: #705 (comment)

Using nft module it allows user to change token data on transferring back to original collection.

[taitruong]

Importance of Truth-of-Source by validating incoming classId provided in NonFungibleTokenPacketData against source port + source channel + token id: https://twitter.com/arkprotocol/status/1641756940962283520