BUG: cw-ICS721 cannot mint tokens received.

[giansalex]

Summary of Bug

That happens when a token is sent in a second flow and the cw-ics721 has not received IBC Acknowledge in time.
e.g:

1. i -> j (OK)
2. j -> i (JUNO does not receive IBC ACK in time)
3. i -> j (JUNO cannot mint the token on IBC Recv)

Looking in the code, this occurs because cw-ics721 burns the token sent to the remote chain in IBC ACK event, but due to spam it does not receive the ACK in time.
https://github.com/public-awesome/ics721/blob/main/contracts/cw-ics721-bridge/src/ibc.rs#L172

Steps to Reproduce

• Transfer new nft from IRIS to JUNO. Tx, IBC Rcv
• Return nft to IRIS network. Tx, IBC Rcv
• Transfer the nft again from IRIS to JUNO. Tx

  you got an ACK error, it cannot mint the token on Juno.
  Recv Tx: FDF77BA26C620AAF5D6609727153B27A31958AC9111DDA6D1B3246EC6D927D37

Expected and Actual Behavior

• Expected: Successful token receipt
• Actual: ACK with error: execute wasm contract failed

Additional Context

The burn event must occur at the time of transfer, as does nft-transfer module.
https://github.com/bianjieai/nft-transfer/blob/v1.1.1-beta/keeper/relay.go#L259

[0xekez]

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

[0xekez]

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.

[giansalex]

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.

[0xekez]

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

[taitruong]

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.

[giansalex]

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)

[giansalex]

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