Design issue: Bank sent to a fresh address should not initialize a base account

[zmanian]

It is becoming more and more common for Cosmos chains to use Module and vesting accounts but sending funds to a fresh Cosmos address automatically initializes the account as a BaseAccount.

We should modify the auth module with a protoaccount type that can receive funds and then be migrated to an actual account type on initialization.

[robert-zaremba]

What is the problem? How does "protoaccount" solve it? It will need to be somehow connected to the the not existing account or use preimage hash check.

[alexanderbez]

@zmanian I'm not quite following the problem. What's wrong with newly created accounts being extensions of a BaseAccount?

[zmanian]

@zmanian I'm not quite following the problem. What's wrong with newly created accounts being extensions of a BaseAccount?

The problem is that if you send coins to an account that is expected to be initialized as Module account by Cosmwasm or Interchain Accounts or as a vesting account by CreateVestingAccount then the account can never be initialized properly.

This leads to each application developer or user planning to use these account to have to develop their own mitigations if initialization is front run on their account.

This is a higher level design issue but obvious in the quicksilver ICA account front running attack.

[alexanderbez]

Ahhh yes, right. But IBC recently fixed this issue where addresses were NOT guessable (anymore). Gaia and thus Quicksilver did not update to this version IIUC.

However, this doesn't protect us from other situations where addresses are guessable (e.g. other ICS standards). What is your proposal?

[ValarDragon]

Yeah this is an issue. Both Osmosis and cosmwasm do hacky workarounds for creating a module account. Here's Osmosis' https://github.com/osmosis-labs/osmosis/blob/main/osmoutils/module_account.go

However, just fixing token sends won't fix the entire problem for us. You can also instantiate a vesting account at any address right now, (hence the complexity of that check)

[zmanian]

My mental model of an API would be something like these state machines

Protoaccount -> tx from signer -> BaseAccount

Protoaccount -> tx from signer -> protovesting account-> vesting tx -> one of the vesting accounts

Protoaccount -> ICA handshake -> ICA account

Protoaccount -> cosmwasm tx -> cosmwasm modules account

Protoaccount -> Module account init -> module account

[ValarDragon]

Hrmm, not sure that the vesting account line is permissive enough
Protoaccount -> tx from signer -> protovesting account-> vesting tx

Perhaps it should allow both of:

• BaseAccount -> tx from signer -> BaseAcc + protovesting -> vesting tx
• Protoaccount -> tx from signer -> BaseAcc + protovesting -> vesting tx

Seems to me you should want to be able to convert an existing address that you own into a vesting account. Its an architectural / gas DOS question if you want to permissionlessly allow anyone to send you vested tokens. (Answer is most likely no, let permissionless stuff be in contracts)

[tac0turtle]

In talking with @testinginprod, we discussed vesting to be a sort of module account to the main address. For example: if addressA sends a vesting tx to addressB it creates addressB-1 for vesting. At the end of vesting the user has to withdraw from the vesting account into the main account. If we account for key rotation or something similar we can merge the vested tokens into the main account even if delegated.

[iramiller]

It feels like the actual root of this issue is that an account can only have a single type... so it is a "first in wins" instead of an aggregation of capabilities. If there is a redesign happening here to address this problem I would rather see solutions that address the lack of support for aggregating traits on accounts.

[robert-zaremba]

expected to be initialized as Module account by Cosmwasm

how cosmwasm accounts are represented? Are they Module derived accounts, eg, using address.Derive?

[robert-zaremba]

@tac0turtle thats a good idea for vesting accounts, when doing it, it would be good to consider support few vesting per single account.

[robert-zaremba]

@ValarDragon it seams that in CreateModuleAccount (given the contract in the comment holds: "addr is derived from address.Module(ModuleName, key)") you will cryptographically never have a conflict (unless you deliberately create two submoulde accounts for the same module / key pair). address.Module is 32 bytes.

[alexanderbez]

It seems what we're all arriving at is a more powerful and capability-driven account system...

[aaronc]

I think the general issue is that there is polymorphism around account types at all (AccountI). In my mind this is all tech debt and everything should be BaseAccount. See #13211.

Vesting and other capabilities should be totally separate concerns from the account structure. The fact that adding multiple vesting locks to a single account could be a DOS issue has been discussed and needs to be dealt with in a refactored vesting module. But again that's a separate concern from accounts which should be really simple and not polymorphic IMHO.

Module accounts should IMHO be represented with module credentials on BaseAccount (see #13742).

What I am describing above is already loosely planned on the SDK roadmap in recent discussions with the team.

[testinginprod]

I agree with @aaronc, I don't think an account should do more besides providing identity which in our case is the AccAddress.
To some extent, considering account rekeying breaks the relationship between Address and PubKey, the credential info could be separated from the account itself.

Then module accounts, cosmwasm contracts etc could just be seen as identities existing in a chain that map to no credential, and hence it's impossible to authorise transactions on their behalf.

All duties of x/auth that do not include:

• identity provision
• authorisation of a state transition on behalf of an identity

should go into separate modules, as @tac0turtle and I discussed, a vesting account can just be seen as a module account that can be controlled by an address and can be used to trigger delegation/undelegation/gov vote, and eventually withdraw coins.

[iramiller]

I don't think an account should do more besides providing identity

I agree with this. The auth module should manage the process of maintaining identity within accounts as well as the associated authentication processes for controlling use of these identities.

which in our case is the AccAddress.

This is where the subtle flaws appear.

Currently the concept of identity within the Cosmos SDK is very incomplete. On one hand the widely held simplified assumption is that a public/private key pair constitutes the entirety of an identity which can be deterministically computed into an address. On the other hand concepts of "re-keying" and accounts that are not backed by keys at all (systems, module, groups, etc) break this model completely.

There are many authorization processes outside of the auth module that rely on aspects of the identity. These processes are constrained by the fact that very little information is associated with a BaseAccount identity so they are forced to rely on inferred knowledge which is why you will see checks against the sequence number and public [key] to determine if an identity has been proven or not. Or alternatively they will check the type of the identity and make determinations on how it can be used based on this (not allowing coins to be sent directly to module accounts for example).

Relying only on BaseAccount and removing the AccountI (and subsequent type extension) will result in wide spread breakage of authorization processes outside of the auth module. If changes are to be made to the auth module concept of identity then we need to make changes that support properly managing an identity. This means tracking more aspects of the identity and the sources of it. It should be explicit when an identity is a system level account and when it is a user account. It should be straight forward to review if an identity that has no proven key pairs (or many). It should be possible to determine if an identity is considered invalid, superseded, or never existed at all.

In summary the auth module should embrace the concept that it manages identity and shed the broken idea that an address (and the implied key pair) constitute a complete view of what an identity is.

[iramiller]

Separately from my comment above for clarity:

A VestingAccount is a great example of a misunderstanding of what an identity is. Vesting Accounts are something that an identity might have, but they are not a defining concept of identity. If we discuss these at all here it should be from the concept of what not to do. Contrasted these with a ModuleAccount which is a fundamental type of identity representing a system process.

[tac0turtle]

this is a topic we are thinking through as part of the auth working group. there are some new primitives we are considering. Let me know if you would like to join the working group slack group

[tac0turtle]

referenced this in a bank epic issue, closing this but it will be used for future discussions of a bank rewrite

[robert-zaremba]

Why the issue is closed as "not planned"? I think the issue is valid and should be tackled.

[tac0turtle]

Why the issue is closed as "not planned"? I think the issue is valid and should be tackled.

if you click on the linked pr #19092 you will see the current conversation and that we have an open pr working on it. I understand its easy to comment prematurely but if you click to linked issues there is usually more information.