Fix gas cost overflow for opCall opcode (#2026)

* Fix gas cost overflow for opCall opcode * Remove variable * Minor update * Minor fix * Another test case
0xPolygon · Nov 3, 2023 · 47f2d9b · 47f2d9b
1 parent 42ee55f
commit 47f2d9b
Show file tree

Hide file tree

Showing 6 changed files with 146 additions and 13 deletions.
diff --git a/chain/params.go b/chain/params.go
@@ -107,8 +107,10 @@ func (f *Forks) SetFork(name string, value Fork) {
 	(*f)[name] = value
 }
 
-func (f *Forks) RemoveFork(name string) {
+func (f *Forks) RemoveFork(name string) *Forks {
 	delete(*f, name)
+
+	return f
 }
 
 // At returns ForksInTime instance that shows which supported forks are enabled for the block

diff --git a/helper/common/common.go b/helper/common/common.go
@@ -358,6 +358,17 @@ func BigIntDivCeil(a, b *big.Int) *big.Int {
 		Div(result, b)
 }
 
+// SafeAddUint64 sums two unsigned int64 numbers if there is no overflow.
+// In case there is an overflow, it would return 0 and true, otherwise sum and false.
+func SafeAddUint64(a, b uint64) (uint64, bool) {
+	sum := a + b
+	if sum < a || sum < b {
+		return 0, true
+	}
+
+	return sum, false
+}
+
 // EncodeUint64ToBytes encodes provided uint64 to big endian byte slice
 func EncodeUint64ToBytes(value uint64) []byte {
 	result := make([]byte, 8)

diff --git a/helper/common/common_test.go b/helper/common/common_test.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"math"
 	"math/big"
 	"testing"
 	"time"
@@ -124,3 +126,43 @@ func TestRetryForever_CancelContext_ShouldEnd(t *testing.T) {
 	})
 	require.True(t, errors.Is(ctx.Err(), context.Canceled))
 }
+
+func Test_SafeAddUint64(t *testing.T) {
+	cases := []struct {
+		a        uint64
+		b        uint64
+		result   uint64
+		overflow bool
+	}{
+		{
+			a:      10,
+			b:      4,
+			result: 14,
+		},
+		{
+			a:      0,
+			b:      5,
+			result: 5,
+		},
+		{
+			a:        math.MaxUint64,
+			b:        3,
+			result:   0,
+			overflow: true,
+		},
+		{
+			a:        math.MaxUint64,
+			b:        math.MaxUint64,
+			result:   0,
+			overflow: true,
+		},
+	}
+
+	for i, c := range cases {
+		t.Run(fmt.Sprintf("%d case", i+1), func(t *testing.T) {
+			actualResult, actualOverflow := SafeAddUint64(c.a, c.b)
+			require.Equal(t, c.result, actualResult)
+			require.Equal(t, c.overflow, actualOverflow)
+		})
+	}
+}
diff --git a/state/runtime/evm/instructions.go b/state/runtime/evm/instructions.go
@@ -3,12 +3,12 @@ package evm
 
 import (
 	"errors"
-	"fmt"
 	"math/big"
 	"math/bits"
 	"sync"
 
 	"github.com/0xPolygon/polygon-edge/crypto"
+	"github.com/0xPolygon/polygon-edge/helper/common"
 	"github.com/0xPolygon/polygon-edge/helper/keccak"
 	"github.com/0xPolygon/polygon-edge/state/runtime"
 	"github.com/0xPolygon/polygon-edge/types"
@@ -1232,11 +1232,10 @@ func (c *state) buildCallContract(op OpCode) (*runtime.Contract, uint64, uint64,
 		gasCost = 40
 	}
 
-	eip158 := c.config.EIP158
 	transfersValue := (op == CALL || op == CALLCODE) && value != nil && value.Sign() != 0
 
 	if op == CALL {
-		if eip158 {
+		if c.config.EIP158 {
 			if transfersValue && c.host.Empty(addr) {
 				gasCost += 25000
 			}
@@ -1271,7 +1270,14 @@ func (c *state) buildCallContract(op OpCode) (*runtime.Contract, uint64, uint64,
 		gas = initialGas.Uint64()
 	}
 
-	gasCost = gasCost + gas
+	gasCostTmp, isOverflow := common.SafeAddUint64(gasCost, gas)
+	if isOverflow {
+		c.exit(errGasUintOverflow)
+
+		return nil, 0, 0, nil
+	}
+
+	gasCost = gasCostTmp
 
 	// Consume gas cost
 	if !c.consumeGas(gasCost) {
@@ -1309,7 +1315,7 @@ func (c *state) buildCallContract(op OpCode) (*runtime.Contract, uint64, uint64,
 
 	if transfersValue {
 		if c.host.GetBalance(c.msg.Address).Cmp(value) < 0 {
-			return contract, 0, 0, fmt.Errorf("bad")
+			return contract, 0, 0, types.ErrInsufficientFunds
 		}
 	}
 
@@ -1352,7 +1358,7 @@ func (c *state) buildCreateContract(op OpCode) (*runtime.Contract, error) {
 
 	if hasTransfer {
 		if c.host.GetBalance(c.msg.Address).Cmp(value) < 0 {
-			return nil, fmt.Errorf("bad")
+			return nil, types.ErrInsufficientFunds
 		}
 	}
 

diff --git a/state/runtime/evm/instructions_test.go b/state/runtime/evm/instructions_test.go
@@ -1,6 +1,7 @@
 package evm
 
 import (
+	"math"
 	"math/big"
 	"testing"
 
@@ -666,7 +667,7 @@ func Test_opCall(t *testing.T) {
 		name        string
 		op          OpCode
 		contract    *runtime.Contract
-		config      *chain.ForksInTime
+		config      chain.ForksInTime
 		initState   *state
 		resultState *state
 		mockHost    *mockHostForInstructions
@@ -678,7 +679,7 @@ func Test_opCall(t *testing.T) {
 			contract: &runtime.Contract{
 				Static: true,
 			},
-			config: &allEnabledForks,
+			config: allEnabledForks,
 			initState: &state{
 				gas: 1000,
 				sp:  6,
@@ -696,6 +697,73 @@ func Test_opCall(t *testing.T) {
 				memory: []byte{0x01},
 				stop:   false,
 				err:    nil,
+				gas:    300,
+			},
+			mockHost: &mockHostForInstructions{
+				callxResult: &runtime.ExecutionResult{
+					ReturnValue: []byte{0x03},
+				},
+			},
+		},
+		{
+			name: "call cost overflow (EIP150 fork disabled)",
+			op:   CALLCODE,
+			contract: &runtime.Contract{
+				Static: false,
+			},
+			config: chain.AllForksEnabled.RemoveFork(chain.EIP150).At(0),
+			initState: &state{
+				gas: 6640,
+				sp:  7,
+				stack: []*big.Int{
+					big.NewInt(0x00),                        // outSize
+					big.NewInt(0x00),                        // outOffset
+					big.NewInt(0x00),                        // inSize
+					big.NewInt(0x00),                        // inOffset
+					big.NewInt(0x01),                        // value
+					big.NewInt(0x03),                        // address
+					big.NewInt(0).SetUint64(math.MaxUint64), // initialGas
+				},
+				memory: []byte{0x01},
+			},
+			resultState: &state{
+				memory: []byte{0x01},
+				stop:   true,
+				err:    errGasUintOverflow,
+				gas:    6640,
+			},
+			mockHost: &mockHostForInstructions{
+				callxResult: &runtime.ExecutionResult{
+					ReturnValue: []byte{0x03},
+				},
+			},
+		},
+		{
+			name: "available gas underflow",
+			op:   CALLCODE,
+			contract: &runtime.Contract{
+				Static: false,
+			},
+			config: allEnabledForks,
+			initState: &state{
+				gas: 6640,
+				sp:  7,
+				stack: []*big.Int{
+					big.NewInt(0x00),                        // outSize
+					big.NewInt(0x00),                        // outOffset
+					big.NewInt(0x00),                        // inSize
+					big.NewInt(0x00),                        // inOffset
+					big.NewInt(0x01),                        // value
+					big.NewInt(0x03),                        // address
+					big.NewInt(0).SetUint64(math.MaxUint64), // initialGas
+				},
+				memory: []byte{0x01},
+			},
+			resultState: &state{
+				memory: []byte{0x01},
+				stop:   true,
+				err:    errOutOfGas,
+				gas:    6640,
 			},
 			mockHost: &mockHostForInstructions{
 				callxResult: &runtime.ExecutionResult{
@@ -717,14 +785,15 @@ func Test_opCall(t *testing.T) {
 			state.sp = test.initState.sp
 			state.stack = test.initState.stack
 			state.memory = test.initState.memory
-			state.config = test.config
+			state.config = &test.config
 			state.host = test.mockHost
 
 			opCall(test.op)(state)
 
-			assert.Equal(t, test.resultState.memory, state.memory, "memory in state after execution is not correct")
-			assert.Equal(t, test.resultState.stop, state.stop, "stop in state after execution is not correct")
-			assert.Equal(t, test.resultState.err, state.err, "err in state after execution is not correct")
+			assert.Equal(t, test.resultState.memory, state.memory, "memory in state after execution is incorrect")
+			assert.Equal(t, test.resultState.stop, state.stop, "stop in state after execution is incorrect")
+			assert.Equal(t, test.resultState.err, state.err, "err in state after execution is incorrect")
+			assert.Equal(t, test.resultState.gas, state.gas, "gas in state after execution is incorrect")
 		})
 	}
 }
diff --git a/types/types.go b/types/types.go
@@ -40,6 +40,9 @@ var (
 
 	// ErrTxTypeNotSupported denotes that transaction is not supported
 	ErrTxTypeNotSupported = errors.New("transaction type not supported")
+
+	// ErrInsufficientFunds denotes that account has insufficient funds for transaction execution
+	ErrInsufficientFunds = errors.New("insufficient funds for execution")
 )
 
 type Hash [HashLength]byte