Comments (18)
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.
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:
- Provide integration tests for ICS721 contract (Sep 2022)
Source: public-awesome/cw-ics721@6b43422
- 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
- Extend ICS721 contract with class and token data
Source: public-awesome/cw-ics721#43
- 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
- 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:
- Create cw721 on Juno (this custom cw721 can change metadata of received NFTs)
- Buy any cheap NFT on iris (NFT1)
- Transfer it to created cw721 on step 1
- Change NFT1 metadata to 1/1 (to very rare metadata)
- 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.
Related Issues (20)
- AWARD: Disperze Claim 3
- AWARD: Disperze Claim 5
- CLOSED
- AWARD: keyllli223 Claim 2
- AWARD: DAICers Claim 6
- AWARD: Xavier Team Claim 4 HOT 1
- AWARD: Ankaria Claim "6 - Best NFT Memes"
- AWARD: silent Claim 6
- AWARD: Stakeflow Claim 2 HOT 2
- AWARD: NSeven Projects Claim 4
- AWARD: LIVE.Node Claim 5
- AWARD: LIVE.Node Claim 6
- closed
- AWARD: Power//Team Claim 6 - MEME
- AWARD:xbdyhh Claim 6 - MEME
- AWARD:DreamGallery Claim 6 - MEME
- AWARD: Ping.pub Claim "2 - Public Good Awards"
- AWARD: NSeven Projects Claim 6
- AWARD: Disperze Claim 5
- AWARD: Disperze Claim 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gon-evidence.