Cosmos-sdk: Security Considerations for modules that enable issuance of new tokens handled by the bank module

[zmanian]

Writing up some comments from @jaekwon

The x/bank module currently supports no mechanisms for generating new denominations.

It is possible/ trivial for any module that supports permissionless creation of the new assets that consume excessive resources.

The bank module support arbitrary size BigNums. A module that creates new Coins can issue them in excessively large numbers consuming enormous amounts of state.

The bank module also does not impose any maximum length on denom. One of the design decisions with the bank module is that denoms are stored with every balance instead of having a fixed size identifier that then looks up information from some table. This design decision might require for instance the ICS 20 module to reject tokens with excessively large paths.

[cwgoes]

The Cosmos SDK bank module will definitely need to impose restrictions on denominations & amounts - I am not sure those belong in the IBC specs, but they belong in a spec - maybe the SDK-side one.

The bank module also does not impose any maximum length on denom. One of the design decisions with the bank module is that denoms are stored with every balance instead of having a fixed size identifier that then looks up information from some table.

I am hesitant to make this decision at the specification level (e.g. in ICS 20) due to an implementation choice in a particular bank module, since that implementation choice might not be shared by different bank modules which all communicate using the same IBC packet format.

In general, separately, I do not think it is possible for application-packet specifications to provide meaningful constraints on data validation in cases where data is created by the user (as is the case for denominations and amounts) - at most, the specification can discuss or suggest ways in which state machines might validate the data, but it is still up to the state machines to implement the appropriate checks.

ICS 20 does call the bank module in such a fashion that it could elect to reject an overlong denomination, throw an exception, and cause the recvPacket to revert, allowing a timeout on the source chain. An alternative, though perhaps less UX-friendly, option is to simply burn incoming coins of invalid denomination or amount and let the packet receive complete.

A separate and potentially more serious concern that I recall from an earlier discussion is dealing with the case when a user account has too many denominations, since at present in the SDK we read the whole account into memory at once. Rejecting incoming deposits after a denomination count limit doesn't work here, since it opens up a griefing vector: I could send you lots of nonsensical coins over IBC, and then prevent you from withdrawing staking rewards (if that would increase the number of denominations in your account above the limit). As I recall, the idea we had in that discussion was to whitelist certain denominations ("uatom"), which accounts could always receive, and limit the number of other denominations contained in an account. A bit inelegant, but that would seem to work.

cc @fedekunze @alexanderbez from the SDK side - thoughts?

[alexanderbez]

I am hesitant to make this decision at the specification level (e.g. in ICS 20) due to an implementation choice in a particular bank module, since that implementation choice might not be shared by different bank modules which all communicate using the same IBC packet format.

I pretty much second this.

Wrt to integer and denomination size validation, what do we imagine this will look like in the bank module? Are these on-chain governed parameters? Note, we do have "basic" validation on denom/int size but I imagine IBC will need validation beyond this.

[zmanian]

Yeah I put in this in cosmos-sdk instead ics because it's implementation level issue not a spec level issue.

[cwgoes]

Closing in favour of #5467 and ADR 4 (since merged).