AWARD: Ark Protocol Claim Best Auditor and/or Bug :) about gon-evidence HOT 18 OPEN

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

from gon-evidence.

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

from gon-evidence.

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

ics721_exploit.zip

from gon-evidence.

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": ""
} 

from gon-evidence.

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

from gon-evidence.

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

from gon-evidence.

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

from gon-evidence.

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

from gon-evidence.

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.

from gon-evidence.

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

from gon-evidence.

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.

from gon-evidence.

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

from gon-evidence.

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

from gon-evidence.

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?

from gon-evidence.

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

from gon-evidence.

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

from gon-evidence.

@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

from gon-evidence.

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.

from gon-evidence.