From 397119f942de02e635c6293fbe564d9146f249c5 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 4 Jul 2022 10:49:42 -0400 Subject: [PATCH] fix!: prevent 0 gas txs (backport #12416) (#12432) --- CHANGELOG.md | 1 + types/errors/errors.go | 4 ++ x/auth/ante/fee.go | 9 +++ x/auth/ante/fee_test.go | 114 ++++++++++++++++++++++------------- x/auth/ante/testutil_test.go | 91 ++++++++++++++-------------- 5 files changed, 131 insertions(+), 88 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cb59207d500..946ecbb2dc6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* [#12416](https://github.com/cosmos/cosmos-sdk/pull/12416) Prevent zero gas transactions in the `DeductFeeDecorator` AnteHandler decorator. * (x/mint) [#12384](https://github.com/cosmos/cosmos-sdk/pull/12384) Ensure `GoalBonded` must be positive when performing `x/mint` parameter validation. * (x/auth) [#12261](https://github.com/cosmos/cosmos-sdk/pull/12261) Deprecate pagination in GetTxsEventRequest/Response in favor of page and limit to align with tendermint `SignClient.TxSearch` * (vesting) [#12190](https://github.com/cosmos/cosmos-sdk/pull/12190) Replace https://github.com/cosmos/cosmos-sdk/pull/12190 to use `NewBaseAccountWithAddress` in all vesting account message handlers. diff --git a/types/errors/errors.go b/types/errors/errors.go index f16464eca45b..e7c5cae28bff 100644 --- a/types/errors/errors.go +++ b/types/errors/errors.go @@ -162,6 +162,10 @@ var ( // ErrAppConfig defines an error occurred if min-gas-prices field in BaseConfig is empty. ErrAppConfig = Register(RootCodespace, 40, "error in app.toml") + // ErrInvalidGasLimit defines an error when an invalid GasWanted value is + // supplied. + ErrInvalidGasLimit = Register(RootCodespace, 41, "invalid gas limit") + // ErrPanic is only set when we recover from a panic, so we know to // redact potentially sensitive system info ErrPanic = errorsmod.ErrPanic diff --git a/x/auth/ante/fee.go b/x/auth/ante/fee.go index 7a38dbc399d6..2325fc5f672e 100644 --- a/x/auth/ante/fee.go +++ b/x/auth/ante/fee.go @@ -37,6 +37,15 @@ func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKee } func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + feeTx, ok := tx.(sdk.FeeTx) + if !ok { + return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") + } + + if ctx.BlockHeight() > 0 && feeTx.GetGas() == 0 { + return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidGasLimit, "must provide positive gas") + } + fee, priority, err := dfd.txFeeChecker(ctx, tx) if err != nil { return ctx, err diff --git a/x/auth/ante/fee_test.go b/x/auth/ante/fee_test.go index 21a0a2d7ff8e..9ecd75cbed45 100644 --- a/x/auth/ante/fee_test.go +++ b/x/auth/ante/fee_test.go @@ -8,66 +8,96 @@ import ( "github.com/cosmos/cosmos-sdk/x/bank/testutil" ) -func (suite *AnteTestSuite) TestEnsureMempoolFees() { - suite.SetupTest(true) // setup - suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() +func (s *AnteTestSuite) TestDeductFeeDecorator_ZeroGas() { + s.SetupTest(true) // setup + s.txBuilder = s.clientCtx.TxConfig.NewTxBuilder() - mfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, suite.app.FeeGrantKeeper, nil) + mfd := ante.NewDeductFeeDecorator(s.app.AccountKeeper, s.app.BankKeeper, s.app.FeeGrantKeeper, nil) antehandler := sdk.ChainAnteDecorators(mfd) // keys and addresses priv1, _, addr1 := testdata.KeyTestPubAddr() coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(300))) - testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins) + testutil.FundAccount(s.app.BankKeeper, s.ctx, addr1, coins) + + // msg and signatures + msg := testdata.NewTestMsg(addr1) + s.Require().NoError(s.txBuilder.SetMsgs(msg)) + + // set zero gas + s.txBuilder.SetGasLimit(0) + + privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0} + tx, err := s.CreateTestTx(privs, accNums, accSeqs, s.ctx.ChainID()) + s.Require().NoError(err) + + // Set IsCheckTx to true + s.ctx = s.ctx.WithIsCheckTx(true) + + _, err = antehandler(s.ctx, tx, false) + s.Require().Error(err) +} + +func (s *AnteTestSuite) TestEnsureMempoolFees() { + s.SetupTest(true) // setup + s.txBuilder = s.clientCtx.TxConfig.NewTxBuilder() + + mfd := ante.NewDeductFeeDecorator(s.app.AccountKeeper, s.app.BankKeeper, s.app.FeeGrantKeeper, nil) + antehandler := sdk.ChainAnteDecorators(mfd) + + // keys and addresses + priv1, _, addr1 := testdata.KeyTestPubAddr() + coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(300))) + testutil.FundAccount(s.app.BankKeeper, s.ctx, addr1, coins) // msg and signatures msg := testdata.NewTestMsg(addr1) feeAmount := testdata.NewTestFeeAmount() gasLimit := testdata.NewTestGasLimit() - suite.Require().NoError(suite.txBuilder.SetMsgs(msg)) - suite.txBuilder.SetFeeAmount(feeAmount) - suite.txBuilder.SetGasLimit(gasLimit) + s.Require().NoError(s.txBuilder.SetMsgs(msg)) + s.txBuilder.SetFeeAmount(feeAmount) + s.txBuilder.SetGasLimit(gasLimit) privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0} - tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID()) - suite.Require().NoError(err) + tx, err := s.CreateTestTx(privs, accNums, accSeqs, s.ctx.ChainID()) + s.Require().NoError(err) // Set high gas price so standard test fee fails atomPrice := sdk.NewDecCoinFromDec("atom", sdk.NewDec(200).Quo(sdk.NewDec(100000))) highGasPrice := []sdk.DecCoin{atomPrice} - suite.ctx = suite.ctx.WithMinGasPrices(highGasPrice) + s.ctx = s.ctx.WithMinGasPrices(highGasPrice) // Set IsCheckTx to true - suite.ctx = suite.ctx.WithIsCheckTx(true) + s.ctx = s.ctx.WithIsCheckTx(true) // antehandler errors with insufficient fees - _, err = antehandler(suite.ctx, tx, false) - suite.Require().NotNil(err, "Decorator should have errored on too low fee for local gasPrice") + _, err = antehandler(s.ctx, tx, false) + s.Require().NotNil(err, "Decorator should have errored on too low fee for local gasPrice") // Set IsCheckTx to false - suite.ctx = suite.ctx.WithIsCheckTx(false) + s.ctx = s.ctx.WithIsCheckTx(false) // antehandler should not error since we do not check minGasPrice in DeliverTx - _, err = antehandler(suite.ctx, tx, false) - suite.Require().Nil(err, "MempoolFeeDecorator returned error in DeliverTx") + _, err = antehandler(s.ctx, tx, false) + s.Require().Nil(err, "MempoolFeeDecorator returned error in DeliverTx") // Set IsCheckTx back to true for testing sufficient mempool fee - suite.ctx = suite.ctx.WithIsCheckTx(true) + s.ctx = s.ctx.WithIsCheckTx(true) atomPrice = sdk.NewDecCoinFromDec("atom", sdk.NewDec(0).Quo(sdk.NewDec(100000))) lowGasPrice := []sdk.DecCoin{atomPrice} - suite.ctx = suite.ctx.WithMinGasPrices(lowGasPrice) + s.ctx = s.ctx.WithMinGasPrices(lowGasPrice) - newCtx, err := antehandler(suite.ctx, tx, false) - suite.Require().Nil(err, "Decorator should not have errored on fee higher than local gasPrice") + newCtx, err := antehandler(s.ctx, tx, false) + s.Require().Nil(err, "Decorator should not have errored on fee higher than local gasPrice") // Priority is the smallest amount in any denom. Since we have only 1 fee // of 150atom, the priority here is 150. - suite.Require().Equal(feeAmount.AmountOf("atom").Int64(), newCtx.Priority()) + s.Require().Equal(feeAmount.AmountOf("atom").Int64(), newCtx.Priority()) } -func (suite *AnteTestSuite) TestDeductFees() { - suite.SetupTest(false) // setup - suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() +func (s *AnteTestSuite) TestDeductFees() { + s.SetupTest(false) // setup + s.txBuilder = s.clientCtx.TxConfig.NewTxBuilder() // keys and addresses priv1, _, addr1 := testdata.KeyTestPubAddr() @@ -76,34 +106,34 @@ func (suite *AnteTestSuite) TestDeductFees() { msg := testdata.NewTestMsg(addr1) feeAmount := testdata.NewTestFeeAmount() gasLimit := testdata.NewTestGasLimit() - suite.Require().NoError(suite.txBuilder.SetMsgs(msg)) - suite.txBuilder.SetFeeAmount(feeAmount) - suite.txBuilder.SetGasLimit(gasLimit) + s.Require().NoError(s.txBuilder.SetMsgs(msg)) + s.txBuilder.SetFeeAmount(feeAmount) + s.txBuilder.SetGasLimit(gasLimit) privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0} - tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID()) - suite.Require().NoError(err) + tx, err := s.CreateTestTx(privs, accNums, accSeqs, s.ctx.ChainID()) + s.Require().NoError(err) // Set account with insufficient funds - acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr1) - suite.app.AccountKeeper.SetAccount(suite.ctx, acc) + acc := s.app.AccountKeeper.NewAccountWithAddress(s.ctx, addr1) + s.app.AccountKeeper.SetAccount(s.ctx, acc) coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(10))) - err = testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins) - suite.Require().NoError(err) + err = testutil.FundAccount(s.app.BankKeeper, s.ctx, addr1, coins) + s.Require().NoError(err) - dfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, nil, nil) + dfd := ante.NewDeductFeeDecorator(s.app.AccountKeeper, s.app.BankKeeper, nil, nil) antehandler := sdk.ChainAnteDecorators(dfd) - _, err = antehandler(suite.ctx, tx, false) + _, err = antehandler(s.ctx, tx, false) - suite.Require().NotNil(err, "Tx did not error when fee payer had insufficient funds") + s.Require().NotNil(err, "Tx did not error when fee payer had insufficient funds") // Set account with sufficient funds - suite.app.AccountKeeper.SetAccount(suite.ctx, acc) - err = testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(200)))) - suite.Require().NoError(err) + s.app.AccountKeeper.SetAccount(s.ctx, acc) + err = testutil.FundAccount(s.app.BankKeeper, s.ctx, addr1, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(200)))) + s.Require().NoError(err) - _, err = antehandler(suite.ctx, tx, false) + _, err = antehandler(s.ctx, tx, false) - suite.Require().Nil(err, "Tx errored after account has been set with sufficient funds") + s.Require().Nil(err, "Tx errored after account has been set with sufficient funds") } diff --git a/x/auth/ante/testutil_test.go b/x/auth/ante/testutil_test.go index 15598b3b23b0..62e577b6a8f1 100644 --- a/x/auth/ante/testutil_test.go +++ b/x/auth/ante/testutil_test.go @@ -5,8 +5,6 @@ import ( "fmt" "testing" - minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" - "github.com/stretchr/testify/suite" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" @@ -21,6 +19,7 @@ import ( xauthsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" "github.com/cosmos/cosmos-sdk/x/auth/types" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" ) // TestAccount represents an account used in the tests in x/auth/ante. @@ -49,10 +48,14 @@ func createTestApp(t *testing.T, isCheckTx bool) (*simapp.SimApp, sdk.Context) { return app, ctx } +func TestAnteTestSuite(t *testing.T) { + suite.Run(t, new(AnteTestSuite)) +} + // SetupTest setups a new test, with new app, context, and anteHandler. -func (suite *AnteTestSuite) SetupTest(isCheckTx bool) { - suite.app, suite.ctx = createTestApp(suite.T(), isCheckTx) - suite.ctx = suite.ctx.WithBlockHeight(1) +func (s *AnteTestSuite) SetupTest(isCheckTx bool) { + s.app, s.ctx = createTestApp(s.T(), isCheckTx) + s.ctx = s.ctx.WithBlockHeight(1) // Set up TxConfig. encodingConfig := simapp.MakeTestEncodingConfig() @@ -60,42 +63,42 @@ func (suite *AnteTestSuite) SetupTest(isCheckTx bool) { encodingConfig.Amino.RegisterConcrete(&testdata.TestMsg{}, "testdata.TestMsg", nil) testdata.RegisterInterfaces(encodingConfig.InterfaceRegistry) - suite.clientCtx = client.Context{}. + s.clientCtx = client.Context{}. WithTxConfig(encodingConfig.TxConfig) anteHandler, err := ante.NewAnteHandler( ante.HandlerOptions{ - AccountKeeper: suite.app.AccountKeeper, - BankKeeper: suite.app.BankKeeper, - FeegrantKeeper: suite.app.FeeGrantKeeper, + AccountKeeper: s.app.AccountKeeper, + BankKeeper: s.app.BankKeeper, + FeegrantKeeper: s.app.FeeGrantKeeper, SignModeHandler: encodingConfig.TxConfig.SignModeHandler(), SigGasConsumer: ante.DefaultSigVerificationGasConsumer, }, ) - suite.Require().NoError(err) - suite.anteHandler = anteHandler + s.Require().NoError(err) + s.anteHandler = anteHandler } // CreateTestAccounts creates `numAccs` accounts, and return all relevant // information about them including their private keys. -func (suite *AnteTestSuite) CreateTestAccounts(numAccs int) []TestAccount { +func (s *AnteTestSuite) CreateTestAccounts(numAccs int) []TestAccount { var accounts []TestAccount for i := 0; i < numAccs; i++ { priv, _, addr := testdata.KeyTestPubAddr() - acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr) + acc := s.app.AccountKeeper.NewAccountWithAddress(s.ctx, addr) err := acc.SetAccountNumber(uint64(i)) - suite.Require().NoError(err) - suite.app.AccountKeeper.SetAccount(suite.ctx, acc) + s.Require().NoError(err) + s.app.AccountKeeper.SetAccount(s.ctx, acc) someCoins := sdk.Coins{ sdk.NewInt64Coin("atom", 10000000), } - err = suite.app.BankKeeper.MintCoins(suite.ctx, minttypes.ModuleName, someCoins) - suite.Require().NoError(err) + err = s.app.BankKeeper.MintCoins(s.ctx, minttypes.ModuleName, someCoins) + s.Require().NoError(err) - err = suite.app.BankKeeper.SendCoinsFromModuleToAccount(suite.ctx, minttypes.ModuleName, addr, someCoins) - suite.Require().NoError(err) + err = s.app.BankKeeper.SendCoinsFromModuleToAccount(s.ctx, minttypes.ModuleName, addr, someCoins) + s.Require().NoError(err) accounts = append(accounts, TestAccount{acc, priv}) } @@ -104,7 +107,7 @@ func (suite *AnteTestSuite) CreateTestAccounts(numAccs int) []TestAccount { } // CreateTestTx is a helper function to create a tx given multiple inputs. -func (suite *AnteTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums []uint64, accSeqs []uint64, chainID string) (xauthsigning.Tx, error) { +func (s *AnteTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums []uint64, accSeqs []uint64, chainID string) (xauthsigning.Tx, error) { // First round: we gather all the signer infos. We use the "set empty // signature" hack to do that. var sigsV2 []signing.SignatureV2 @@ -112,7 +115,7 @@ func (suite *AnteTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums [] sigV2 := signing.SignatureV2{ PubKey: priv.PubKey(), Data: &signing.SingleSignatureData{ - SignMode: suite.clientCtx.TxConfig.SignModeHandler().DefaultMode(), + SignMode: s.clientCtx.TxConfig.SignModeHandler().DefaultMode(), Signature: nil, }, Sequence: accSeqs[i], @@ -120,7 +123,7 @@ func (suite *AnteTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums [] sigsV2 = append(sigsV2, sigV2) } - err := suite.txBuilder.SetSignatures(sigsV2...) + err := s.txBuilder.SetSignatures(sigsV2...) if err != nil { return nil, err } @@ -134,20 +137,20 @@ func (suite *AnteTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums [] Sequence: accSeqs[i], } sigV2, err := tx.SignWithPrivKey( - suite.clientCtx.TxConfig.SignModeHandler().DefaultMode(), signerData, - suite.txBuilder, priv, suite.clientCtx.TxConfig, accSeqs[i]) + s.clientCtx.TxConfig.SignModeHandler().DefaultMode(), signerData, + s.txBuilder, priv, s.clientCtx.TxConfig, accSeqs[i]) if err != nil { return nil, err } sigsV2 = append(sigsV2, sigV2) } - err = suite.txBuilder.SetSignatures(sigsV2...) + err = s.txBuilder.SetSignatures(sigsV2...) if err != nil { return nil, err } - return suite.txBuilder.GetTx(), nil + return s.txBuilder.GetTx(), nil } // TestCase represents a test case used in test tables. @@ -160,41 +163,37 @@ type TestCase struct { } // CreateTestTx is a helper function to create a tx given multiple inputs. -func (suite *AnteTestSuite) RunTestCase(privs []cryptotypes.PrivKey, msgs []sdk.Msg, feeAmount sdk.Coins, gasLimit uint64, accNums, accSeqs []uint64, chainID string, tc TestCase) { - suite.Run(fmt.Sprintf("Case %s", tc.desc), func() { - suite.Require().NoError(suite.txBuilder.SetMsgs(msgs...)) - suite.txBuilder.SetFeeAmount(feeAmount) - suite.txBuilder.SetGasLimit(gasLimit) +func (s *AnteTestSuite) RunTestCase(privs []cryptotypes.PrivKey, msgs []sdk.Msg, feeAmount sdk.Coins, gasLimit uint64, accNums, accSeqs []uint64, chainID string, tc TestCase) { + s.Run(fmt.Sprintf("Case %s", tc.desc), func() { + s.Require().NoError(s.txBuilder.SetMsgs(msgs...)) + s.txBuilder.SetFeeAmount(feeAmount) + s.txBuilder.SetGasLimit(gasLimit) // Theoretically speaking, ante handler unit tests should only test // ante handlers, but here we sometimes also test the tx creation // process. - tx, txErr := suite.CreateTestTx(privs, accNums, accSeqs, chainID) - newCtx, anteErr := suite.anteHandler(suite.ctx, tx, tc.simulate) + tx, txErr := s.CreateTestTx(privs, accNums, accSeqs, chainID) + newCtx, anteErr := s.anteHandler(s.ctx, tx, tc.simulate) if tc.expPass { - suite.Require().NoError(txErr) - suite.Require().NoError(anteErr) - suite.Require().NotNil(newCtx) + s.Require().NoError(txErr) + s.Require().NoError(anteErr) + s.Require().NotNil(newCtx) - suite.ctx = newCtx + s.ctx = newCtx } else { switch { case txErr != nil: - suite.Require().Error(txErr) - suite.Require().True(errors.Is(txErr, tc.expErr)) + s.Require().Error(txErr) + s.Require().True(errors.Is(txErr, tc.expErr)) case anteErr != nil: - suite.Require().Error(anteErr) - suite.Require().True(errors.Is(anteErr, tc.expErr)) + s.Require().Error(anteErr) + s.Require().True(errors.Is(anteErr, tc.expErr)) default: - suite.Fail("expected one of txErr,anteErr to be an error") + s.Fail("expected one of txErr,anteErr to be an error") } } }) } - -func TestAnteTestSuite(t *testing.T) { - suite.Run(t, new(AnteTestSuite)) -}