Problem: improve permission checking in a few messages (#1256)

* Problem: improve permission checking in a few messages * Update CHANGELOG.md Signed-off-by: yihuang <[email protected]> * fix unit test * fix error message * fix lint * fix sim * fix msg * Update x/cronos/keeper/msg_server.go Signed-off-by: mmsqe <[email protected]> --------- Signed-off-by: yihuang <[email protected]> Signed-off-by: mmsqe <[email protected]> Co-authored-by: mmsqe <[email protected]>
crypto-org-chain · Dec 11, 2023 · 29cdd9d · 29cdd9d
1 parent ad49e0c
commit 29cdd9d
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 90 deletions.
diff --git 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
diff --git a/app/ante/handler_options.go b/app/ante/handler_options.go
diff --git a/app/ante/interface.go b/app/ante/interface.go
diff --git 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
@@ -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
@@ -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
@@ -41,23 +41,23 @@ 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))
 
 	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
@@ -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