Move ModuleAccounts into a seperate store

[AdityaSripal]

Summary

This is a proposal to move moduleAccounts into a separate account store that can be accessed from a separate keeper that is different from the user account store and accountkeeper.

Currently ModuleAccounts are stored in the same store as user accounts. This allows anyone to send coins to a moduleAccount using the account keeper. An example of this has been provided here: #4795
A user can send a MsgSend tx to send coins to a moduleAccount. Since these extra funds are unexpected, they will break the module account invariant.

By separating the moduleAccounts into their own (sub-)store, it will be impossible for users to exploit a message intended for sending funds to other user accounts to change the balance of a moduleAccount. This is because msgs that are intended for userAccounts will have handlers that access the accountKeeper and update user-account balances. Msgs that are intended to update moduleAccounts will have handlers that can access moduleAccountKeeper as well, so that they alone can update module account balances in a controlled way.

Problem Definition

As mentioned above, so long as moduleAccounts and userAccounts are in the same store, msgs that update account balances can update a moduleAccount's balance in unexpected ways. One potential way to solve this problem is to blacklist moduleAccount addresses from having their balances updated in each relevant msg handler. However, the issue is not restrained to just the bank module. Here's another example of a user breaking the invariant:

1. I set WithdrawAddress in MsgSetWithdrawAddress to be the address of the stake module's bonded_pool moduleAccount
2. I withdraw the rewards and increase the bonded_pool's funds
3. Since this increase is unexpected by stake, the module account invariant breaks

Requiring each module that allows sending of funds (directly or indirectly) to maintain a blacklist of module accounts is also not a long-term solution for the following reasons:

1. This bug is easy to slip past code review even in the best teams, especially if the way funds get sent is a bit roundabout (like the withdraw bug described above).
2. A subtle bug in one module can cause any other module that makes use of moduleAccounts to enter into an incorrect state. Since one only needs to have the address of a moduleAccount to send it funds using this incorrectly implemented msg handler.
3. Thus a module even if it does not itself make use of moduleAccounts, it must still blacklist moduleAddresses. Even if the module developers don't use moduleAccounts in their project, they will have to add blacklist functionality if they intend for the module to be usable by the rest of the community.

Proposal

Create a new store (or some isolated sub-store/keyspace) to store the moduleAccounts.

Create a new Keeper that can interact with this moduleAccountStore in the same way the current AccountKeeper can interact with moduleAccounts.

In any handler that needs to modify a moduleAccount, we pass in the moduleAccountKeeper, where the state changes will happen in a well-defined and expected manner.

In handlers that are simply moving funds between user accounts, we use the regular AccountKeeper which cannot access, let alone update, the moduleAccounts.

This way with any balance we expect to make for user-accounts, we use the AccountKeeper and make it impossible for moduleAccounts to get affected.
With any balance update we expect to make for module-accounts, we do that in a controlled manner through a separate keeper that can only access moduleAccounts and cannot access userAccounts.

This will solve the problem first brought up in #4795, while also providing a clean isolation barrier between moduleAccounts and userAccounts. This reduces the load on module developers while offering the same protection as blacklisting without the risk of wrong implementation

---

For Admin Use

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

[colin-axner]

If we do this I think that the separate store should just be another initialization of AccountKeeper held in Supply Keeper. Also, I think we should consider prefixing ModuleAccount addresses. This would create a clearer distinction between the two