From a23cf1fbb1a76a4a43b9ce7019a8efbe0d2739ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= Date: Thu, 2 Nov 2023 11:58:20 +0100 Subject: [PATCH 1/5] Fix gas cost overflow for opCall opcode --- chain/params.go | 8 ++- helper/common/common.go | 10 ++++ helper/common/common_test.go | 31 ++++++++++ state/runtime/evm/instructions.go | 18 ++++-- state/runtime/evm/instructions_test.go | 83 ++++++++++++++++++++++++-- types/types.go | 3 + 6 files changed, 139 insertions(+), 14 deletions(-) diff --git a/chain/params.go b/chain/params.go index 1630362aa0..eed4fce8a6 100644 --- a/chain/params.go +++ b/chain/params.go @@ -103,12 +103,16 @@ func (f *Forks) IsActive(name string, block uint64) bool { } // SetFork adds/updates fork defined by name -func (f *Forks) SetFork(name string, value Fork) { +func (f *Forks) SetFork(name string, value Fork) *Forks { (*f)[name] = value + + return f } -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 index 237d5c14d8..124260a018 100644 --- a/helper/common/common.go +++ b/helper/common/common.go @@ -358,6 +358,16 @@ func BigIntDivCeil(a, b *big.Int) *big.Int { Div(result, b) } +// SafeAddUint64 sums two unsigned int64 numbers if there are 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) { + if a > math.MaxUint64-b { + return 0, true + } + + return a + b, 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 index cca507c590..85766837cb 100644 --- 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,32 @@ 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: math.MaxUint64, + b: 3, + 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 index 4f9c798e11..d2a38374a1 100644 --- 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 index e17f79fdcc..fd77554c49 100644 --- 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" @@ -15,6 +16,8 @@ var ( two = big.NewInt(2) allEnabledForks = chain.AllForksEnabled.At(0) + + eip150DisabledForks = chain.AllForksEnabled.RemoveFork(chain.EIP150).At(0) ) type cases2To1 []struct { @@ -666,7 +669,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 +681,7 @@ func Test_opCall(t *testing.T) { contract: &runtime.Contract{ Static: true, }, - config: &allEnabledForks, + config: allEnabledForks, initState: &state{ gas: 1000, sp: 6, @@ -696,6 +699,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: eip150DisabledForks, + 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 +787,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 index 3ca3cc35b5..d3e53a1d36 100644 --- 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 From 112b4d673d9778ff83b7046324b994ee71e32d02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= Date: Thu, 2 Nov 2023 12:10:20 +0100 Subject: [PATCH 2/5] Remove variable --- state/runtime/evm/instructions_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/state/runtime/evm/instructions_test.go b/state/runtime/evm/instructions_test.go index fd77554c49..b0d303ad51 100644 --- a/state/runtime/evm/instructions_test.go +++ b/state/runtime/evm/instructions_test.go @@ -16,8 +16,6 @@ var ( two = big.NewInt(2) allEnabledForks = chain.AllForksEnabled.At(0) - - eip150DisabledForks = chain.AllForksEnabled.RemoveFork(chain.EIP150).At(0) ) type cases2To1 []struct { @@ -713,7 +711,7 @@ func Test_opCall(t *testing.T) { contract: &runtime.Contract{ Static: false, }, - config: eip150DisabledForks, + config: chain.AllForksEnabled.RemoveFork(chain.EIP150).At(0), initState: &state{ gas: 6640, sp: 7, From d373a4c255037fc4f888153076a3bf572639dbe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= Date: Thu, 2 Nov 2023 13:20:39 +0100 Subject: [PATCH 3/5] Minor update --- chain/params.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/chain/params.go b/chain/params.go index eed4fce8a6..8969ea4c2b 100644 --- a/chain/params.go +++ b/chain/params.go @@ -103,10 +103,8 @@ func (f *Forks) IsActive(name string, block uint64) bool { } // SetFork adds/updates fork defined by name -func (f *Forks) SetFork(name string, value Fork) *Forks { +func (f *Forks) SetFork(name string, value Fork) { (*f)[name] = value - - return f } func (f *Forks) RemoveFork(name string) *Forks { From 76c28464ccc8971a6a08240d2c4fa3b78686d750 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= Date: Fri, 3 Nov 2023 07:32:53 +0100 Subject: [PATCH 4/5] Minor fix --- helper/common/common.go | 7 ++++--- helper/common/common_test.go | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/helper/common/common.go b/helper/common/common.go index 124260a018..b1db542c6c 100644 --- a/helper/common/common.go +++ b/helper/common/common.go @@ -358,14 +358,15 @@ func BigIntDivCeil(a, b *big.Int) *big.Int { Div(result, b) } -// SafeAddUint64 sums two unsigned int64 numbers if there are no overflow. +// 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) { - if a > math.MaxUint64-b { + sum := a + b + if sum < a || sum < b { return 0, true } - return a + b, false + return sum, false } // EncodeUint64ToBytes encodes provided uint64 to big endian byte slice diff --git a/helper/common/common_test.go b/helper/common/common_test.go index 85766837cb..494e731637 100644 --- a/helper/common/common_test.go +++ b/helper/common/common_test.go @@ -139,6 +139,11 @@ func Test_SafeAddUint64(t *testing.T) { b: 4, result: 14, }, + { + a: 0, + b: 5, + result: 5, + }, { a: math.MaxUint64, b: 3, From b0870e5dbd2fd61c541d5c6fce3491f1ac69deed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= Date: Fri, 3 Nov 2023 09:09:45 +0100 Subject: [PATCH 5/5] Another test case --- helper/common/common_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/helper/common/common_test.go b/helper/common/common_test.go index 494e731637..91a654a424 100644 --- a/helper/common/common_test.go +++ b/helper/common/common_test.go @@ -150,6 +150,12 @@ func Test_SafeAddUint64(t *testing.T) { result: 0, overflow: true, }, + { + a: math.MaxUint64, + b: math.MaxUint64, + result: 0, + overflow: true, + }, } for i, c := range cases {