Skip to content

Commit

Permalink
[Bugfix] limit wasm max gas with param (#502)
Browse files Browse the repository at this point in the history
* limit wasm gas usage with param

* update comment

* use explicit subCtx
  • Loading branch information
yys authored Jun 28, 2021
1 parent 505a4c8 commit c167a9d
Show file tree
Hide file tree
Showing 8 changed files with 180 additions and 36 deletions.
3 changes: 0 additions & 3 deletions x/wasm/keeper/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@ func (k Keeper) getGasRemaining(ctx sdk.Context) uint64 {
}

remaining := (meter.Limit() - meter.GasConsumed())
if maxGas := k.MaxContractGas(ctx); remaining > maxGas {
remaining = maxGas
}
return remaining * types.GasMultiplier
}

Expand Down
3 changes: 1 addition & 2 deletions x/wasm/keeper/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ func (k Keeper) dispatchMessages(ctx sdk.Context, contractAddr sdk.AccAddress, m

// dispatchMessageWithGasLimit does not emit events to prevent duplicate emission
func (k Keeper) dispatchMessageWithGasLimit(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.CosmosMsg, gasLimit uint64) (events sdk.Events, data []byte, err error) {
limitedMeter := sdk.NewGasMeter(gasLimit)
subCtx := ctx.WithGasMeter(limitedMeter)
subCtx := ctx.WithGasMeter(sdk.NewGasMeter(gasLimit))

// catch out of gas panic and just charge the entire gas limit
defer func() {
Expand Down
20 changes: 12 additions & 8 deletions x/wasm/keeper/contract.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,8 @@ func (k Keeper) InstantiateContract(
k.getGasRemaining(ctx),
)

// consume gas before raise error
k.consumeGas(ctx, gasUsed, "Contract init")
// add types.GasMultiplier to occur out of gas panic
k.consumeGas(ctx, gasUsed+types.GasMultiplier, "Contract init")
if err != nil {
return nil, nil, sdkerrors.Wrap(types.ErrInstantiateFailed, err.Error())
}
Expand Down Expand Up @@ -230,7 +230,8 @@ func (k Keeper) ExecuteContract(
k.getGasRemaining(ctx),
)

k.consumeGas(ctx, gasUsed, "Contract Execution")
// add types.GasMultiplier to occur out of gas panic
k.consumeGas(ctx, gasUsed+types.GasMultiplier, "Contract Execution")
if err != nil {
return nil, sdkerrors.Wrap(types.ErrExecuteFailed, err.Error())
}
Expand Down Expand Up @@ -306,7 +307,8 @@ func (k Keeper) MigrateContract(
k.getGasRemaining(ctx),
)

k.consumeGas(ctx, gasUsed, "Contract Migration")
// add types.GasMultiplier to occur out of gas panic
k.consumeGas(ctx, gasUsed+types.GasMultiplier, "Contract Migration")
if err != nil {
return nil, sdkerrors.Wrap(types.ErrMigrationFailed, err.Error())
}
Expand Down Expand Up @@ -367,9 +369,10 @@ func (k Keeper) reply(
k.getGasRemaining(ctx),
)

k.consumeGas(ctx, gasUsed, "Contract Reply")
// add types.GasMultiplier to occur out of gas panic
k.consumeGas(ctx, gasUsed+types.GasMultiplier, "Contract Reply")
if err != nil {
return sdkerrors.Wrap(types.ErrExecuteFailed, err.Error())
return sdkerrors.Wrap(types.ErrReplyFailed, err.Error())
}

// vaildate events is size and parse to sdk events
Expand Down Expand Up @@ -427,9 +430,10 @@ func (k Keeper) queryToContract(ctx sdk.Context, contractAddress sdk.AccAddress,
k.getGasRemaining(ctx),
)

k.consumeGas(ctx, gasUsed, "Contract Query")
// add types.GasMultiplier to occur out of gas panic
k.consumeGas(ctx, gasUsed+types.GasMultiplier, "Contract Query")
if err != nil {
err = sdkerrors.Wrap(types.ErrContractQueryFailed, err.Error())
return nil, sdkerrors.Wrap(types.ErrContractQueryFailed, err.Error())
}

return queryResult, err
Expand Down
45 changes: 36 additions & 9 deletions x/wasm/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,15 @@ func (k msgServer) InstantiateContract(goCtx context.Context, msg *types.MsgInst
}
}

eventManager := sdk.NewEventManager()
maxGas := k.MaxContractGas(ctx)
remain := ctx.GasMeter().Limit() - ctx.GasMeter().GasConsumed()
if remain > maxGas {
remain = maxGas
}

subCtx := ctx.WithEventManager(sdk.NewEventManager()).WithGasMeter(sdk.NewGasMeter(remain))
contractAddr, data, err := k.Keeper.InstantiateContract(
ctx.WithEventManager(eventManager),
subCtx,
msg.CodeID,
senderAddr,
adminAddr,
Expand All @@ -110,6 +116,9 @@ func (k msgServer) InstantiateContract(goCtx context.Context, msg *types.MsgInst
return nil, err
}

// consume gas used from wasm execution
ctx.GasMeter().ConsumeGas(subCtx.GasMeter().GasConsumed(), "wasm vm execute")

// prepend the event to keep the events order
ctx.EventManager().EmitEvents(
sdk.Events{
Expand All @@ -125,7 +134,7 @@ func (k msgServer) InstantiateContract(goCtx context.Context, msg *types.MsgInst
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender),
),
}.AppendEvents(eventManager.Events()),
}.AppendEvents(subCtx.EventManager().Events()),
)

return &types.MsgInstantiateContractResponse{
Expand All @@ -147,9 +156,15 @@ func (k msgServer) ExecuteContract(goCtx context.Context, msg *types.MsgExecuteC
return nil, err
}

eventManager := sdk.NewEventManager()
maxGas := k.MaxContractGas(ctx)
remain := ctx.GasMeter().Limit() - ctx.GasMeter().GasConsumed()
if remain > maxGas {
remain = maxGas
}

subCtx := ctx.WithEventManager(sdk.NewEventManager()).WithGasMeter(sdk.NewGasMeter(remain))
data, err := k.Keeper.ExecuteContract(
ctx.WithEventManager(eventManager),
subCtx,
contractAddr,
senderAddr,
msg.ExecuteMsg,
Expand All @@ -159,6 +174,9 @@ func (k msgServer) ExecuteContract(goCtx context.Context, msg *types.MsgExecuteC
return nil, err
}

// consume gas used from wasm execution
ctx.GasMeter().ConsumeGas(subCtx.GasMeter().GasConsumed(), "wasm vm execute")

// prepend the event to keep the events order
ctx.EventManager().EmitEvents(
sdk.Events{
Expand All @@ -172,7 +190,7 @@ func (k msgServer) ExecuteContract(goCtx context.Context, msg *types.MsgExecuteC
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender),
),
}.AppendEvents(eventManager.Events()),
}.AppendEvents(subCtx.EventManager().Events()),
)

return &types.MsgExecuteContractResponse{
Expand All @@ -193,9 +211,15 @@ func (k msgServer) MigrateContract(goCtx context.Context, msg *types.MsgMigrateC
return nil, err
}

eventManager := sdk.NewEventManager()
maxGas := k.MaxContractGas(ctx)
remain := ctx.GasMeter().Limit() - ctx.GasMeter().GasConsumed()
if remain > maxGas {
remain = maxGas
}

subCtx := ctx.WithEventManager(sdk.NewEventManager()).WithGasMeter(sdk.NewGasMeter(remain))
data, err := k.Keeper.MigrateContract(
ctx.WithEventManager(eventManager),
subCtx,
contractAddr,
adminAddr,
msg.NewCodeID,
Expand All @@ -205,6 +229,9 @@ func (k msgServer) MigrateContract(goCtx context.Context, msg *types.MsgMigrateC
return nil, err
}

// consume gas used from wasm execution
ctx.GasMeter().ConsumeGas(subCtx.GasMeter().GasConsumed(), "wasm vm execute")

// prepend the event to keep the events order
ctx.EventManager().EmitEvents(
sdk.Events{
Expand All @@ -218,7 +245,7 @@ func (k msgServer) MigrateContract(goCtx context.Context, msg *types.MsgMigrateC
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
sdk.NewAttribute(sdk.AttributeKeySender, msg.Admin),
),
}.AppendEvents(eventManager.Events()),
}.AppendEvents(subCtx.EventManager().Events()),
)

return &types.MsgMigrateContractResponse{
Expand Down
116 changes: 116 additions & 0 deletions x/wasm/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package keeper

import (
"encoding/json"
"io/ioutil"
"testing"

"github.com/stretchr/testify/require"
core "github.com/terra-money/core/types"
"github.com/terra-money/core/x/wasm/types"

sdk "github.com/cosmos/cosmos-sdk/types"
)

func TestInstantiateExceedMaxGas(t *testing.T) {
input := CreateTestInput(t)
ctx, accKeeper, bankKeeper, keeper := input.Ctx, input.AccKeeper, input.BankKeeper, input.WasmKeeper

deposit := sdk.NewCoins(sdk.NewInt64Coin(core.MicroLunaDenom, 100000))
creator := createFakeFundedAccount(ctx, accKeeper, bankKeeper, deposit)

wasmCode, err := ioutil.ReadFile("./testdata/hackatom.wasm")
require.NoError(t, err)

codeID, err := keeper.StoreCode(ctx, creator, wasmCode)
require.NoError(t, err)

_, _, bob := keyPubAddr()
_, _, fred := keyPubAddr()

initMsg := HackatomExampleInitMsg{
Verifier: fred,
Beneficiary: bob,
}

initMsgBz, err := json.Marshal(initMsg)
require.NoError(t, err)

// must panic
require.Panics(t, func() {
params := keeper.GetParams(ctx)
params.MaxContractGas = types.InstanceCost + 1
keeper.SetParams(ctx, params)
NewMsgServerImpl(keeper).InstantiateContract(ctx.Context(), types.NewMsgInstantiateContract(creator, sdk.AccAddress{}, codeID, initMsgBz, nil))
})
}

func TestExecuteExceedMaxGas(t *testing.T) {
input := CreateTestInput(t)
ctx, accKeeper, bankKeeper, keeper := input.Ctx, input.AccKeeper, input.BankKeeper, input.WasmKeeper

deposit := sdk.NewCoins(sdk.NewInt64Coin(core.MicroLunaDenom, 100000))
creator := createFakeFundedAccount(ctx, accKeeper, bankKeeper, deposit)

wasmCode, err := ioutil.ReadFile("./testdata/hackatom.wasm")
require.NoError(t, err)

codeID, err := keeper.StoreCode(ctx, creator, wasmCode)
require.NoError(t, err)

_, _, bob := keyPubAddr()
_, _, fred := keyPubAddr()

initMsg := HackatomExampleInitMsg{
Verifier: fred,
Beneficiary: bob,
}

initMsgBz, err := json.Marshal(initMsg)
require.NoError(t, err)

addr, _, err := keeper.InstantiateContract(ctx, codeID, creator, sdk.AccAddress{}, initMsgBz, nil)

// must panic
require.Panics(t, func() {
params := keeper.GetParams(ctx)
params.MaxContractGas = types.InstanceCost + 1
keeper.SetParams(ctx, params)
NewMsgServerImpl(keeper).ExecuteContract(ctx.Context(), types.NewMsgExecuteContract(creator, addr, []byte(`{"release":{}}`), nil))
})
}

func TestMigrateExceedMaxGas(t *testing.T) {
input := CreateTestInput(t)
ctx, accKeeper, bankKeeper, keeper := input.Ctx, input.AccKeeper, input.BankKeeper, input.WasmKeeper

deposit := sdk.NewCoins(sdk.NewInt64Coin(core.MicroLunaDenom, 100000))
creator := createFakeFundedAccount(ctx, accKeeper, bankKeeper, deposit)

wasmCode, err := ioutil.ReadFile("./testdata/hackatom.wasm")
require.NoError(t, err)

codeID, err := keeper.StoreCode(ctx, creator, wasmCode)
require.NoError(t, err)

_, _, bob := keyPubAddr()
_, _, fred := keyPubAddr()

initMsg := HackatomExampleInitMsg{
Verifier: fred,
Beneficiary: bob,
}

initMsgBz, err := json.Marshal(initMsg)
require.NoError(t, err)

addr, _, err := keeper.InstantiateContract(ctx, codeID, creator, sdk.AccAddress{}, initMsgBz, nil)

// must panic
require.Panics(t, func() {
params := keeper.GetParams(ctx)
params.MaxContractGas = types.InstanceCost + 1
keeper.SetParams(ctx, params)
NewMsgServerImpl(keeper).MigrateContract(ctx.Context(), types.NewMsgMigrateContract(creator, addr, codeID, []byte(`{"release":{}}`)))
})
}
18 changes: 9 additions & 9 deletions x/wasm/keeper/recursive_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func initRecurseContract(t *testing.T) (contract sdk.AccAddress, creator sdk.Acc
}

func TestGasCostOnQuery(t *testing.T) {
GasNoWork := types.InstanceCost + 4_372
GasNoWork := types.InstanceCost + 3_343
// Note: about 100 SDK gas (10k wasmVM gas) for each round of sha256
GasWork50 := GasNoWork + 5_692 // this is a little shy of 50k gas - to keep an eye on the limit

Expand Down Expand Up @@ -182,14 +182,14 @@ func TestGasCostOnQuery(t *testing.T) {
}

func TestGasOnExternalQuery(t *testing.T) {
GasNoWork := types.InstanceCost + 4_372
GasNoWork := types.InstanceCost + 3_343
// Note: about 100 SDK gas (10k wasmVM gas) for each round of sha256
GasWork50 := GasNoWork + 5_692 // this is a little shy of 50k gas - to keep an eye on the limit

cases := map[string]struct {
gasLimit uint64
msg Recurse
expectPanic bool
gasLimit uint64
msg Recurse
expectOutOfGas bool
}{
"no recursion, plenty gas": {
gasLimit: 400_000,
Expand All @@ -210,7 +210,7 @@ func TestGasOnExternalQuery(t *testing.T) {
msg: Recurse{
Work: 50,
},
expectPanic: true,
expectOutOfGas: true,
},
"recursion 4, external gas limit": {
// this uses 244708 gas but give less
Expand All @@ -219,7 +219,7 @@ func TestGasOnExternalQuery(t *testing.T) {
Depth: 4,
Work: 50,
},
expectPanic: true,
expectOutOfGas: true,
},
}

Expand All @@ -239,7 +239,7 @@ func TestGasOnExternalQuery(t *testing.T) {
bz, err := cdc.MarshalJSON(types.NewQueryContractParams(contractAddr, msg))
require.NoError(t, err)

if tc.expectPanic {
if tc.expectOutOfGas {
_, err = querier(ctx, []string{types.QueryContractStore}, abci.RequestQuery{Data: []byte(bz)})
require.Error(t, err)
require.Contains(t, err.Error(), sdkerror.ErrOutOfGas.Error())
Expand All @@ -260,7 +260,7 @@ func TestLimitRecursiveQueryGas(t *testing.T) {
// This attack would allow us to use far more than the provided gas before
// eventually hitting an OutOfGas panic.

GasNoWork := types.InstanceCost + 4_372
GasNoWork := types.InstanceCost + 3_343
GasWork2k := GasNoWork + 229_495

// This is overhead for calling into a sub-contract
Expand Down
Loading

0 comments on commit c167a9d

Please sign in to comment.