Skip to content

Commit

Permalink
fix!: prevent 0 gas txs (#12416)
Browse files Browse the repository at this point in the history
## Description

Update `DeductFeeDecorator` to prevent 0-gas txs.

closes: #12415

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 74f265c)

# Conflicts:
#	types/errors/errors.go
#	x/auth/ante/fee_test.go
#	x/auth/ante/testutil_test.go
  • Loading branch information
alexanderbez authored and mergify[bot] committed Jul 4, 2022
1 parent 1d9f966 commit e6079fe
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 36 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 8 additions & 0 deletions types/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,15 @@ var (
// ErrAppConfig defines an error occurred if min-gas-prices field in BaseConfig is empty.
ErrAppConfig = Register(RootCodespace, 40, "error in app.toml")

<<<<<<< HEAD
// ErrPanic is only set when we recover from a panic, so we know to
// redact potentially sensitive system info
=======
// ErrInvalidGasLimit defines an error when an invalid GasWanted value is
// supplied.
ErrInvalidGasLimit = Register(RootCodespace, 41, "invalid gas limit")

// ErrPanic should only be set when we recovering from a panic
>>>>>>> 74f265c94 (fix!: prevent 0 gas txs (#12416))
ErrPanic = errorsmod.ErrPanic
)
9 changes: 9 additions & 0 deletions x/auth/ante/fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
118 changes: 86 additions & 32 deletions x/auth/ante/fee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,66 +8,104 @@ 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()

<<<<<<< HEAD
mfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, suite.app.FeeGrantKeeper, nil)
=======
mfd := ante.NewDeductFeeDecorator(s.accountKeeper, s.bankKeeper, s.feeGrantKeeper, nil)
>>>>>>> 74f265c94 (fix!: prevent 0 gas txs (#12416))
antehandler := sdk.ChainAnteDecorators(mfd)

// keys and addresses
priv1, _, addr1 := testdata.KeyTestPubAddr()
coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(300)))
<<<<<<< HEAD
testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins)
=======
testutil.FundAccount(s.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.accountKeeper, s.bankKeeper, s.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.bankKeeper, s.ctx, addr1, coins)
>>>>>>> 74f265c94 (fix!: prevent 0 gas txs (#12416))

// 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()
Expand All @@ -76,34 +114,50 @@ 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
<<<<<<< HEAD
acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr1)
suite.app.AccountKeeper.SetAccount(suite.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)

dfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, nil, nil)
=======
acc := s.accountKeeper.NewAccountWithAddress(s.ctx, addr1)
s.accountKeeper.SetAccount(s.ctx, acc)
coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(10)))
err = testutil.FundAccount(s.bankKeeper, s.ctx, addr1, coins)
s.Require().NoError(err)

dfd := ante.NewDeductFeeDecorator(s.accountKeeper, s.bankKeeper, nil, nil)
>>>>>>> 74f265c94 (fix!: prevent 0 gas txs (#12416))
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
<<<<<<< HEAD
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.accountKeeper.SetAccount(s.ctx, acc)
err = testutil.FundAccount(s.bankKeeper, s.ctx, addr1, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(200))))
s.Require().NoError(err)
>>>>>>> 74f265c94 (fix!: prevent 0 gas txs (#12416))

_, 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")
}
17 changes: 13 additions & 4 deletions x/auth/ante/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,24 @@ type AnteTestSuite struct {
txBuilder client.TxBuilder
}

<<<<<<< HEAD
// returns context and app with params set on account keeper
func createTestApp(t *testing.T, isCheckTx bool) (*simapp.SimApp, sdk.Context) {
app := simapp.Setup(t, isCheckTx)
ctx := app.BaseApp.NewContext(isCheckTx, tmproto.Header{})
app.AccountKeeper.SetParams(ctx, authtypes.DefaultParams())
=======
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) {
var (
txConfig client.TxConfig
legacyAmino *codec.LegacyAmino
)
>>>>>>> 74f265c94 (fix!: prevent 0 gas txs (#12416))

return app, ctx
}
Expand Down Expand Up @@ -194,7 +207,3 @@ func (suite *AnteTestSuite) RunTestCase(privs []cryptotypes.PrivKey, msgs []sdk.
}
})
}

func TestAnteTestSuite(t *testing.T) {
suite.Run(t, new(AnteTestSuite))
}

0 comments on commit e6079fe

Please sign in to comment.