From d98b3ae718433729d51922878fc9e54977fb51b6 Mon Sep 17 00:00:00 2001 From: zhiqiangxu <652732310@qq.com> Date: Sun, 29 Jan 2023 01:46:46 +0800 Subject: [PATCH 1/5] fix ReadInteger --- accounts/abi/error_handling.go | 10 ++++- accounts/abi/unpack.go | 72 ++++++++++++++++++++++++---------- accounts/abi/unpack_test.go | 26 ++++++++++++ 3 files changed, 86 insertions(+), 22 deletions(-) diff --git a/accounts/abi/error_handling.go b/accounts/abi/error_handling.go index 7add7072925e..c106e9ac4321 100644 --- a/accounts/abi/error_handling.go +++ b/accounts/abi/error_handling.go @@ -23,7 +23,15 @@ import ( ) var ( - errBadBool = errors.New("abi: improperly encoded boolean value") + errBadBool = errors.New("abi: improperly encoded boolean value") + 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 diff --git a/accounts/abi/unpack.go b/accounts/abi/unpack.go index b6ca0a038480..58f5a47b2d1e 100644 --- a/accounts/abi/unpack.go +++ b/accounts/abi/unpack.go @@ -19,6 +19,7 @@ package abi import ( "encoding/binary" "fmt" + "math" "math/big" "reflect" @@ -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 { + v64 := ret.Uint64() switch typ.Size { case 8: - return b[len(b)-1] + if v64 >= math.MaxUint8 { + return nil, errBadUint8 + } + return byte(v64), nil case 16: - return binary.BigEndian.Uint16(b[len(b)-2:]) + if v64 >= math.MaxUint16 { + return nil, errBadUint16 + } + return uint16(v64), nil case 32: - return binary.BigEndian.Uint32(b[len(b)-4:]) + if v64 >= math.MaxUint32 { + return nil, errBadUint32 + } + return uint32(v64), nil case 64: - return binary.BigEndian.Uint64(b[len(b)-8:]) + if v64 > math.MaxUint64 { + return nil, errBadUint64 + } + return v64, 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) + } + iv64 := ret.Int64() switch typ.Size { case 8: - return int8(b[len(b)-1]) + if iv64 < math.MinInt8 || iv64 > math.MaxInt8 { + return nil, errBadInt8 + } + return int8(iv64), nil case 16: - return int16(binary.BigEndian.Uint16(b[len(b)-2:])) + if iv64 < math.MinInt16 || iv64 > math.MaxInt16 { + return nil, errBadInt16 + } + return int16(iv64), nil case 32: - return int32(binary.BigEndian.Uint32(b[len(b)-4:])) + if iv64 < math.MinInt32 || iv64 > math.MaxInt32 { + return nil, errBadInt32 + } + return int32(iv64), nil case 64: - return int64(binary.BigEndian.Uint64(b[len(b)-8:])) + if iv64 < math.MinInt64 || iv64 > math.MaxInt64 { + return nil, errBadInt64 + } + return iv64, 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 } } @@ -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: diff --git a/accounts/abi/unpack_test.go b/accounts/abi/unpack_test.go index 363e0cd5943e..0bb6df3c5b38 100644 --- a/accounts/abi/unpack_test.go +++ b/accounts/abi/unpack_test.go @@ -943,3 +943,29 @@ func TestOOMMaliciousInput(t *testing.T) { } } } + +func TestPackAndUnpackIncompatibleNumber(t *testing.T) { + var encodeABI, decodeABI Arguments + uint256Ty, err := NewType("uint256", "", nil) + if err != nil { + panic(err) + } + uint8Ty, err := NewType("uint8", "", nil) + if err != nil { + panic(err) + } + encodeABI = Arguments{ + {Type: uint256Ty}, + } + decodeABI = Arguments{ + {Type: uint8Ty}, + } + packed, err := encodeABI.Pack(big.NewInt(256)) + if err != nil { + panic(err) + } + _, err = decodeABI.Unpack(packed) + if err != errBadUint8 { + t.Fatal(err) + } +} From ec3fc81a3b82b3cf7720eb77e8bbbca02b4bb956 Mon Sep 17 00:00:00 2001 From: zhiqiangxu <652732310@qq.com> Date: Sun, 29 Jan 2023 02:26:47 +0800 Subject: [PATCH 2/5] fix --- accounts/abi/unpack.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/accounts/abi/unpack.go b/accounts/abi/unpack.go index 58f5a47b2d1e..47d803c16efd 100644 --- a/accounts/abi/unpack.go +++ b/accounts/abi/unpack.go @@ -38,25 +38,25 @@ func ReadInteger(typ Type, b []byte) (interface{}, error) { ret := new(big.Int).SetBytes(b) if typ.T == UintTy { - v64 := ret.Uint64() + v64, isu64 := ret.Uint64(), ret.IsUint64() switch typ.Size { case 8: - if v64 >= math.MaxUint8 { + if !isu64 || v64 >= math.MaxUint8 { return nil, errBadUint8 } return byte(v64), nil case 16: - if v64 >= math.MaxUint16 { + if !isu64 || v64 >= math.MaxUint16 { return nil, errBadUint16 } return uint16(v64), nil case 32: - if v64 >= math.MaxUint32 { + if !isu64 || v64 >= math.MaxUint32 { return nil, errBadUint32 } return uint32(v64), nil case 64: - if v64 > math.MaxUint64 { + if !isu64 || v64 > math.MaxUint64 { return nil, errBadUint64 } return v64, nil @@ -74,25 +74,25 @@ func ReadInteger(typ Type, b []byte) (interface{}, error) { ret.Add(ret, common.Big1) ret.Neg(ret) } - iv64 := ret.Int64() + iv64, isi64 := ret.Int64(), ret.IsInt64() switch typ.Size { case 8: - if iv64 < math.MinInt8 || iv64 > math.MaxInt8 { + if !isi64 || iv64 < math.MinInt8 || iv64 > math.MaxInt8 { return nil, errBadInt8 } return int8(iv64), nil case 16: - if iv64 < math.MinInt16 || iv64 > math.MaxInt16 { + if !isi64 || iv64 < math.MinInt16 || iv64 > math.MaxInt16 { return nil, errBadInt16 } return int16(iv64), nil case 32: - if iv64 < math.MinInt32 || iv64 > math.MaxInt32 { + if !isi64 || iv64 < math.MinInt32 || iv64 > math.MaxInt32 { return nil, errBadInt32 } return int32(iv64), nil case 64: - if iv64 < math.MinInt64 || iv64 > math.MaxInt64 { + if !isi64 || iv64 < math.MinInt64 || iv64 > math.MaxInt64 { return nil, errBadInt64 } return iv64, nil From 17e9024a5f60723f67a9fb7e5742915f031888ca Mon Sep 17 00:00:00 2001 From: zhiqiangxu <652732310@qq.com> Date: Sun, 29 Jan 2023 08:35:53 +0800 Subject: [PATCH 3/5] fix for ci --- accounts/abi/unpack.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/accounts/abi/unpack.go b/accounts/abi/unpack.go index 47d803c16efd..9e96b4968cbf 100644 --- a/accounts/abi/unpack.go +++ b/accounts/abi/unpack.go @@ -38,28 +38,28 @@ func ReadInteger(typ Type, b []byte) (interface{}, error) { ret := new(big.Int).SetBytes(b) if typ.T == UintTy { - v64, isu64 := ret.Uint64(), ret.IsUint64() + u64, isu64 := ret.Uint64(), ret.IsUint64() switch typ.Size { case 8: - if !isu64 || v64 >= math.MaxUint8 { + if !isu64 || u64 >= math.MaxUint8 { return nil, errBadUint8 } - return byte(v64), nil + return byte(u64), nil case 16: - if !isu64 || v64 >= math.MaxUint16 { + if !isu64 || u64 >= math.MaxUint16 { return nil, errBadUint16 } - return uint16(v64), nil + return uint16(u64), nil case 32: - if !isu64 || v64 >= math.MaxUint32 { + if !isu64 || u64 >= math.MaxUint32 { return nil, errBadUint32 } - return uint32(v64), nil + return uint32(u64), nil case 64: - if !isu64 || v64 > math.MaxUint64 { + if !isu64 { return nil, errBadUint64 } - return v64, nil + return u64, nil default: // the only case left for unsigned integer is uint256. return ret, nil @@ -74,28 +74,28 @@ func ReadInteger(typ Type, b []byte) (interface{}, error) { ret.Add(ret, common.Big1) ret.Neg(ret) } - iv64, isi64 := ret.Int64(), ret.IsInt64() + i64, isi64 := ret.Int64(), ret.IsInt64() switch typ.Size { case 8: - if !isi64 || iv64 < math.MinInt8 || iv64 > math.MaxInt8 { + if !isi64 || i64 < math.MinInt8 || i64 > math.MaxInt8 { return nil, errBadInt8 } - return int8(iv64), nil + return int8(i64), nil case 16: - if !isi64 || iv64 < math.MinInt16 || iv64 > math.MaxInt16 { + if !isi64 || i64 < math.MinInt16 || i64 > math.MaxInt16 { return nil, errBadInt16 } - return int16(iv64), nil + return int16(i64), nil case 32: - if !isi64 || iv64 < math.MinInt32 || iv64 > math.MaxInt32 { + if !isi64 || i64 < math.MinInt32 || i64 > math.MaxInt32 { return nil, errBadInt32 } - return int32(iv64), nil + return int32(i64), nil case 64: - if !isi64 || iv64 < math.MinInt64 || iv64 > math.MaxInt64 { + if !isi64 { return nil, errBadInt64 } - return iv64, nil + return i64, nil default: // the only case left for integer is int256 From f3a3c8969cf598eca64559a14431bd4fa6555886 Mon Sep 17 00:00:00 2001 From: zhiqiangxu <652732310@qq.com> Date: Mon, 30 Jan 2023 16:31:30 +0800 Subject: [PATCH 4/5] fix for review --- accounts/abi/unpack.go | 6 +- accounts/abi/unpack_test.go | 163 +++++++++++++++++++++++++++++++++--- 2 files changed, 153 insertions(+), 16 deletions(-) diff --git a/accounts/abi/unpack.go b/accounts/abi/unpack.go index 9e96b4968cbf..1e778a6ff0db 100644 --- a/accounts/abi/unpack.go +++ b/accounts/abi/unpack.go @@ -41,17 +41,17 @@ func ReadInteger(typ Type, b []byte) (interface{}, error) { u64, isu64 := ret.Uint64(), ret.IsUint64() switch typ.Size { case 8: - if !isu64 || u64 >= math.MaxUint8 { + if !isu64 || u64 > math.MaxUint8 { return nil, errBadUint8 } return byte(u64), nil case 16: - if !isu64 || u64 >= math.MaxUint16 { + if !isu64 || u64 > math.MaxUint16 { return nil, errBadUint16 } return uint16(u64), nil case 32: - if !isu64 || u64 >= math.MaxUint32 { + if !isu64 || u64 > math.MaxUint32 { return nil, errBadUint32 } return uint32(u64), nil diff --git a/accounts/abi/unpack_test.go b/accounts/abi/unpack_test.go index 0bb6df3c5b38..b667a496c805 100644 --- a/accounts/abi/unpack_test.go +++ b/accounts/abi/unpack_test.go @@ -20,6 +20,7 @@ import ( "bytes" "encoding/hex" "fmt" + "math" "math/big" "reflect" "strconv" @@ -945,27 +946,163 @@ func TestOOMMaliciousInput(t *testing.T) { } func TestPackAndUnpackIncompatibleNumber(t *testing.T) { - var encodeABI, decodeABI Arguments + + var encodeABI Arguments uint256Ty, err := NewType("uint256", "", nil) if err != nil { panic(err) } - uint8Ty, err := NewType("uint8", "", nil) - if err != nil { - panic(err) - } encodeABI = Arguments{ {Type: uint256Ty}, } - decodeABI = Arguments{ - {Type: uint8Ty}, + + maxU64, ok := new(big.Int).SetString(strconv.FormatUint(math.MaxUint64, 10), 10) + if !ok { + panic("bug") } - packed, err := encodeABI.Pack(big.NewInt(256)) - if err != nil { - panic(err) + 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), + }, } - _, err = decodeABI.Unpack(packed) - if err != errBadUint8 { - t.Fatal(err) + 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]) + } } } From 706e2401bb753808b233ca5629757be818acbe87 Mon Sep 17 00:00:00 2001 From: zhiqiangxu <652732310@qq.com> Date: Mon, 30 Jan 2023 19:40:40 +0800 Subject: [PATCH 5/5] make ci happy --- accounts/abi/unpack_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/accounts/abi/unpack_test.go b/accounts/abi/unpack_test.go index b667a496c805..6dd2db0d583d 100644 --- a/accounts/abi/unpack_test.go +++ b/accounts/abi/unpack_test.go @@ -946,7 +946,6 @@ func TestOOMMaliciousInput(t *testing.T) { } func TestPackAndUnpackIncompatibleNumber(t *testing.T) { - var encodeABI Arguments uint256Ty, err := NewType("uint256", "", nil) if err != nil {