diff --git a/CHANGELOG.md b/CHANGELOG.md index 436bd7e010..f5ccea9e74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - [#1216](https://github.com/crypto-org-chain/cronos/pull/1216) Update ethermint to fix of avoid redundant parse chainID from gensis when start server. - [#1230](https://github.com/crypto-org-chain/cronos/pull/1230) Fix mem store in versiondb multistore. - [#1233](https://github.com/crypto-org-chain/cronos/pull/1233) Re-emit logs in callback contract. +- [#1256](https://github.com/crypto-org-chain/cronos/pull/1256) Improve permission checkings for some messages. ### State Machine Breaking diff --git a/app/ante/ante.go b/app/ante/ante.go deleted file mode 100644 index 276d8d76df..0000000000 --- a/app/ante/ante.go +++ /dev/null @@ -1,54 +0,0 @@ -package ante - -import ( - "cosmossdk.io/errors" - sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "github.com/crypto-org-chain/cronos/v2/x/cronos/keeper" - "github.com/crypto-org-chain/cronos/v2/x/cronos/types" - evmante "github.com/evmos/ethermint/app/ante" -) - -// NewAnteHandler add additional logic on top of Ethermint's anteHandler -func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { - return func( - ctx sdk.Context, tx sdk.Tx, sim bool, - ) (newCtx sdk.Context, err error) { - var anteHandler sdk.AnteHandler - - defer evmante.Recover(ctx.Logger(), &err) - - // Check msg authorization - for _, msg := range tx.GetMsgs() { - var permissionToCheck uint64 - var accountToCheck sdk.AccAddress - - switch v := msg.(type) { - case *types.MsgUpdateTokenMapping: - permissionToCheck = keeper.CanChangeTokenMapping - acc, err := sdk.AccAddressFromBech32(v.Sender) - if err != nil { - panic(err) - } - accountToCheck = acc - case *types.MsgTurnBridge: - permissionToCheck = keeper.CanTurnBridge - acc, err := sdk.AccAddressFromBech32(v.Sender) - if err != nil { - panic(err) - } - accountToCheck = acc - } - - if !options.CronosKeeper.HasPermission(ctx, accountToCheck, permissionToCheck) { - return newCtx, errors.Wrap(sdkerrors.ErrInvalidAddress, "msg sender is unauthorized") - } - } - - anteHandler, err = evmante.NewAnteHandler(options.EvmOptions) - if err != nil { - panic(err) - } - return anteHandler(ctx, tx, sim) - }, nil -} diff --git a/app/ante/handler_options.go b/app/ante/handler_options.go deleted file mode 100644 index bb3cfa2e69..0000000000 --- a/app/ante/handler_options.go +++ /dev/null @@ -1,12 +0,0 @@ -package ante - -import ( - evmante "github.com/evmos/ethermint/app/ante" -) - -// HandlerOptions extend the ethermint's AnteHandler options by adding extra keeper necessary for -// custom ante handler logics -type HandlerOptions struct { - EvmOptions evmante.HandlerOptions - CronosKeeper CronosKeeper -} diff --git a/app/ante/interface.go b/app/ante/interface.go deleted file mode 100644 index ae2f0b750b..0000000000 --- a/app/ante/interface.go +++ /dev/null @@ -1,7 +0,0 @@ -package ante - -import sdk "github.com/cosmos/cosmos-sdk/types" - -type CronosKeeper interface { - HasPermission(ctx sdk.Context, account sdk.AccAddress, permissionsToCheck uint64) bool -} diff --git a/app/app.go b/app/app.go index 5458d9b97c..37fc8316d7 100644 --- a/app/app.go +++ b/app/app.go @@ -144,7 +144,6 @@ import ( memiavlstore "github.com/crypto-org-chain/cronos/store" memiavlrootmulti "github.com/crypto-org-chain/cronos/store/rootmulti" - "github.com/crypto-org-chain/cronos/v2/app/ante" "github.com/crypto-org-chain/cronos/v2/x/cronos" cronosclient "github.com/crypto-org-chain/cronos/v2/x/cronos/client" cronoskeeper "github.com/crypto-org-chain/cronos/v2/x/cronos/keeper" @@ -936,7 +935,7 @@ func (app *App) setAnteHandler(txConfig client.TxConfig, maxGasWanted uint64, bl blockedMap[string(addr)] = struct{}{} } blockAddressDecorator := NewBlockAddressesDecorator(blockedMap) - evmOptions := evmante.HandlerOptions{ + options := evmante.HandlerOptions{ AccountKeeper: app.AccountKeeper, BankKeeper: app.BankKeeper, EvmKeeper: app.EvmKeeper, @@ -954,12 +953,8 @@ func (app *App) setAnteHandler(txConfig client.TxConfig, maxGasWanted uint64, bl }, ExtraDecorators: []sdk.AnteDecorator{blockAddressDecorator}, } - options := ante.HandlerOptions{ - EvmOptions: evmOptions, - CronosKeeper: app.CronosKeeper, - } - anteHandler, err := ante.NewAnteHandler(options) + anteHandler, err := evmante.NewAnteHandler(options) if err != nil { return err } diff --git a/x/cronos/keeper/msg_server.go b/x/cronos/keeper/msg_server.go index 9d70a9f753..b2df2384df 100644 --- a/x/cronos/keeper/msg_server.go +++ b/x/cronos/keeper/msg_server.go @@ -66,6 +66,12 @@ func (k msgServer) TransferTokens(goCtx context.Context, msg *types.MsgTransferT // UpdateTokenMapping implements the grpc method func (k msgServer) UpdateTokenMapping(goCtx context.Context, msg *types.MsgUpdateTokenMapping) (*types.MsgUpdateTokenMappingResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + + // check permission + if !k.Keeper.HasPermission(ctx, msg.GetSigners(), CanChangeTokenMapping) { + return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "msg sender is not authorized") + } + // msg is already validated if err := k.Keeper.RegisterOrUpdateTokenMapping(ctx, msg); err != nil { return nil, err @@ -76,6 +82,12 @@ func (k msgServer) UpdateTokenMapping(goCtx context.Context, msg *types.MsgUpdat // TurnBridge implements the grpc method func (k msgServer) TurnBridge(goCtx context.Context, msg *types.MsgTurnBridge) (*types.MsgTurnBridgeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + + // check permission + if !k.Keeper.HasPermission(ctx, msg.GetSigners(), CanTurnBridge) { + return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "msg sender is not authorized") + } + gravityParams := k.gravityKeeper.GetParams(ctx) gravityParams.BridgeActive = msg.Enable k.gravityKeeper.SetParams(ctx, gravityParams) @@ -101,7 +113,7 @@ func (k msgServer) UpdatePermissions(goCtx context.Context, msg *types.MsgUpdate admin := k.Keeper.GetParams(ctx).CronosAdmin // if admin is empty, no sender could be equal to it if admin != msg.From { - return nil, errors.Wrap(sdkerrors.ErrInvalidAddress, "msg sender is authorized") + return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "msg sender is not authorized") } acc, err := sdk.AccAddressFromBech32(msg.Address) if err != nil { diff --git a/x/cronos/keeper/permissions.go b/x/cronos/keeper/permissions.go index 492adf21dc..11fadf7a9f 100644 --- a/x/cronos/keeper/permissions.go +++ b/x/cronos/keeper/permissions.go @@ -28,13 +28,21 @@ func (k Keeper) GetPermissions(ctx sdk.Context, address sdk.AccAddress) uint64 { } // HasPermission check if an account has a specific permission. by default cronos admin has all permissions -func (k Keeper) HasPermission(ctx sdk.Context, account sdk.AccAddress, permissionsToCheck uint64) bool { +func (k Keeper) HasPermission(ctx sdk.Context, accounts []sdk.AccAddress, permissionsToCheck uint64) bool { // case when no permission is needed if permissionsToCheck == 0 { return true } admin := k.GetParams(ctx).CronosAdmin - permission := k.GetPermissions(ctx, account) - mask := permission & permissionsToCheck - return (admin == account.String()) || (mask == permissionsToCheck) + for _, account := range accounts { + if admin == account.String() { + return true + } + permission := k.GetPermissions(ctx, account) + if permission&permissionsToCheck == permissionsToCheck { + return true + } + } + + return false } diff --git a/x/cronos/keeper/permissions_test.go b/x/cronos/keeper/permissions_test.go index 2a42af1963..e8892206d0 100644 --- a/x/cronos/keeper/permissions_test.go +++ b/x/cronos/keeper/permissions_test.go @@ -41,7 +41,7 @@ func (suite *KeeperTestSuite) TestHasPermissions() { priv, err := ethsecp256k1.GenerateKey() suite.Require().NoError(err) address := common.BytesToAddress(priv.PubKey().Address().Bytes()) - cosmosAddress := sdk.AccAddress(address.Bytes()) + cosmosAddress := []sdk.AccAddress{sdk.AccAddress(address.Bytes())} suite.Require().Equal(true, keeper.HasPermission(suite.ctx, cosmosAddress, 0)) suite.Require().Equal(true, keeper.HasPermission(suite.ctx, cosmosAddress, 0)) @@ -49,15 +49,15 @@ func (suite *KeeperTestSuite) TestHasPermissions() { suite.Require().Equal(false, keeper.HasPermission(suite.ctx, cosmosAddress, CanChangeTokenMapping)) suite.Require().Equal(false, keeper.HasPermission(suite.ctx, cosmosAddress, CanTurnBridge)) - keeper.SetPermissions(suite.ctx, cosmosAddress, CanChangeTokenMapping) + keeper.SetPermissions(suite.ctx, cosmosAddress[0], CanChangeTokenMapping) suite.Require().Equal(true, keeper.HasPermission(suite.ctx, cosmosAddress, CanChangeTokenMapping)) suite.Require().Equal(false, keeper.HasPermission(suite.ctx, cosmosAddress, CanTurnBridge)) - keeper.SetPermissions(suite.ctx, cosmosAddress, CanTurnBridge) + keeper.SetPermissions(suite.ctx, cosmosAddress[0], CanTurnBridge) suite.Require().Equal(false, keeper.HasPermission(suite.ctx, cosmosAddress, CanChangeTokenMapping)) suite.Require().Equal(true, keeper.HasPermission(suite.ctx, cosmosAddress, CanTurnBridge)) - keeper.SetPermissions(suite.ctx, cosmosAddress, All) + keeper.SetPermissions(suite.ctx, cosmosAddress[0], All) suite.Require().Equal(true, keeper.HasPermission(suite.ctx, cosmosAddress, CanChangeTokenMapping)) suite.Require().Equal(true, keeper.HasPermission(suite.ctx, cosmosAddress, CanTurnBridge)) } diff --git a/x/cronos/simulation/operations.go b/x/cronos/simulation/operations.go index c664ce54cd..722a8d18fb 100644 --- a/x/cronos/simulation/operations.go +++ b/x/cronos/simulation/operations.go @@ -91,7 +91,7 @@ func SimulateUpdateTokenMapping(ak types.AccountKeeper, bk types.BankKeeper, k * } oper, ops, err := simulation.GenAndDeliverTxWithRandFees(txCtx) - if simAccount.Address.String() != cronosAdmin && errors.Is(err, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "msg sender is authorized")) { + if simAccount.Address.String() != cronosAdmin && errors.Is(err, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "msg sender is not authorized")) { return simtypes.NoOpMsg(types.ModuleName, msg.Type(), "unauthorized tx should fail"), nil, nil } return oper, ops, err