From 0a325deddc0bac58cc43e650773cc3408eabc011 Mon Sep 17 00:00:00 2001 From: sampocs Date: Fri, 10 Mar 2023 18:17:07 -0600 Subject: [PATCH] Safely Add Module Account (#658) Closes: #XXX ## Context and purpose of the change * Borrowed from osmosis - in the event that we are attempting to create a module account with the same address as an active user account, we should error instead of overriding it. ## Brief Changelog * Added `CreateModuleAccount` function to first check that a module account can be created * Modified `msg_server_register_host_zone.go` to use the new function when creating module accounts for each host zone * **Question for reviewer**: Are there any additional accounts to add? ## Author's Checklist I have... - [x] Run and PASSED locally all GAIA integration tests - [ ] If the change is contentful, I either: - [ ] Added a new unit test OR - [ ] Added test cases to existing unit tests - [ ] OR this change is a trivial rework / code cleanup without any test coverage If skipped any of the tests above, explain. ## Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] manually tested (if applicable) - [ ] confirmed the author wrote unit tests for new logic - [ ] reviewed documentation exists and is accurate ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? - [ ] This pull request updates existing proto field values (and require a backend and frontend migration)? - [ ] Does this pull request change existing proto field names (and require a frontend migration)? How is the feature or change documented? - [ ] not applicable - [ ] jira ticket `XXX` - [ ] specification (`x//spec/`) - [ ] README.md - [ ] not documented --- utils/module_account.go | 76 +++++++++++++++++++ .../keeper/msg_server_register_host_zone.go | 15 ++-- 2 files changed, 81 insertions(+), 10 deletions(-) create mode 100644 utils/module_account.go diff --git a/utils/module_account.go b/utils/module_account.go new file mode 100644 index 0000000000..46d6f9434d --- /dev/null +++ b/utils/module_account.go @@ -0,0 +1,76 @@ +package utils + +// Borrowed from Osmosis +// https://github.com/osmosis-labs/osmosis/blob/5297ea4b641c0c1090cb253a34a510dd0c85b407/osmoutils/module_account.go +// Allows the safe creation of module accounts in the event that an account has already been initialized at an address + +import ( + "errors" + "fmt" + "reflect" + + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types" +) + +type AccountKeeper interface { + NewAccount(sdk.Context, authtypes.AccountI) authtypes.AccountI + GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI + SetAccount(ctx sdk.Context, acc authtypes.AccountI) +} + +// CanCreateModuleAccountAtAddr tells us if we can safely make a module account at +// a given address. By collision resistance of the address (given API safe construction), +// the only way for an account to be already be at this address is if its claimed by the same +// pre-image from the correct module, +// or some SDK command breaks assumptions and creates an account at designated address. +// This function checks if there is an account at that address, and runs some safety checks +// to be extra-sure its not a user account (e.g. non-zero sequence, pubkey, of fore-seen account types). +// If there is no account, or if we believe its not a user-spendable account, we allow module account +// creation at the address. +// else, we do not. +func CanCreateModuleAccountAtAddr(ctx sdk.Context, ak AccountKeeper, addr sdk.AccAddress) error { + existingAcct := ak.GetAccount(ctx, addr) + if existingAcct == nil { + return nil + } + if existingAcct.GetSequence() != 0 || existingAcct.GetPubKey() != nil { + return fmt.Errorf("cannot create module account %s, "+ + "due to an account at that address already existing & having sent txs", addr) + } + overrideAccountTypes := map[reflect.Type]struct{}{ + reflect.TypeOf(&authtypes.BaseAccount{}): {}, + reflect.TypeOf(&vestingtypes.DelayedVestingAccount{}): {}, + reflect.TypeOf(&vestingtypes.ContinuousVestingAccount{}): {}, + reflect.TypeOf(&vestingtypes.BaseVestingAccount{}): {}, + reflect.TypeOf(&vestingtypes.PeriodicVestingAccount{}): {}, + reflect.TypeOf(&vestingtypes.PermanentLockedAccount{}): {}, + } + + if _, clear := overrideAccountTypes[reflect.TypeOf(existingAcct)]; clear { + return nil + } + return errors.New("cannot create module account %s, " + + "due to an account at that address already existing & not being an overrideable type") +} + +// CreateModuleAccount creates a module account at the provided address. +// It overrides an account if it exists at that address, with a non-zero sequence number & pubkey +// Contract: addr is derived from `address.Module(ModuleName, key)` +func CreateModuleAccount(ctx sdk.Context, ak AccountKeeper, addr sdk.AccAddress) error { + err := CanCreateModuleAccountAtAddr(ctx, ak, addr) + if err != nil { + return err + } + + acc := ak.NewAccount( + ctx, + authtypes.NewModuleAccount( + authtypes.NewBaseAccountWithAddress(addr), + addr.String(), + ), + ) + ak.SetAccount(ctx, acc) + return nil +} diff --git a/x/stakeibc/keeper/msg_server_register_host_zone.go b/x/stakeibc/keeper/msg_server_register_host_zone.go index ae923e60d3..53b9e6426b 100644 --- a/x/stakeibc/keeper/msg_server_register_host_zone.go +++ b/x/stakeibc/keeper/msg_server_register_host_zone.go @@ -5,9 +5,9 @@ import ( "fmt" sdkmath "cosmossdk.io/math" - authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" + "github.com/Stride-Labs/stride/v6/utils" epochtypes "github.com/Stride-Labs/stride/v6/x/epochs/types" recordstypes "github.com/Stride-Labs/stride/v6/x/records/types" "github.com/Stride-Labs/stride/v6/x/stakeibc/types" @@ -64,16 +64,11 @@ func (k msgServer) RegisterHostZone(goCtx context.Context, msg *types.MsgRegiste } } - // create and save the zones's module account to the account keeper + // create and save the zones's module account zoneAddress := types.NewZoneAddress(chainId) - acc := k.accountKeeper.NewAccount( - ctx, - authtypes.NewModuleAccount( - authtypes.NewBaseAccountWithAddress(zoneAddress), - zoneAddress.String(), - ), - ) - k.accountKeeper.SetAccount(ctx, acc) + if err := utils.CreateModuleAccount(ctx, k.accountKeeper, zoneAddress); err != nil { + return nil, errorsmod.Wrapf(err, "unable to create module account for host zone %s", chainId) + } params := k.GetParams(ctx) if msg.MinRedemptionRate.IsNil() || msg.MinRedemptionRate.IsZero() {