Merge pull request from GHSA-hwgx-9jq6-cx8h

fix(vesting): refactor to fix vulnerabilities

CHANGELOG.md

@@ -0,0 +1,7 @@
+# Changelog
+
+## Unreleased
+
+### State Machine Breaking
+
+- [#1](https://github.com/evmos/vesting-ghsa-hwgx-9jq6-cx8h/pull/1) Refactor logic to fix the track delegation calculation and anteHandler for `MsgCreateValidator`

ante/setup_test.go

@@ -0,0 +1,99 @@
+package ante_test
+
+import (
+	"testing"
+
+	"github.com/cosmos/cosmos-sdk/client"
+	"github.com/cosmos/cosmos-sdk/client/tx"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
+	sdktestutil "github.com/cosmos/cosmos-sdk/testutil"
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
+	xauthsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
+	gomock "go.uber.org/mock/gomock"
+
+	"github.com/evmos/vesting/ante"
+	"github.com/evmos/vesting/testutil"
+	"github.com/evmos/vesting/x/vesting"
+	"github.com/evmos/vesting/x/vesting/types"
+)
+
+// AnteTestSuite is a test suite to be used with ante handler tests.
+type AnteTestSuite struct {
+	dec           sdk.AnteDecorator
+	ctx           sdk.Context
+	clientCtx     client.Context
+	txBuilder     client.TxBuilder
+	accountKeeper *testutil.MockAccountKeeper
+	bankKeeper    *testutil.MockBankKeeper
+	stakingKeeper *testutil.MockStakingKeeper
+	encCfg        moduletestutil.TestEncodingConfig
+}
+
+// SetupTest setups a new test, with new app, context, and anteHandler.
+func setupTestSuite(t *testing.T) *AnteTestSuite {
+	suite := &AnteTestSuite{}
+	ctrl := gomock.NewController(t)
+	suite.accountKeeper = testutil.NewMockAccountKeeper(ctrl)
+	suite.bankKeeper = testutil.NewMockBankKeeper(ctrl)
+	suite.stakingKeeper = testutil.NewMockStakingKeeper(ctrl)
+
+	key := sdk.NewKVStoreKey(types.StoreKey)
+	testCtx := sdktestutil.DefaultContextWithDB(t, key, sdk.NewTransientStoreKey("transient_test"))
+	suite.ctx = testCtx.Ctx.WithBlockHeight(1)
+	suite.encCfg = moduletestutil.MakeTestEncodingConfig(vesting.AppModuleBasic{})
+
+	// We're using TestMsg encoding in some tests, so register it here.
+	testdata.RegisterInterfaces(suite.encCfg.InterfaceRegistry)
+
+	suite.clientCtx = client.Context{}.
+		WithTxConfig(suite.encCfg.TxConfig)
+
+	anteHandler := ante.NewVestingDelegationDecorator(
+		suite.accountKeeper,
+		suite.stakingKeeper,
+		suite.bankKeeper,
+		suite.encCfg.Codec,
+	)
+
+	suite.dec = anteHandler
+
+	suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()
+
+	// Setup response for bond denom
+	suite.stakingKeeper.EXPECT().BondDenom(gomock.Any()).Return(testutil.StakeDenom).AnyTimes()
+
+	return suite
+}
+
+// createTestTx is a helper function to create a tx with given inputs.
+func (suite *AnteTestSuite) createTestTx(priv cryptotypes.PrivKey, accNum, accSeq uint64, chainID string) (xauthsigning.Tx, error) {
+	signerData := xauthsigning.SignerData{
+		ChainID:       chainID,
+		AccountNumber: accNum,
+		Sequence:      accSeq,
+	}
+	sigV2, err := tx.SignWithPrivKey(
+		suite.clientCtx.TxConfig.SignModeHandler().DefaultMode(), signerData,
+		suite.txBuilder, priv, suite.clientCtx.TxConfig, accSeq)
+	if err != nil {
+		return nil, err
+	}
+
+	err = suite.txBuilder.SetSignatures(sigV2)
+	if err != nil {
+		return nil, err
+	}
+
+	return suite.txBuilder.GetTx(), nil
+}
+
+// nextFn is a no-op function that returns the context and no error in order to mock
+// the next function in the AnteHandler chain.
+//
+// It can be used in unit tests when calling a decorator's AnteHandle method, e.g.
+// `dec.AnteHandle(ctx, tx, false, nextFn)`
+func nextFn(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) {
+	return ctx, nil
+}

ante/vesting.go

@@ -1,30 +1,29 @@
 // Copyright Tharsis Labs Ltd.(Evmos)
 // SPDX-License-Identifier:ENCL-1.0(https://github.com/evmos/evmos/blob/main/LICENSE)
-package cosmos
+package ante
 
 import (
 	errorsmod "cosmossdk.io/errors"
+	"cosmossdk.io/math"
 	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	errortypes "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/authz"
-	bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
-	"github.com/cosmos/cosmos-sdk/x/bank/types"
-	stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
-	vestingtypes "github.com/evmos/vesting/x/vesting/types"
+
+	"github.com/evmos/vesting/x/vesting/types"
 )
 
 // VestingDelegationDecorator validates delegation of vested coins
 type VestingDelegationDecorator struct {
 	ak  types.AccountKeeper
-	sk  stakingkeeper.Keeper
-	bk  bankkeeper.Keeper
+	sk  types.StakingKeeper
+	bk  types.BankKeeper
 	cdc codec.BinaryCodec
 }
 
 // NewVestingDelegationDecorator creates a new VestingDelegationDecorator
-func NewVestingDelegationDecorator(ak types.AccountKeeper, sk stakingkeeper.Keeper, bk bankkeeper.Keeper, cdc codec.BinaryCodec) VestingDelegationDecorator {
+func NewVestingDelegationDecorator(ak types.AccountKeeper, sk types.StakingKeeper, bk types.BankKeeper, cdc codec.BinaryCodec) VestingDelegationDecorator {
 	return VestingDelegationDecorator{
 		ak:  ak,
 		sk:  sk,
@@ -72,8 +71,15 @@ func (vdd VestingDelegationDecorator) validateAuthz(ctx sdk.Context, execMsg *au
 
 // validateMsg checks that the only vested coins can be delegated
 func (vdd VestingDelegationDecorator) validateMsg(ctx sdk.Context, msg sdk.Msg) error {
-	delegateMsg, ok := msg.(*stakingtypes.MsgDelegate)
-	if !ok {
+	var delegationAmt math.Int
+	// need to validate delegation amount in MsgDelegate
+	// and self delegation amount in MsgCreateValidator
+	switch stkMsg := msg.(type) {
+	case *stakingtypes.MsgDelegate:
+		delegationAmt = stkMsg.Amount.Amount
+	case *stakingtypes.MsgCreateValidator:
+		delegationAmt = stkMsg.Value.Amount
+	default:
 		return nil
 	}
 
@@ -86,35 +92,39 @@ func (vdd VestingDelegationDecorator) validateMsg(ctx sdk.Context, msg sdk.Msg)
 			)
 		}
 
-		clawbackAccount, isClawback := acc.(*vestingtypes.ClawbackVestingAccount)
+		clawbackAccount, isClawback := acc.(*types.ClawbackVestingAccount)
 		if !isClawback {
 			// continue to next decorator as this logic only applies to vesting
 			return nil
 		}
 
 		// error if bond amount is > vested coins
 		bondDenom := vdd.sk.BondDenom(ctx)
-		coins := clawbackAccount.GetVestedOnly(ctx.BlockTime())
+		coins := clawbackAccount.GetVestedCoins(ctx.BlockTime())
 		if coins == nil || coins.Empty() {
 			return errorsmod.Wrap(
-				vestingtypes.ErrInsufficientVestedCoins,
+				types.ErrInsufficientVestedCoins,
 				"account has no vested coins",
 			)
 		}
 
 		balance := vdd.bk.GetBalance(ctx, addr, bondDenom)
-		unvestedOnly := clawbackAccount.GetUnvestedOnly(ctx.BlockTime())
-		spendable, hasNeg := sdk.Coins{balance}.SafeSub(unvestedOnly...)
-		if hasNeg {
-			spendable = sdk.NewCoins()
+		unvestedCoins := clawbackAccount.GetVestingCoins(ctx.BlockTime())
+		// Can only delegate bondable coins
+		unvestedBondableAmt := unvestedCoins.AmountOf(bondDenom)
+		// A ClawbackVestingAccount can delegate coins from the vesting schedule
+		// when having vested locked coins or unlocked vested coins.
+		// It CANNOT delegate unvested coins
+		availableAmt := balance.Amount.Sub(unvestedBondableAmt)
+		if availableAmt.IsNegative() {
+			availableAmt = math.ZeroInt()
 		}
 
-		vested := spendable.AmountOf(bondDenom)
-		if vested.LT(delegateMsg.Amount.Amount) {
+		if availableAmt.LT(delegationAmt) {
 			return errorsmod.Wrapf(
-				vestingtypes.ErrInsufficientVestedCoins,
-				"cannot delegate unvested coins. coins vested < delegation amount (%s < %s)",
-				vested, delegateMsg.Amount.Amount,
+				types.ErrInsufficientVestedCoins,
+				"cannot delegate unvested coins. delegatable coins < delegation amount (%s < %s)",
+				availableAmt, delegationAmt,
 			)
 		}
 	}