Game of Zones Phase 3 Deception/Attacks

[zmanian]

Iris
https://github.com/irisnet/goz/tree/master/phase-3

P2P validator
https://economy.p2p.org/cosmos-game-of-zones-phase-3-a-deceptive-rootchain/

Vitwit
https://medium.com/regen-network/goz-phase-3-invalidate-real-tokens-by-minting-fake-tokens-275bb91b7678

[cwgoes]

Iris's bug (which was a spec mistake) has been fixed in cosmos/cosmos-sdk#6337 (SDK-side) and a903b09 (spec-side) already.

P2P validator's deceptive root-chain is a known result in the intentionally chosen IBC security model. Users who transfer tokens to a chain do so at their own risk - if that chain incorrectly implements ICS 20, they may not be able to transfer their tokens back. We should be very clear about this in communication and documentation, but I don't think this requires any code changes.

Vitwit/Regen's post also does not break the security model per se, but it does seem to indicate a bug, we should try to reproduce that - @AdityaSripal or @fedekunze do you have any ideas? Should we try to write an SDK test-case?

Anything I missed?

[ethanfrey]

VitWit's attack is based on the fact that native tokens and ibc-remote tokens are handled the same by the bank module. There is no differentiator other than a naming convention.

A larger change, but maybe useful, would be to store a "ibc" flag on the token somewhere, which is checked. So it is no longer a string but a struct. This is probably too breaking everywhere.

A simpler change would simply to limit the valid characters in any tokens that are not minted by IBC. Only IBC module can mint tokens that have a / and, in fact, all tokens minted by it must contain at least one /. And we get:

func IsIBCToken(denom string) bool {
    return strings.Contains(denom, "/")
}

It seems that [a-z0-9/] are consistently valid chars in token denoms. I wonder if it is possible to add another case-dependent check (for ibc/non-ibc tokens)

[cwgoes]

VitWit's attack is based on the fact that native tokens and ibc-remote tokens are handled the same by the bank module. There is no differentiator other than a naming convention.

That's right, but I still don't understand why they can't send back the amount of tokens originally sent from the Hub - do you know why that was the case?

A simpler change would simply to limit the valid characters in any tokens that are not minted by IBC. Only IBC module can mint tokens that have a / and, in fact, all tokens minted by it must contain at least one /.

This is probably a prudent idea.

[cwgoes]

This has since been fixed; IBC-module-generated tokens have an IBC-specific denomination.

[AdityaSripal]

^Note: This is true assuming that chains do not allow any other module/user to mint tokens with the ibc prefix