diff --git a/PENDING.md b/PENDING.md index 8be9469675..ac3eca9b69 100644 --- a/PENDING.md +++ b/PENDING.md @@ -24,6 +24,8 @@ BREAKING CHANGES * [stake] \#2513 Validator power type from Dec -> Int * [stake] \#3233 key and value now contain duplicate fields to simplify code * [\#3064](https://github.com/cosmos/cosmos-sdk/issues/3064) Sanitize `sdk.Coin` denom. Coins denoms are now case insensitive, i.e. 100fooToken equals to 100FOOTOKEN. + * [\#3242](https://github.com/cosmos/cosmos-sdk/issues/3242) Fix infinite gas + meter utilization during aborted ante handler executions. * Tendermint diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 19944ced2f..5094934434 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -503,6 +503,7 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg } else { gasMeter = sdk.NewInfiniteGasMeter() } + app.deliverState.ctx = app.deliverState.ctx.WithBlockGasMeter(gasMeter) if app.beginBlocker != nil { @@ -732,7 +733,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk defer func() { if mode == runTxModeDeliver { ctx.BlockGasMeter().ConsumeGas( - ctx.GasMeter().GasConsumedToLimit(), "block gas meter") + ctx.GasMeter().GasConsumedToLimit(), + "block gas meter", + ) + if ctx.BlockGasMeter().GasConsumed() < startingGas { panic(sdk.ErrorGasOverflow{"tx gas summation"}) } @@ -751,23 +755,29 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk // Cache wrap context before anteHandler call in case it aborts. // This is required for both CheckTx and DeliverTx. - // https://github.com/cosmos/cosmos-sdk/issues/2772 + // Ref: https://github.com/cosmos/cosmos-sdk/issues/2772 + // // NOTE: Alternatively, we could require that anteHandler ensures that // writes do not happen if aborted/failed. This may have some // performance benefits, but it'll be more difficult to get right. anteCtx, msCache = app.cacheTxContext(ctx, txBytes) newCtx, result, abort := app.anteHandler(anteCtx, tx, (mode == runTxModeSimulate)) + if !newCtx.IsZero() { + // At this point, newCtx.MultiStore() is cache-wrapped, or something else + // replaced by the ante handler. We want the original multistore, not one + // which was cache-wrapped for the ante handler. + // + // Also, in the case of the tx aborting, we need to track gas consumed via + // the instantiated gas meter in the ante handler, so we update the context + // prior to returning. + ctx = newCtx.WithMultiStore(ms) + } + if abort { return result } - if !newCtx.IsZero() { - // At this point, newCtx.MultiStore() is cache wrapped, - // or something else replaced by anteHandler. - // We want the original ms, not one which was cache-wrapped - // for the ante handler. - ctx = newCtx.WithMultiStore(ms) - } + msCache.Write() gasWanted = result.GasWanted } diff --git a/x/auth/ante.go b/x/auth/ante.go index 7470f3198e..1008bdfa83 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -29,7 +29,10 @@ func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { // all transactions must be of type auth.StdTx stdTx, ok := tx.(StdTx) if !ok { - return ctx, sdk.ErrInternal("tx must be StdTx").Result(), true + // Set a gas meter with limit 0 as to prevent an infinite gas meter attack + // during runTx. + newCtx = SetGasMeter(simulate, ctx, 0) + return newCtx, sdk.ErrInternal("tx must be StdTx").Result(), true } params := ak.GetParams(ctx) @@ -44,7 +47,7 @@ func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { } } - newCtx = SetGasMeter(simulate, ctx, stdTx) + newCtx = SetGasMeter(simulate, ctx, stdTx.Fee.Gas) // AnteHandlers must have their own defer/recover in order for the BaseApp // to know how much gas was used! This is because the GasMeter is created in @@ -306,14 +309,14 @@ func EnsureSufficientMempoolFees(ctx sdk.Context, stdTx StdTx) sdk.Result { } // SetGasMeter returns a new context with a gas meter set from a given context. -func SetGasMeter(simulate bool, ctx sdk.Context, stdTx StdTx) sdk.Context { +func SetGasMeter(simulate bool, ctx sdk.Context, gasLimit uint64) sdk.Context { // In various cases such as simulation and during the genesis block, we do not // meter any gas utilization. if simulate || ctx.BlockHeight() == 0 { return ctx.WithGasMeter(sdk.NewInfiniteGasMeter()) } - return ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas)) + return ctx.WithGasMeter(sdk.NewGasMeter(gasLimit)) } // GetSignBytes returns a slice of bytes to sign over for a given transaction