From 8a9ebb7101cf4ce00dd28c5eadcd3662be908dd1 Mon Sep 17 00:00:00 2001 From: Luke Date: Sun, 19 Nov 2023 13:20:42 +0800 Subject: [PATCH 1/4] 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") ) From 78a746700efa454bb5c80a4af6371d8cc4b63589 Mon Sep 17 00:00:00 2001 From: Luke Date: Mon, 20 Nov 2023 22:07:48 +0800 Subject: [PATCH 2/4] imp(staking): use default bond denom constant --- x/staking/keeper/msg_server_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index df1381aabb25..a869e668025e 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -71,7 +71,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: true, expErrMsg: "empty description", @@ -91,7 +91,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: sdk.AccAddress([]byte("invalid")).String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: true, expErrMsg: "invalid validator address", @@ -111,7 +111,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: nil, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: true, expErrMsg: "empty validator public key", @@ -131,7 +131,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkeyInvalidLen, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: true, expErrMsg: "consensus pubkey len is invalid", @@ -151,7 +151,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 0), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 0), }, expErr: true, expErrMsg: "invalid delegation amount", @@ -191,7 +191,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: true, expErrMsg: "minimum self delegation must be a positive integer", @@ -211,7 +211,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: true, expErrMsg: "minimum self delegation must be a positive integer", @@ -231,7 +231,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 10), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10), }, expErr: true, expErrMsg: "validator's self delegation must be greater than their minimum self delegation", @@ -255,7 +255,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { DelegatorAddress: Addr.String(), ValidatorAddress: ValAddr.String(), Pubkey: pubkey, - Value: sdk.NewInt64Coin("stake", 10000), + Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000), }, expErr: false, }, @@ -285,7 +285,7 @@ func (s *KeeperTestSuite) TestMsgEditValidator() { require.NotNil(pk) comm := stakingtypes.NewCommissionRates(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0)) - msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin("stake", math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt()) + msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt()) require.NoError(err) res, err := msgServer.CreateValidator(ctx, msg) @@ -459,7 +459,7 @@ func (s *KeeperTestSuite) TestMsgDelegate() { comm := stakingtypes.NewCommissionRates(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0)) - msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin("stake", math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt()) + msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt()) require.NoError(err) res, err := msgServer.CreateValidator(ctx, msg) From de5a9dd9852c6b52e4728e43ab0e76789368c1a7 Mon Sep 17 00:00:00 2001 From: Luke Date: Mon, 20 Nov 2023 22:17:33 +0800 Subject: [PATCH 3/4] imp(staking): use the constant for the ed25519 pubkey type --- types/staking.go | 3 +++ x/staking/keeper/msg_server.go | 2 +- x/staking/keeper/msg_server_test.go | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/types/staking.go b/types/staking.go index 38019d46a422..a9cd043d1e46 100644 --- a/types/staking.go +++ b/types/staking.go @@ -22,6 +22,9 @@ var ( // DefaultPowerReduction is the default amount of staking tokens required for 1 unit of consensus-engine power DefaultPowerReduction = sdkmath.NewIntFromUint64(1000000) + + // PubKeyEd25519Type is ed25519 for type the consensus params validator pub_key_types params + PubKeyEd25519Type = "ed25519" ) // TokensToConsensusPower - convert input tokens to potential consensus-engine power diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 83efbd39bb51..b214d4eda82f 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -82,7 +82,7 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali ) } - if pkType == "ed25519" && len(pk.Bytes()) != ed25519.PubKeySize { + if pkType == sdk.PubKeyEd25519Type && len(pk.Bytes()) != ed25519.PubKeySize { return nil, errorsmod.Wrapf( types.ErrConsensusPubKeyLenInvalid, "got: %d, expected: %d", len(pk.Bytes()), ed25519.PubKeySize, diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index a869e668025e..9f371d3223c9 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -48,7 +48,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() { ctx = ctx.WithConsensusParams(cmtproto.ConsensusParams{ Validator: &cmtproto.ValidatorParams{ - PubKeyTypes: []string{"ed25519"}, + PubKeyTypes: []string{sdk.PubKeyEd25519Type}, }, }) From 6cc1bbb885e4fe91815861951ddc4a9d0ecefa38 Mon Sep 17 00:00:00 2001 From: Luke Date: Tue, 21 Nov 2023 09:38:02 +0800 Subject: [PATCH 4/4] imp(staking): use slices.Contains above instead of manually looping --- x/staking/keeper/msg_server.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index b214d4eda82f..a56557b8572b 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -3,6 +3,7 @@ package keeper import ( "context" "errors" + "slices" "strconv" "time" @@ -61,21 +62,14 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali pk, ok := msg.Pubkey.GetCachedValue().(cryptotypes.PubKey) if !ok { - return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting cryptotypes.PubKey, got %T", pk) + return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting cryptotypes.PubKey, got %T", msg.Pubkey.GetCachedValue()) } sdkCtx := sdk.UnwrapSDKContext(ctx) cp := sdkCtx.ConsensusParams() if cp.Validator != nil { pkType := pk.Type() - hasKeyType := false - for _, keyType := range cp.Validator.PubKeyTypes { - if pkType == keyType { - hasKeyType = true - break - } - } - if !hasKeyType { + if !slices.Contains(cp.Validator.PubKeyTypes, pkType) { return nil, errorsmod.Wrapf( types.ErrValidatorPubKeyTypeNotSupported, "got: %s, expected: %s", pk.Type(), cp.Validator.PubKeyTypes,