BUG: cw-ICS721 cannot mint tokens received. about gon-evidence HOT 7 OPEN

@giansalex thanks for the detailed write up of this. i appreciate the simple example, and link to the code. i'll get to this early next week likely.

from gon-evidence.

https://github.com/cosmos/ibc/tree/main/spec/app/ics-721-nft-transfer#packet-relay

• When a packet times out, tokens represented in the packet are either unescrowed or minted back to the sender appropriately -- depending on whether the tokens are being moved away from or back toward their source.

According to this, it is understood that the token must have been burned when it was sent.

from gon-evidence.

reposting my thoughts on this from the Discord:

I feel that the CW implementation is behaving correctly and in line with the spec here. the state machine is:

1. send,
2. on error, return
3. on success, burn

the SDK module approach is:

1. send & burn
2. on error, re-mint
3. on success, do nothing

the trouble with this is that the NFT is destroyed before it is confirmed to have been transferred! this feels like an invariant violation to me. imagine if other code performed an action when the NFT is destroyed - the author would probably be surprised to learn that the NFT can be re-minted. we are building a bridge, so in my view we should prioritise correctness over all else.

additionally, NFTs are not locked by the CW approach, you just need to self-relay your ACK, in addition to the packet. If you do that, it will happily burn the NFT on completion.

from gon-evidence.

@giansalex, what's your opinion on how the spec should work? it feels strange to me to allow transferring a NFT that has not been ACK'd as being received yet.

from gon-evidence.

https://github.com/cosmos/ibc/tree/main/spec/app/ics-721-nft-transfer#packet-relay

• When a packet times out, tokens represented in the packet are either unescrowed or minted back to the sender appropriately -- depending on whether the tokens are being moved away from or back toward their source.

According to this, it is understood that the token must have been burned when it was sent.

This is a heavy bug and exploit. Check my explanation here: #705 (comment)

The issue you are pointing out is due to congestions by IBC messages relaying issues, where ACK success and fail messages are delivered very late due to mass amount of packets (e.g. by vector attacks). IBC though guarantees sending ACKs - no matter how long it takes. Only solution here is to self-relay or wait :). But for ICS721 transfer it is good that NFT is locked until then.

from gon-evidence.

This is a heavy bug and exploit. Check my explanation here: #705 (comment)

can you demonstrate this exploit on the chain?
My comment: #705 (comment)

from gon-evidence.

@giansalex, what's your opinion on how the spec should work? it feels strange to me to allow transferring a NFT that has not been ACK'd as being received yet.

sorry, I had not seen this.

I agree with the current implementation, generally nft-transfers will be successful, ACK+error and Timeout events occur rarely or exceptionally (in testnet occurred due to a malicious contract attack).

it is also more optimal for the relayers, since the gas consumed in the NFT burning will be charged to the user (nft-transfer Tx)

from gon-evidence.