diff --git a/state/runtime/evm/instructions.go b/state/runtime/evm/instructions.go index ecaf91602e..7cd86a58e6 100644 --- a/state/runtime/evm/instructions.go +++ b/state/runtime/evm/instructions.go @@ -809,41 +809,44 @@ func opReturnDataCopy(c *state) { return } - memOffset := c.pop() - dataOffset := c.pop() - length := c.pop() + var ( + memOffset = c.pop() + dataOffset = c.pop() + length = c.pop() + ) - if !dataOffset.IsUint64() { + // Check if: + // 1. the dataOffset is uint64 (overflow check) + // 2. the sum of dataOffset and length overflows uint64 + // 3. the length of return data has enough space to receive offset + length bytes + end := new(big.Int).Add(dataOffset, length) + endAddress := end.Uint64() + + if !dataOffset.IsUint64() || + !end.IsUint64() || + uint64(len(c.returnData)) < endAddress { c.exit(errReturnDataOutOfBounds) return } // if length is 0, return immediately since no need for the data copying nor memory allocation - if length.Sign() == 0 || !c.allocateMemory(memOffset, length) { - return - } - - ulength := length.Uint64() - if !c.consumeGas(((ulength + 31) / 32) * copyGas) { + if length.Sign() == 0 { return } - dataEnd := length.Add(dataOffset, length) - if !dataEnd.IsUint64() { - c.exit(errReturnDataOutOfBounds) - + if !c.allocateMemory(memOffset, length) { + // Error code is set inside the allocateMemory call return } - dataEndIndex := dataEnd.Uint64() - if uint64(len(c.returnData)) < dataEndIndex { - c.exit(errReturnDataOutOfBounds) - + ulength := length.Uint64() + if !c.consumeGas(((ulength + 31) / 32) * copyGas) { + // Error code is set inside the consumeGas return } - data := c.returnData[dataOffset.Uint64():dataEndIndex] + data := c.returnData[dataOffset.Uint64():endAddress] copy(c.memory[memOffset.Uint64():memOffset.Uint64()+ulength], data) } diff --git a/state/runtime/evm/instructions_test.go b/state/runtime/evm/instructions_test.go index d3f7448b96..04053e2510 100644 --- a/state/runtime/evm/instructions_test.go +++ b/state/runtime/evm/instructions_test.go @@ -1726,6 +1726,16 @@ func TestCreate(t *testing.T) { func Test_opReturnDataCopy(t *testing.T) { t.Parallel() + // Positive number that does not fit in uint64 (math.MaxUint64 + 1) + largeNumber := "18446744073709551616" + bigIntValue := new(big.Int) + bigIntValue.SetString(largeNumber, 10) + + // Positive number that does fit in uint64 but multiplied by two does not + largeNumber2 := "18446744073709551615" + bigIntValue2 := new(big.Int) + bigIntValue2.SetString(largeNumber2, 10) + tests := []struct { name string config *chain.ForksInTime @@ -1755,7 +1765,8 @@ func Test_opReturnDataCopy(t *testing.T) { big.NewInt(0), // dataOffset big.NewInt(-1), // memOffset }, - sp: 3, + sp: 3, + returnData: []byte{0xff}, }, resultState: &state{ config: &allEnabledForks, @@ -1764,9 +1775,10 @@ func Test_opReturnDataCopy(t *testing.T) { big.NewInt(0), big.NewInt(-1), }, - sp: 0, - stop: true, - err: errReturnDataOutOfBounds, + sp: 0, + returnData: []byte{0xff}, + stop: true, + err: errReturnDataOutOfBounds, }, }, { @@ -1800,21 +1812,23 @@ func Test_opReturnDataCopy(t *testing.T) { initState: &state{ stack: []*big.Int{ big.NewInt(-1), // length - big.NewInt(0), // dataOffset + big.NewInt(2), // dataOffset big.NewInt(0), // memOffset }, - sp: 3, + sp: 3, + returnData: []byte{0xff}, }, resultState: &state{ config: &allEnabledForks, stack: []*big.Int{ big.NewInt(-1), - big.NewInt(0), + big.NewInt(2), big.NewInt(0), }, - sp: 0, - stop: true, - err: errReturnDataOutOfBounds, + sp: 0, + returnData: []byte{0xff}, + stop: true, + err: errReturnDataOutOfBounds, }, }, { @@ -1849,66 +1863,134 @@ func Test_opReturnDataCopy(t *testing.T) { }, }, { - name: "should expand memory and copy data returnData", + // this test case also verifies that code does not panic when the length is 0 and memOffset > len(memory) + name: "should not copy data if length is zero", config: &allEnabledForks, initState: &state{ stack: []*big.Int{ - big.NewInt(5), // length - big.NewInt(1), // dataOffset - big.NewInt(2), // memOffset + big.NewInt(0), // length + big.NewInt(0), // dataOffset + big.NewInt(4), // memOffset }, sp: 3, - returnData: []byte{0x01, 0x02, 0x03, 0x04, 0x05, 0x06}, - memory: []byte{0x11, 0x22}, - gas: 20, + returnData: []byte{0x01}, + memory: []byte{0x02}, + }, + resultState: &state{ + config: &allEnabledForks, + stack: []*big.Int{ + big.NewInt(0), + big.NewInt(0), + big.NewInt(4), + }, + sp: 0, + returnData: []byte{0x01}, + memory: []byte{0x02}, + stop: false, + err: nil, + }, + }, + { + name: "should return error if data offset overflows uint64", + config: &allEnabledForks, + initState: &state{ + stack: []*big.Int{ + big.NewInt(1), // length + bigIntValue, // dataOffset + big.NewInt(-1), // memOffset + }, + sp: 3, }, resultState: &state{ config: &allEnabledForks, stack: []*big.Int{ - big.NewInt(6), // updated for end index big.NewInt(1), + bigIntValue, + big.NewInt(-1), + }, + sp: 0, + stop: true, + err: errReturnDataOutOfBounds, + }, + }, + { + name: "should return error if sum of data offset and length overflows uint64", + config: &allEnabledForks, + initState: &state{ + stack: []*big.Int{ + bigIntValue2, // length + bigIntValue2, // dataOffset + big.NewInt(-1), // memOffset + }, + sp: 3, + }, + resultState: &state{ + config: &allEnabledForks, + stack: []*big.Int{ + bigIntValue2, + bigIntValue2, + big.NewInt(-1), + }, + sp: 0, + stop: true, + err: errReturnDataOutOfBounds, + }, + }, + { + name: "should return error if the length of return data does not have enough space to receive offset + length bytes", + config: &allEnabledForks, + initState: &state{ + stack: []*big.Int{ + big.NewInt(2), // length + big.NewInt(0), // dataOffset + big.NewInt(0), // memOffset + }, + sp: 3, + returnData: []byte{0xff}, + memory: []byte{0x0}, + }, + resultState: &state{ + config: &allEnabledForks, + stack: []*big.Int{ big.NewInt(2), + big.NewInt(0), + big.NewInt(0), }, sp: 0, - returnData: []byte{0x01, 0x02, 0x03, 0x04, 0x05, 0x06}, - memory: append( - // 1 word (32 bytes) - []byte{0x11, 0x22, 0x02, 0x03, 0x04, 0x05, 0x06}, - make([]byte, 25)..., - ), - gas: 14, - lastGasCost: 3, - currentConsumedGas: 6, - stop: false, - err: nil, + returnData: []byte{0xff}, + memory: []byte{0x0}, + stop: true, + err: errReturnDataOutOfBounds, }, }, { - // this test case also verifies that code does not panic when the length is 0 and memOffset > len(memory) - name: "should not copy data if length is zero", + name: "should return error if there is no gas", config: &allEnabledForks, initState: &state{ stack: []*big.Int{ - big.NewInt(0), // length + big.NewInt(1), // length big.NewInt(0), // dataOffset - big.NewInt(4), // memOffset + big.NewInt(0), // memOffset }, sp: 3, - returnData: []byte{0x01}, - memory: []byte{0x02}, + returnData: []byte{0xff}, + memory: []byte{0x0}, + gas: 0, }, resultState: &state{ config: &allEnabledForks, stack: []*big.Int{ + big.NewInt(1), big.NewInt(0), big.NewInt(0), - big.NewInt(4), }, - sp: 0, - returnData: []byte{0x01}, - memory: []byte{0x02}, - stop: false, - err: nil, + sp: 0, + returnData: []byte{0xff}, + memory: []byte{0x0}, + gas: 0, + currentConsumedGas: 3, + stop: true, + err: errOutOfGas, }, }, }