Codecov Report

Merging #2871 (a942a25) into main (1986aaf) will increase coverage by 0.01%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##             main    #2871      +/-   ##
==========================================
+ Coverage   78.31%   78.32%   +0.01%     
==========================================
  Files         181      182       +1     
  Lines       12565    12585      +20     
==========================================
+ Hits         9840     9857      +17     
- Misses       2288     2290       +2     
- Partials      437      438       +1     

Hi @GAtom22! Thanks for taking the initiative to look into this! I did a poor job of fully elaborating on the original issue so I'm exploring the issue/solution more here.

I think it makes sense to implement this feature with migrations. My reasoning is that having part of your escrows in regular accounts and part in module accounts could increase the complexity of security issues, it also would prevent us from removing the additional logic (handling whether it is regular acc or module acc) until we performed migrations asserting the transition.

Since this is a sensitive part of the codebase, I think it makes sense to take this feature slowly and break it into smaller pieces to ensure we do it right without causing any disruption. It is important to understand the exact benefit of doing this change now vs later. From my understanding, the primary benefit would be to ensure that address derivation is safe for any portID/channelID combo, as only "transfer" and channelID's up to the max uint32 have been verified. Is there another reason I am missing?

Without looking into this issue further, I'm not sure we want to create a ModuleAccount type for every escrow address? What is the benefit of this? My understanding is that we would want to use simple base accounts for each escrow address but that the escrow address should be derived from a single transfer module account, similar to how interchain accounts derives the address. If we do these changes, we also need to ensure that if a user pre-sends funds to an escrow account before a channel is opened for that escrow account address, that the escrow account can still be used. I believe this is the case with base accounts (needs to be verified), I'm unsure what would occur with module accounts.

If we are deriving the address from the transfer module account address, then every escrow address will need to have its funds moved to the new escrow address. That is, the old code and the new code would generate different escrow address for the same portID/channelID pair.

We also need to ensure our cli/gRPC's are updated to return the correct escrow address.

Going back to:

From my understanding, the primary benefit would be to ensure that address derivation is safe for any portID/channelID combo, as only "transfer" and channelID's up to the max uint32 have been verified. Is there another reason I am missing?

If we only switched from base accounts to module accounts without changing address generation, we do not add any extra functionality or safety to our codebase and thus I'd prefer not to do the change. (I'm still in favor of ensuring long term we are collision resistant for any portID/channelID pair)

Hi @colin-axner, thanks for your comment! Having module accounts for the escrow accounts makes more sense in my opinion. Leaving them as simple accounts makes them vulnerable to a birthday attack. Quoting ADR-028

1. simple accounts: represented by a regular public key (ie: secp256k1, sr25519)
2. naive multisig: accounts composed by other addressable objects (ie: naive multisig)
3. composed accounts with a native address key (ie: bls, group module accounts)
4. module accounts: basically any accounts which cannot sign transactions and which are managed internally by modules

I believe the module account description is more aligned with the escrow account than the simple account description. These accounts are managed by the transfer module.

Regarding the address derivation as module sub-accounts, I have no problem with it, seems it's a better design. It is true that this will be a breaking change and needs to be done carefully. But I believe these should be module accounts for the reasons explained. LMK your thoughts

Thanks for the followup @GAtom22. Let me see if I can clarify my point.

Let's define some terminology:
BaseAccount: the type defined here
ModuleAccount: the type defined here

When ADR 028 uses simple account they are referring to a BaseAccount which has a public key set. When they refer to a module account they are referring to the ModuleAccount type which is a BaseAccount without a public key whose address is derived from the hash of its name.

My proposal is that escrow addresses are BaseAccounts without a public key set, whose address is derived from a ModuleAccount associated with the transfer module.

My understanding is that the ModuleAccount type is meant to be associated with an SDK module, not an instance of an SDK module. That is, we can have a ModuleAccount for transfer but I don't think it is recommended to create a ModuleAccount for every escrow address, especially since escrow addresses don't need permissions. Instead we can protect against the birthday attack by deriving our escrow address from the transfer ModuleAccount

If we attempted to use ModuleAccounts for escrow addresses, we would actually run into an issue. The account keeper expects module account permissions to be defined at compile time (within the app.go), it then uses this mapping to GetModuleAccounts. If we set module accounts at run time (during chain execution), we wouldn't be able to update this mapping and thus any call to GetModuleAccount would fail. Calling GetAccount on the module account address would still work, but the split behaviour is slightly odd in my opinion.

Given that BaseAccounts with empty public keys appears to work just fine for escrow addresses, I see no reason to wrap it with the ModuleAccount struct.

What do you think?

@colin-axner thanks for the clarification. I understand your point, I'm aware that the GetModuleAccount would not succeed on getting the escrow accounts even if they are ModuleAccounts.

I'm putting emphasis on the account type because this situation has resulted in undesired behavior on our side. There's logic in our codebase where we check if an account is a ModuleAccount. In some cases we do this to perform actions only on BaseAccounts, as users accounts. But being the escrow accounts BaseAccounts, they are treated as if they were users accounts. This generated issues. So, changing the account type to ModuleAccount solves this type of bugs in a scalable way.

Why not just check if the account has a public key set? If the public key is not set, then it is not a user account

That could work in some cases, but in other cases that does not solve the issue. See this code section for example. Changing the account type would solve it

@GAtom22 thanks for linking the code. The current handling also does not account for interchain account types, though it seems quite difficult to me to find the key which corresponds to an account managed by a on chain module and thus bypass this check. Are there concrete concerns regarding this?

I think there's another unconsidered edge case in the existing code. What happens if I send funds to an address which supports or doesn't support the evmos keys? That account will not have a public key set (as I sent it funds), but it might still support evmos keys, but all its funds will be sent to the connected chain regardless. I don't see a safety concern, but it is undocumented behaviour. This case may be possible for regular genesis accounts as well since their public key may be nil. Are all genesis account presumed to have a nil or non-nil public key?

Getting back to ibc-go. I remain unconvinced we should be making any changes to ICS20. Here's my perspective.

The current solution is works and is safe. The issue linked in the pr #330 aims to increase the hardness by ensuring the existing solution is safe for any variant of the ICS 20 portID name, though it is incredibly unclear why anyone would modify the "transfer" portID string and I have yet to see a single chain do so. It would also likely have external UX repercussions and thus is probably ill-advised.

Increasing the hardness of ICS20 security is quite sensitive though and risky. Because funds will be moved to different addresses, there is no room for a single bug. Thus I would strongly recommend no change be made without strong reason to do so.

Changing the account type of the escrow addresses also does not increase functionality for ibc-go. The module account is a base account, with extra information our escrow addresses do not need.

The linked code is trying to ask the account keeper the question "Is this account a user account? If so, does it have a public key supported by us?" and it is unclear to me whether this is a question the SDK can answer this and if they plan to so in the future. In the existing state, the answer appears to be "no" to me. There is no restriction that BaseAccounts cannot be module managed accounts. ModuleAccounts themselves are intended to be statically defined with a module "name", thus it doesn't seem logical to me for a module to instantiate many accounts, especially at runtime, as that would break the existing GetModuleAccount API and potentially other uninvestigated issues.

To bypass the lack of flexibility by the SDK, the request is to update all dependencies to set all module managed accounts as ModuleAccounts. This is an unsustainable approach in my opinion as you will need to constantly vet all dependencies to ensure this assumption is constantly met. As noted in the beginning of this post, the code doesn't account for interchain accounts either. Since this solution is a surface level patch, you will likely face several security incidences over time regarding this same issue

For these reasons, I don't believe ibc-go should set its escrow accounts as ModuleAccounts, at least not now. I would recommend requesting the SDK to tighten up the restriction on what a user account is and isn't, in addition to creating stronger guidelines for how module managed accounts should be depicted, as I believe base accounts are perfectly fine for ICS 20 escrow accounts (maybe it could be enhanced with the restriction that only ics20 can send to/from the accounts)

Assuming genesis accounts you wish to recover funds from do not have a public key set, my recommendation as a short term hack would be to set the escrow address of every authorized channel. Escrow addresses are stateless and thus could be set into state at the same time you set the authorize channels. You can extend your authorized channel type to also set the escrow address, as you iterate through your authorized channels, ensure the escrow address does not match the recipient

@GAtom22 Following up here after @colin-axner's latest comment: is there anything you would like to reply? Do you think we should still keep this PR open?

@crodriguezvega @colin-axner All good, thanks for your time! Will close this