From 8a9ebb7101cf4ce00dd28c5eadcd3662be908dd1 Mon Sep 17 00:00:00 2001 From: Luke Date: Sun, 19 Nov 2023 13:20:42 +0800 Subject: [PATCH] imp(staking): detect the length of the ed25519 pubkey in CreateValidator to prevent panic --- CHANGELOG.md | 1 + x/staking/keeper/msg_server.go | 46 +++++++++++++++++------------ x/staking/keeper/msg_server_test.go | 32 ++++++++++++++++++++ x/staking/types/errors.go | 1 + 4 files changed, 61 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d70a8118662..bc7ddbe9645e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies * (version) [#18063](https://github.com/cosmos/cosmos-sdk/pull/18063) Include additional information in the Info struct. This change enhances the Info struct by adding support for additional information through the ExtraInfo field * (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle. +* (staking) [#18506](https://github.com/cosmos/cosmos-sdk/pull/18506) Detect the length of the ed25519 pubkey in CreateValidator to prevent panic. ### Bug Fixes diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index c0c882454d3f..83efbd39bb51 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -15,6 +15,7 @@ import ( "cosmossdk.io/math" "cosmossdk.io/x/staking/types" + "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" @@ -63,25 +64,6 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting cryptotypes.PubKey, got %T", pk) } - if _, err := k.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pk)); err == nil { - return nil, types.ErrValidatorPubKeyExists - } - - bondDenom, err := k.BondDenom(ctx) - if err != nil { - return nil, err - } - - if msg.Value.Denom != bondDenom { - return nil, errorsmod.Wrapf( - sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Value.Denom, bondDenom, - ) - } - - if _, err := msg.Description.EnsureLength(); err != nil { - return nil, err - } - sdkCtx := sdk.UnwrapSDKContext(ctx) cp := sdkCtx.ConsensusParams() if cp.Validator != nil { @@ -99,6 +81,32 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali "got: %s, expected: %s", pk.Type(), cp.Validator.PubKeyTypes, ) } + + if pkType == "ed25519" && len(pk.Bytes()) != ed25519.PubKeySize { + return nil, errorsmod.Wrapf( + types.ErrConsensusPubKeyLenInvalid, + "got: %d, expected: %d", len(pk.Bytes()), ed25519.PubKeySize, + ) + } + } + + if _, err := k.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pk)); err == nil { + return nil, types.ErrValidatorPubKeyExists + } + + bondDenom, err := k.BondDenom(ctx) + if err != nil { + return nil, err + } + + if msg.Value.Denom != bondDenom { + return nil, errorsmod.Wrapf( + sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Value.Denom, bondDenom, + ) + } + + if _, err := msg.Description.EnsureLength(); err != nil { + return nil, err } validator, err := types.NewValidator(msg.ValidatorAddress, pk, msg.Description) diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index 72bfc7d4a311..df1381aabb25 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" "github.com/golang/mock/gomock" "cosmossdk.io/collections" @@ -14,6 +15,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec/address" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -40,6 +42,16 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { pubkey, err := codectypes.NewAnyWithValue(pk1) require.NoError(err) + var ed25519pk cryptotypes.PubKey = &ed25519.PubKey{Key: []byte{1, 2, 3, 4, 5, 6}} + pubkeyInvalidLen, err := codectypes.NewAnyWithValue(ed25519pk) + require.NoError(err) + + ctx = ctx.WithConsensusParams(cmtproto.ConsensusParams{ + Validator: &cmtproto.ValidatorParams{ + PubKeyTypes: []string{"ed25519"}, + }, + }) + testCases := []struct { name string input *stakingtypes.MsgCreateValidator @@ -104,6 +116,26 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { expErr: true, expErrMsg: "empty validator public key", }, + { + name: "validator pubkey len is invalid", + input: &stakingtypes.MsgCreateValidator{ + Description: stakingtypes.Description{ + Moniker: "NewValidator", + }, + Commission: stakingtypes.CommissionRates{ + Rate: math.LegacyNewDecWithPrec(5, 1), + MaxRate: math.LegacyNewDecWithPrec(5, 1), + MaxChangeRate: math.LegacyNewDec(0), + }, + MinSelfDelegation: math.NewInt(1), + DelegatorAddress: Addr.String(), + ValidatorAddress: ValAddr.String(), + Pubkey: pubkeyInvalidLen, + Value: sdk.NewInt64Coin("stake", 10000), + }, + expErr: true, + expErrMsg: "consensus pubkey len is invalid", + }, { name: "empty delegation amount", input: &stakingtypes.MsgCreateValidator{ diff --git a/x/staking/types/errors.go b/x/staking/types/errors.go index ee687ae2cd7e..d8e91e196b13 100644 --- a/x/staking/types/errors.go +++ b/x/staking/types/errors.go @@ -51,4 +51,5 @@ var ( // consensus key errors ErrConsensusPubKeyAlreadyUsedForValidator = errors.Register(ModuleName, 46, "consensus pubkey is already used for a validator") ErrExceedingMaxConsPubKeyRotations = errors.Register(ModuleName, 47, "exceeding maximum consensus pubkey rotations within unbonding period") + ErrConsensusPubKeyLenInvalid = errors.Register(ModuleName, 48, "consensus pubkey len is invalid") )