From b0d3ef96506054b66bc1160d48f4b24105e7d35a Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Fri, 7 Jan 2022 19:25:26 +0100 Subject: [PATCH] fix: use full gas on overflow (#10897) ## Description Investigating missing gas consumption --- ### 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/master/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/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/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) --- CHANGELOG.md | 1 + store/types/gas.go | 2 +- x/auth/middleware/fee.go | 16 ++++++++-------- x/auth/middleware/middleware.go | 2 ++ 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c92685869160..4ff953137c72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,6 +188,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ibc-denom. * [\#10593](https://github.com/cosmos/cosmos-sdk/pull/10593) Update swagger-ui to v4.1.0 to fix xss vulnerability. * [\#10674](https://github.com/cosmos/cosmos-sdk/pull/10674) Fix issue with `Error.Wrap` and `Error.Wrapf` usage with `errors.Is`. +* [\#10897](https://github.com/cosmos/cosmos-sdk/pull/10897) Fix: set a non-zero value on gas overflow. ### State Machine Breaking diff --git a/store/types/gas.go b/store/types/gas.go index e3902426bf70..84ca542550b7 100644 --- a/store/types/gas.go +++ b/store/types/gas.go @@ -105,9 +105,9 @@ func addUint64Overflow(a, b uint64) (uint64, bool) { // ConsumeGas adds the given amount of gas to the gas consumed and panics if it overflows the limit or out of gas. func (g *basicGasMeter) ConsumeGas(amount Gas, descriptor string) { var overflow bool - // TODO: Should we set the consumed field after overflow checking? g.consumed, overflow = addUint64Overflow(g.consumed, amount) if overflow { + g.consumed = math.MaxUint64 panic(ErrorGasOverflow{descriptor}) } diff --git a/x/auth/middleware/fee.go b/x/auth/middleware/fee.go index 426631b60d43..718afba479d1 100644 --- a/x/auth/middleware/fee.go +++ b/x/auth/middleware/fee.go @@ -110,7 +110,7 @@ func (dfd deductFeeTxHandler) checkDeductFee(ctx context.Context, sdkTx sdk.Tx) } if addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName); addr == nil { - panic(fmt.Sprintf("%s module account has not been set", types.FeeCollectorName)) + return fmt.Errorf("Fee collector module account (%s) has not been set", types.FeeCollectorName) } fee := feeTx.GetFee() @@ -123,12 +123,11 @@ func (dfd deductFeeTxHandler) checkDeductFee(ctx context.Context, sdkTx sdk.Tx) // this works with only when feegrant enabled. if feeGranter != nil { if dfd.feegrantKeeper == nil { - return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "fee grants are not enabled") + return sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled") } else if !feeGranter.Equals(feePayer) { err := dfd.feegrantKeeper.UseGrantedFees(sdkCtx, feeGranter, feePayer, fee, sdkTx.GetMsgs()) - if err != nil { - return sdkerrors.Wrapf(err, "%s not allowed to pay fees from %s", feeGranter, feePayer) + return sdkerrors.Wrapf(err, "%s does not not allow to pay fees for %s", feeGranter, feePayer) } } @@ -137,7 +136,7 @@ func (dfd deductFeeTxHandler) checkDeductFee(ctx context.Context, sdkTx sdk.Tx) deductFeesFromAcc := dfd.accountKeeper.GetAccount(sdkCtx, deductFeesFrom) if deductFeesFromAcc == nil { - return sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "fee payer address: %s does not exist", deductFeesFrom) + return sdkerrors.ErrUnknownAddress.Wrapf("fee payer address: %s does not exist", deductFeesFrom) } // deduct the fees @@ -182,15 +181,16 @@ func (dfd deductFeeTxHandler) SimulateTx(ctx context.Context, req tx.Request) (t return dfd.next.SimulateTx(ctx, req) } -// DeductFees deducts fees from the given account. +// Deprecated: DeductFees deducts fees from the given account. +// This function will be private in the next release. func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc types.AccountI, fees sdk.Coins) error { if !fees.IsValid() { - return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "invalid fee amount: %s", fees) + return sdkerrors.ErrInsufficientFee.Wrapf("invalid fee amount: %s", fees) } err := bankKeeper.SendCoinsFromAccountToModule(ctx, acc.GetAddress(), types.FeeCollectorName, fees) if err != nil { - return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error()) + return sdkerrors.ErrInsufficientFunds.Wrap(err.Error()) } return nil diff --git a/x/auth/middleware/middleware.go b/x/auth/middleware/middleware.go index 53d16938e219..80a70765dc85 100644 --- a/x/auth/middleware/middleware.go +++ b/x/auth/middleware/middleware.go @@ -97,6 +97,8 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) { TxTimeoutHeightMiddleware, ValidateMemoMiddleware(options.AccountKeeper), ConsumeTxSizeGasMiddleware(options.AccountKeeper), + // No gas should be consumed in any middleware above in a "post" handler part. See + // ComposeMiddlewares godoc for details. DeductFeeMiddleware(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper), TxPriorityMiddleware, SetPubKeyMiddleware(options.AccountKeeper),