Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

accounts/abi: fix ReadInteger #26568

Merged
merged 5 commits into from
Feb 7, 2023
Merged

accounts/abi: fix ReadInteger #26568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d98b3ae
fix ReadInteger
zhiqiangxu Jan 28, 2023
ec3fc81
fix
zhiqiangxu Jan 28, 2023
17e9024
fix for ci
zhiqiangxu Jan 29, 2023
f3a3c89
fix for review
zhiqiangxu Jan 30, 2023
706e240
make ci happy
zhiqiangxu Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion accounts/abi/error_handling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,15 @@ import (
)

var (
errBadBool = errors.New("abi: improperly encoded boolean value")
errBadBool = errors.New("abi: improperly encoded boolean value")
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
errBadUint8 = errors.New("abi: improperly encoded uint8 value")
errBadUint16 = errors.New("abi: improperly encoded uint16 value")
errBadUint32 = errors.New("abi: improperly encoded uint32 value")
errBadUint64 = errors.New("abi: improperly encoded uint64 value")
errBadInt8 = errors.New("abi: improperly encoded int8 value")
errBadInt16 = errors.New("abi: improperly encoded int16 value")
errBadInt32 = errors.New("abi: improperly encoded int32 value")
errBadInt64 = errors.New("abi: improperly encoded int64 value")
)

// formatSliceString formats the reflection kind with the given slice size
Expand Down
72 changes: 51 additions & 21 deletions accounts/abi/unpack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package abi
import (
"encoding/binary"
"fmt"
"math"
"math/big"
"reflect"

Expand All @@ -33,43 +34,72 @@ var (
)

// ReadInteger reads the integer based on its kind and returns the appropriate value.
func ReadInteger(typ Type, b []byte) interface{} {
func ReadInteger(typ Type, b []byte) (interface{}, error) {
ret := new(big.Int).SetBytes(b)

if typ.T == UintTy {
u64, isu64 := ret.Uint64(), ret.IsUint64()
switch typ.Size {
case 8:
return b[len(b)-1]
if !isu64 || u64 > math.MaxUint8 {
return nil, errBadUint8
}
return byte(u64), nil
case 16:
return binary.BigEndian.Uint16(b[len(b)-2:])
if !isu64 || u64 > math.MaxUint16 {
return nil, errBadUint16
}
return uint16(u64), nil
case 32:
return binary.BigEndian.Uint32(b[len(b)-4:])
if !isu64 || u64 > math.MaxUint32 {
return nil, errBadUint32
}
return uint32(u64), nil
case 64:
return binary.BigEndian.Uint64(b[len(b)-8:])
if !isu64 {
return nil, errBadUint64
}
return u64, nil
default:
// the only case left for unsigned integer is uint256.
return new(big.Int).SetBytes(b)
return ret, nil
}
}

// big.SetBytes can't tell if a number is negative or positive in itself.
// On EVM, if the returned number > max int256, it is negative.
// A number is > max int256 if the bit at position 255 is set.
if ret.Bit(255) == 1 {
ret.Add(MaxUint256, new(big.Int).Neg(ret))
ret.Add(ret, common.Big1)
ret.Neg(ret)
}
i64, isi64 := ret.Int64(), ret.IsInt64()
switch typ.Size {
case 8:
return int8(b[len(b)-1])
if !isi64 || i64 < math.MinInt8 || i64 > math.MaxInt8 {
return nil, errBadInt8
}
return int8(i64), nil
Comment on lines +72 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be covered by the testcases in packing_test.go -- since nothing breaks. BUT: It looks to me like you are changing the behaviour for negative numbers here?
I'll look at bit closer at this later on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without your PR, these cases work:

	{
		def:      `[{"type": "int8"}]`,
		unpacked: int8(-1),
		packed:   "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
	},
	{
		def:      `[{"type": "int8"}]`,
		unpacked: int8(-1),
		packed:   "00000000000000000000000000000000000000000000000000000000000000ff",
	},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIth your changes, this is a case which fails, and which should not fail (?)

    unpack_test.go:50: test 15 ([{"type": "int8"}]: 00000000000000000000000000000000000000000000000000000000000000ff) failed: abi: improperly encoded int8 value

more precisely:

	{
		def:      `[{"type": "int8"}]`,
		unpacked: int8(-1),
		packed:   "00000000000000000000000000000000000000000000000000000000000000ff",
	},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this seems to be valid way to encode int8 even after the changes in this PR:

	{
		def:      `[{"type": "int8"}]`,
		unpacked: int8(-1),
		packed:   "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
	},

Copy link
Member

@rjl493456442 rjl493456442 Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is: will abi encoder represents a int8(-1) as 00000000000000000000000000000000000000000000000000000000000000ff in the first place? It feels like an invalid case.

From ABI specification https://docs.soliditylang.org/en/v0.8.17/abi-spec.html#formal-specification-of-the-encoding:

int<M>: enc(X) is the big-endian two’s complement encoding of X, padded on the higher-order (left) side with 0xff bytes for negative X and with zero-bytes for non-negative X such that the length is 32 bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, int8(-1) is represented as all f. In EVM, a signed number is negative when the most significant bit is 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I seem to have been wrong

case 16:
return int16(binary.BigEndian.Uint16(b[len(b)-2:]))
if !isi64 || i64 < math.MinInt16 || i64 > math.MaxInt16 {
return nil, errBadInt16
}
return int16(i64), nil
case 32:
return int32(binary.BigEndian.Uint32(b[len(b)-4:]))
if !isi64 || i64 < math.MinInt32 || i64 > math.MaxInt32 {
return nil, errBadInt32
}
return int32(i64), nil
case 64:
return int64(binary.BigEndian.Uint64(b[len(b)-8:]))
if !isi64 {
return nil, errBadInt64
}
return i64, nil
default:
// the only case left for integer is int256
// big.SetBytes can't tell if a number is negative or positive in itself.
// On EVM, if the returned number > max int256, it is negative.
// A number is > max int256 if the bit at position 255 is set.
ret := new(big.Int).SetBytes(b)
if ret.Bit(255) == 1 {
ret.Add(MaxUint256, new(big.Int).Neg(ret))
ret.Add(ret, common.Big1)
ret.Neg(ret)
}
return ret

return ret, nil
}
}

Expand Down Expand Up @@ -234,7 +264,7 @@ func toGoType(index int, t Type, output []byte) (interface{}, error) {
case StringTy: // variable arrays are written at the end of the return bytes
return string(output[begin : begin+length]), nil
case IntTy, UintTy:
return ReadInteger(t, returnOutput), nil
return ReadInteger(t, returnOutput)
case BoolTy:
return readBool(returnOutput)
case AddressTy:
Expand Down
162 changes: 162 additions & 0 deletions accounts/abi/unpack_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"encoding/hex"
"fmt"
"math"
"math/big"
"reflect"
"strconv"
Expand Down Expand Up @@ -943,3 +944,164 @@ func TestOOMMaliciousInput(t *testing.T) {
}
}
}

func TestPackAndUnpackIncompatibleNumber(t *testing.T) {
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
var encodeABI Arguments
uint256Ty, err := NewType("uint256", "", nil)
if err != nil {
panic(err)
}
encodeABI = Arguments{
{Type: uint256Ty},
}

maxU64, ok := new(big.Int).SetString(strconv.FormatUint(math.MaxUint64, 10), 10)
if !ok {
panic("bug")
}
maxU64Plus1 := new(big.Int).Add(maxU64, big.NewInt(1))
cases := []struct {
decodeType string
inputValue *big.Int
err error
expectValue interface{}
}{
{
decodeType: "uint8",
inputValue: big.NewInt(math.MaxUint8 + 1),
err: errBadUint8,
},
{
decodeType: "uint8",
inputValue: big.NewInt(math.MaxUint8),
err: nil,
expectValue: uint8(math.MaxUint8),
},
{
decodeType: "uint16",
inputValue: big.NewInt(math.MaxUint16 + 1),
err: errBadUint16,
},
{
decodeType: "uint16",
inputValue: big.NewInt(math.MaxUint16),
err: nil,
expectValue: uint16(math.MaxUint16),
},
{
decodeType: "uint32",
inputValue: big.NewInt(math.MaxUint32 + 1),
err: errBadUint32,
},
{
decodeType: "uint32",
inputValue: big.NewInt(math.MaxUint32),
err: nil,
expectValue: uint32(math.MaxUint32),
},
{
decodeType: "uint64",
inputValue: maxU64Plus1,
err: errBadUint64,
},
{
decodeType: "uint64",
inputValue: maxU64,
err: nil,
expectValue: uint64(math.MaxUint64),
},
{
decodeType: "uint256",
inputValue: maxU64Plus1,
err: nil,
expectValue: maxU64Plus1,
},
{
decodeType: "int8",
inputValue: big.NewInt(math.MaxInt8 + 1),
err: errBadInt8,
},
{
decodeType: "int8",
inputValue: big.NewInt(math.MinInt8 - 1),
err: errBadInt8,
},
{
decodeType: "int8",
inputValue: big.NewInt(math.MaxInt8),
err: nil,
expectValue: int8(math.MaxInt8),
},
{
decodeType: "int16",
inputValue: big.NewInt(math.MaxInt16 + 1),
err: errBadInt16,
},
{
decodeType: "int16",
inputValue: big.NewInt(math.MinInt16 - 1),
err: errBadInt16,
},
{
decodeType: "int16",
inputValue: big.NewInt(math.MaxInt16),
err: nil,
expectValue: int16(math.MaxInt16),
},
{
decodeType: "int32",
inputValue: big.NewInt(math.MaxInt32 + 1),
err: errBadInt32,
},
{
decodeType: "int32",
inputValue: big.NewInt(math.MinInt32 - 1),
err: errBadInt32,
},
{
decodeType: "int32",
inputValue: big.NewInt(math.MaxInt32),
err: nil,
expectValue: int32(math.MaxInt32),
},
{
decodeType: "int64",
inputValue: new(big.Int).Add(big.NewInt(math.MaxInt64), big.NewInt(1)),
err: errBadInt64,
},
{
decodeType: "int64",
inputValue: new(big.Int).Sub(big.NewInt(math.MinInt64), big.NewInt(1)),
err: errBadInt64,
},
{
decodeType: "int64",
inputValue: big.NewInt(math.MaxInt64),
err: nil,
expectValue: int64(math.MaxInt64),
},
}
for i, testCase := range cases {
packed, err := encodeABI.Pack(testCase.inputValue)
if err != nil {
panic(err)
}
ty, err := NewType(testCase.decodeType, "", nil)
if err != nil {
panic(err)
}
decodeABI := Arguments{
{Type: ty},
}
decoded, err := decodeABI.Unpack(packed)
if err != testCase.err {
t.Fatalf("Expected error %v, actual error %v. case %d", testCase.err, err, i)
}
if err != nil {
continue
}
if !reflect.DeepEqual(decoded[0], testCase.expectValue) {
t.Fatalf("Expected value %v, actual value %v", testCase.expectValue, decoded[0])
}
}
}