Low inflation rate creates invalid module account - potential chain halt

[rhuairahrighairidh]

Summary of Bug

• If a chain starts with a low inflation rate (or low number of coins), an invalid mint module account is created.
• This invalid account can then trigger a chain halt if the inflation rate (or coin amount) is increased.

Details:

• For new chains, after InitGenesis,mint's module account doesn't exist. Normaly it's created as needed when mint's beginblocker runs.
  • mint's begin blocker calls supply.Mint which creates a mod account if one doesn't exist.
  • then newly minted coins are sent from this to the fee mod account
• However if the coins to be minted that block are 0, then minting is skipped, so no account is created.
• But sending coins from this non existant account to the fee mod account is not skipped. So zero coins are sent.
• This triggers the bank module to create an empty BaseAccount with the address for mint's module account
• Then if supplyever accesses this account it panics in the type assertion trying to convert it to module account. (

  cosmos-sdk/x/supply/internal/keeper/account.go

  Line 37 in 1b6897a

  macc, ok := acc.(exported.ModuleAccountI)

  )

Version

current

Steps to Reproduce

• Set up a new chain using gaia master
• modify the genesis file to set the current inflation rate (and min rate) to zero
• start chain
• gaiacli q account cosmos1m3h30wlvsf8llruxtpukdvsy0km2kum8g38c8q should return the mint module account, however it is not a module account

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

Excellent investigation! Thanks, @rhuairahrighairidh.

@fedekunze, I don't understand why this is the flow of execution in the context of the minting module account not existing in genesis (or any module account for that matter).

It seems much more intuitive and sound to have the module accounts created (if they don't exist) during InitGenesis. Why is this not the case? Was this approach ever considered and decided against for some reason?

[alexanderbez]

Alternatively, I've noticed x/staking takes care of this in its constructor. We can also do this for minting.

@rhuairahrighairidh would you like to take a stab at this simple approach?

[rhuairahrighairidh]

Looks like mint already has the same account checks in it's constructor as staking. These don't check for the presence of an account in the store, just that the account and permissions were registered in the app definition.

[alexanderbez]

@rhuairahrighairidh calling sk.GetModuleAccount(ctx, moduleName) does create the account. You can simply call this method. Although I do agree this is a poor API design.

/cc @fedekunze

[rhuairahrighairidh]

Correct me if I'm missing something, but staking's keeper constructor calls sk.GetModuleAddress, not Account, which doesn't create the accounts.
Calling GetModuleAccount in NewKeeper would need access to the db, which isn't initialised yet at app creation

[alexanderbez]

Correct, so not only are current constructors broken in their intent of using GetModuleAddress, we also cannot call GetModuleAccount as we do not have a Context.

So I would recommend we:

1. Remove GetModuleAddress from constructors as it's pointless.
2. Call GetModuleAccount during InitGenesis (for the relevant modules)

Changes should be pretty trivial to implement.

[rhuairahrighairidh]

I agree with point 2, that would align mint with how other modules do it. However I believe GetModuleAddress does check whether the account has had permissions set in the app constructor.
I'm lacking some context for why things were set up this way. @fedekunze do you have any recommendations?

Also what do you think about skipping sending 0 coins in the mint begin blocker? Even with an account set up it will emit a send event for 0 which seems a bit odd.

[alexanderbez]

I agree that we should avoid a zero balance send, but I don't see why (1) won't fix the overall issue?