AWARD: EmpowerChain Claim 1: issue in nft-transfer might be possible to use to attack relaying capabilities

[gjermundgaraba]

While relaying I discovered some strange transactions that seemed non-refundable. This was discussed here as well: #329 (comment)

In short, the nft-transfer module incorrectly assumes any class-id with a / in it is an ibc-denom and therefore creates a hashed class-id (ibc/hash)
The issue is this code: https://github.com/bianjieai/nft-transfer/blob/6b8596c47042c45cdba994b2b4e1b64f1520041b/types/trace.go#L64-L78

(A PR with a test and solution is incoming from me over the weekend!)

Seeing how these transactions keep coming up in any relayer configured to clear out timed out transactions, it seems like if you spam these transactions that can never be cleared, you can clog up the relayers. And if relayers are not able to clear out transactions, other legitimate timeout packets can be denied (so basically making this a potential denial of service attack).

[giansalex]

about slash / issue was also reported here #245

[gjermundgaraba]

about slash / issue was also reported here #245

True! This is more about a possible attack vector related to the fact that timeouts cannot be resolved.

[gjermundgaraba]

After having looked more at the code and the spec itself, I am holding back on creating a PR. I have some solutions locally that could allow for / characters, but I am not sure it is wise. So, I wonder if this is actually a bug that should be solved slightly differently:

The ICS721 spec itself declares that: If {classId} contains /, then it must also be in the ICS-721 form which indicates that the tokens have a multi-hop record. Note that this requires that the / (slash character) is prohibited in non-IBC token classIds.

From what I can tell, this means that the NFT module should probably not allow the slash character. But the nft-transfer module should also prohibit transfers of native nft classes that has slash character in the name.

[giansalex]

@gjermundgaraba were you able to verify that this causes the relayer to be unable to complete the IBC Txs?
is related to the problem between Juno <> Stargaze?

[gjermundgaraba]

@gjermundgaraba were you able to verify that this causes the relayer to be unable to complete the IBC Txs? is related to the problem between Juno <> Stargaze?

No it is hard to tell right now because there is spam on that channel as well. But I am convinced this would cause the problem to get much worse if these transactions were in there as well because each relayer bundles up a bunch of these transactions and if some of them start to fail...

[0xekez]

fwiw, it is possible to correctly process NFT denoms with slashes AFIK, just need to parse from the beginning of the string only: https://github.com/public-awesome/ics721/blob/3af19e421a95aec5291a0cabbe796c58698ac97f/contracts/cw-ics721-bridge/src/ibc_helpers.rs#L11-L28

[giansalex]

The problem is in nft-transfer module, it can't transfer the token (denom with slashes) in events such as IBC Rcv, IBC Timeout, IBC Ack with error.