Merge pull request #233 from CosmWasm/restrict-querier-redispatched-gas

Restrict querier redispatched gas

Author: ethanfrey

go.mod

@@ -3,7 +3,7 @@ module github.com/CosmWasm/wasmd
 go 1.13
 
 require (
-	github.com/CosmWasm/go-cosmwasm v0.10.0-alpha2
+	github.com/CosmWasm/go-cosmwasm v0.10.0-alpha4
 	github.com/cosmos/cosmos-sdk v0.39.1-0.20200727135228-9d00f712e334
 	github.com/golang/mock v1.4.3 // indirect
 	github.com/google/gofuzz v1.0.0

go.sum

@@ -9,8 +9,8 @@ github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/ChainSafe/go-schnorrkel v0.0.0-20200405005733-88cbf1b4c40d h1:nalkkPQcITbvhmL4+C4cKA87NW0tfm3Kl9VXRoPywFg=
 github.com/ChainSafe/go-schnorrkel v0.0.0-20200405005733-88cbf1b4c40d/go.mod h1:URdX5+vg25ts3aCh8H5IFZybJYKWhJHYMTnf+ULtoC4=
-github.com/CosmWasm/go-cosmwasm v0.10.0-alpha2 h1:k069wQF0f24w3A52mjR3AK6AfkuvQ5d0Mdu/rpB5nEk=
-github.com/CosmWasm/go-cosmwasm v0.10.0-alpha2/go.mod h1:gAFCwllx97ejI+m9SqJQrmd2SBW7HA0fOjvWWJjM2uc=
+github.com/CosmWasm/go-cosmwasm v0.10.0-alpha4 h1:1a3j/vdhnyYvUV+67Hg3GU87M9wn1jR6bReXDwy+TZQ=
+github.com/CosmWasm/go-cosmwasm v0.10.0-alpha4/go.mod h1:gAFCwllx97ejI+m9SqJQrmd2SBW7HA0fOjvWWJjM2uc=
 github.com/Knetic/govaluate v3.0.1-0.20171022003610-9aa49832a739+incompatible/go.mod h1:r7JcOSlj0wfOMncg0iLm8Leh48TZaKVeNIfJntJ2wa0=
 github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
 github.com/Shopify/sarama v1.19.0/go.mod h1:FVkBWblsNy7DGZRfXLU0O9RCGt5g3g3yEuWXgklEdEo=

x/wasm/internal/keeper/query_plugins.go

@@ -2,7 +2,6 @@ package keeper
 
 import (
 	"encoding/json"
-
 	wasmTypes "github.com/CosmWasm/go-cosmwasm/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -17,18 +16,28 @@ type QueryHandler struct {
 
 var _ wasmTypes.Querier = QueryHandler{}
 
-func (q QueryHandler) Query(request wasmTypes.QueryRequest) ([]byte, error) {
+func (q QueryHandler) Query(request wasmTypes.QueryRequest, gasLimit uint64) ([]byte, error) {
+	// set a limit for a subctx
+	sdkGas := gasLimit / GasMultiplier
+	subctx := q.Ctx.WithGasMeter(sdk.NewGasMeter(sdkGas))
+
+	// make sure we charge the higher level context even on panic
+	defer func() {
+		q.Ctx.GasMeter().ConsumeGas(subctx.GasMeter().GasConsumed(), "contract sub-query")
+	}()
+
+	// do the query
 	if request.Bank != nil {
-		return q.Plugins.Bank(q.Ctx, request.Bank)
+		return q.Plugins.Bank(subctx, request.Bank)
 	}
 	if request.Custom != nil {
-		return q.Plugins.Custom(q.Ctx, request.Custom)
+		return q.Plugins.Custom(subctx, request.Custom)
 	}
 	if request.Staking != nil {
-		return q.Plugins.Staking(q.Ctx, request.Staking)
+		return q.Plugins.Staking(subctx, request.Staking)
 	}
 	if request.Wasm != nil {
-		return q.Plugins.Wasm(q.Ctx, request.Wasm)
+		return q.Plugins.Wasm(subctx, request.Wasm)
 	}
 	return nil, wasmTypes.Unknown{}
 }

x/wasm/internal/keeper/recurse_test.go

@@ -2,15 +2,16 @@ package keeper
 
 import (
 	"encoding/json"
-	abci "github.com/tendermint/tendermint/abci/types"
 	"io/ioutil"
 	"os"
 	"testing"
 
+	wasmTypes "github.com/CosmWasm/go-cosmwasm/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	abci "github.com/tendermint/tendermint/abci/types"
 )
 
 type Recurse struct {
@@ -34,6 +35,50 @@ type recurseResponse struct {
 	Hashed []byte `json:"hashed"`
 }
 
+// number os wasm queries called from a contract
+var totalWasmQueryCounter int
+
+func initRecurseContract(t *testing.T) (contract sdk.AccAddress, creator sdk.AccAddress, ctx sdk.Context, keeper Keeper, cleanup func()) {
+	// we do one basic setup before all test cases (which are read-only and don't change state)
+	tempDir, err := ioutil.TempDir("", "wasm")
+	require.NoError(t, err)
+	cleanup = func() { os.RemoveAll(tempDir) }
+
+	var realWasmQuerier func(ctx sdk.Context, request *wasmTypes.WasmQuery) ([]byte, error)
+	countingQuerier := &QueryPlugins{
+		Wasm: func(ctx sdk.Context, request *wasmTypes.WasmQuery) ([]byte, error) {
+			totalWasmQueryCounter++
+			return realWasmQuerier(ctx, request)
+		},
+	}
+	ctx, keepers := CreateTestInput(t, false, tempDir, SupportedFeatures, nil, countingQuerier)
+	accKeeper, keeper := keepers.AccountKeeper, keepers.WasmKeeper
+	realWasmQuerier = WasmQuerier(&keeper)
+
+	deposit := sdk.NewCoins(sdk.NewInt64Coin("denom", 100000))
+	creator = createFakeFundedAccount(ctx, accKeeper, deposit.Add(deposit...))
+
+	// store the code
+	wasmCode, err := ioutil.ReadFile("./testdata/contract.wasm")
+	require.NoError(t, err)
+	codeID, err := keeper.Create(ctx, creator, wasmCode, "", "", nil)
+	require.NoError(t, err)
+
+	// instantiate the contract
+	_, _, bob := keyPubAddr()
+	_, _, fred := keyPubAddr()
+	initMsg := InitMsg{
+		Verifier:    fred,
+		Beneficiary: bob,
+	}
+	initMsgBz, err := json.Marshal(initMsg)
+	require.NoError(t, err)
+	contractAddr, err := keeper.Instantiate(ctx, codeID, creator, nil, initMsgBz, "recursive contract", deposit)
+	require.NoError(t, err)
+
+	return contractAddr, creator, ctx, keeper, cleanup
+}
+
 func TestGasCostOnQuery(t *testing.T) {
 	const (
 		GasNoWork uint64 = InstanceCost + 2_756
@@ -87,33 +132,8 @@ func TestGasCostOnQuery(t *testing.T) {
 		},
 	}
 
-	// we do one basic setup before all test cases (which are read-only and don't change state)
-	tempDir, err := ioutil.TempDir("", "wasm")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempDir)
-
-	ctx, keepers := CreateTestInput(t, false, tempDir, SupportedFeatures, nil, nil)
-	accKeeper, keeper := keepers.AccountKeeper, keepers.WasmKeeper
-	deposit := sdk.NewCoins(sdk.NewInt64Coin("denom", 100000))
-	creator := createFakeFundedAccount(ctx, accKeeper, deposit.Add(deposit...))
-
-	// store the code
-	wasmCode, err := ioutil.ReadFile("./testdata/contract.wasm")
-	require.NoError(t, err)
-	codeID, err := keeper.Create(ctx, creator, wasmCode, "", "", nil)
-	require.NoError(t, err)
-
-	// instantiate the contract
-	_, _, bob := keyPubAddr()
-	_, _, fred := keyPubAddr()
-	initMsg := InitMsg{
-		Verifier:    fred,
-		Beneficiary: bob,
-	}
-	initMsgBz, err := json.Marshal(initMsg)
-	require.NoError(t, err)
-	contractAddr, err := keeper.Instantiate(ctx, codeID, creator, nil, initMsgBz, "recursive contract", deposit)
-	require.NoError(t, err)
+	contractAddr, creator, ctx, keeper, cleanup := initRecurseContract(t)
+	defer cleanup()
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -149,8 +169,7 @@ func TestGasCostOnQuery(t *testing.T) {
 
 func TestGasOnExternalQuery(t *testing.T) {
 	const (
-		GasWork50       uint64 = InstanceCost + 8_464
-		GasReturnHashed uint64 = 597
+		GasWork50 uint64 = InstanceCost + 8_464
 	)
 
 	cases := map[string]struct {
@@ -190,33 +209,8 @@ func TestGasOnExternalQuery(t *testing.T) {
 		},
 	}
 
-	// we do one basic setup before all test cases (which are read-only and don't change state)
-	tempDir, err := ioutil.TempDir("", "wasm")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempDir)
-
-	ctx, keepers := CreateTestInput(t, false, tempDir, SupportedFeatures, nil, nil)
-	accKeeper, keeper := keepers.AccountKeeper, keepers.WasmKeeper
-	deposit := sdk.NewCoins(sdk.NewInt64Coin("denom", 100000))
-	creator := createFakeFundedAccount(ctx, accKeeper, deposit.Add(deposit...))
-
-	// store the code
-	wasmCode, err := ioutil.ReadFile("./testdata/contract.wasm")
-	require.NoError(t, err)
-	codeID, err := keeper.Create(ctx, creator, wasmCode, "", "", nil)
-	require.NoError(t, err)
-
-	// instantiate the contract
-	_, _, bob := keyPubAddr()
-	_, _, fred := keyPubAddr()
-	initMsg := InitMsg{
-		Verifier:    fred,
-		Beneficiary: bob,
-	}
-	initMsgBz, err := json.Marshal(initMsg)
-	require.NoError(t, err)
-	contractAddr, err := keeper.Instantiate(ctx, codeID, creator, nil, initMsgBz, "recursive contract", deposit)
-	require.NoError(t, err)
+	contractAddr, _, ctx, keeper, cleanup := initRecurseContract(t)
+	defer cleanup()
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -233,7 +227,8 @@ func TestGasOnExternalQuery(t *testing.T) {
 			if tc.expectPanic {
 				require.Panics(t, func() {
 					// this should run out of gas
-					_, _ = NewQuerier(keeper)(ctx, path, req)
+					_, err := NewQuerier(keeper)(ctx, path, req)
+					t.Logf("%v", err)
 				})
 			} else {
 				// otherwise, make sure we get a good success
@@ -243,3 +238,95 @@ func TestGasOnExternalQuery(t *testing.T) {
 		})
 	}
 }
+
+func TestLimitRecursiveQueryGas(t *testing.T) {
+	// The point of this test from https://github.com/CosmWasm/cosmwasm/issues/456
+	// Basically, if I burn 90% of gas in CPU loop, then query out (to my self)
+	// the sub-query will have all the original gas (minus the 40k instance charge)
+	// and can burn 90% and call a sub-contract again...
+	// This attack would allow us to use far more than the provided gas before
+	// eventually hitting an OutOfGas panic.
+
+	const (
+		// Note: about 100 SDK gas (10k wasmer gas) for each round of sha256
+		GasWork2k uint64 = InstanceCost + 233_379 // we have 6x gas used in cpu than in the instance
+		// This is overhead for calling into a sub-contract
+		GasReturnHashed uint64 = 603
+	)
+
+	cases := map[string]struct {
+		gasLimit                  uint64
+		msg                       Recurse
+		expectQueriesFromContract int
+		expectedGas               uint64
+		expectOutOfGas            bool
+	}{
+		"no recursion, lots of work": {
+			gasLimit: 4_000_000,
+			msg: Recurse{
+				Depth: 0,
+				Work:  2000,
+			},
+			expectQueriesFromContract: 0,
+			expectedGas:               GasWork2k,
+		},
+		"recursion 5, lots of work": {
+			gasLimit: 4_000_000,
+			msg: Recurse{
+				Depth: 5,
+				Work:  2000,
+			},
+			expectQueriesFromContract: 5,
+			expectedGas:               GasWork2k + 5*(GasWork2k+GasReturnHashed),
+		},
+		// this is where we expect an error...
+		// it has enough gas to run 4 times and die on the 5th (4th time dispatching to sub-contract)
+		// however, if we don't charge the cpu gas before sub-dispatching, we can recurse over 20 times
+		// TODO: figure out how to asset how deep it went
+		"deep recursion, should die on 5th level": {
+			gasLimit: 1_200_000,
+			msg: Recurse{
+				Depth: 50,
+				Work:  2000,
+			},
+			expectQueriesFromContract: 4,
+			expectOutOfGas:            true,
+		},
+	}
+
+	contractAddr, _, ctx, keeper, cleanup := initRecurseContract(t)
+	defer cleanup()
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// reset the counter before test
+			totalWasmQueryCounter = 0
+
+			// make sure we set a limit before calling
+			ctx = ctx.WithGasMeter(sdk.NewGasMeter(tc.gasLimit))
+			require.Equal(t, uint64(0), ctx.GasMeter().GasConsumed())
+
+			// prepare the query
+			recurse := tc.msg
+			recurse.Contract = contractAddr
+			msg := buildQuery(t, recurse)
+
+			// if we expect out of gas, make sure this panics
+			if tc.expectOutOfGas {
+				require.Panics(t, func() {
+					_, err := keeper.QuerySmart(ctx, contractAddr, msg)
+					t.Logf("Got error not panic: %#v", err)
+				})
+				assert.Equal(t, tc.expectQueriesFromContract, totalWasmQueryCounter)
+				return
+			}
+
+			// otherwise, we expect a successful call
+			_, err := keeper.QuerySmart(ctx, contractAddr, msg)
+			require.NoError(t, err)
+			assert.Equal(t, tc.expectedGas, ctx.GasMeter().GasConsumed())
+
+			assert.Equal(t, tc.expectQueriesFromContract, totalWasmQueryCounter)
+		})
+	}
+}