Skip to content

Commit a6b099f

Browse files
jwasingerMariusVanDerWijden
authored and
howjmay
committed
accounts/abi: error when packing negative values in unsigned types (ethereum#31790)
This is an alternative approach to ethereum#31607 , that doesn't break backwards-compatibility with abigen. Note that this does change the behavior of `Argument.Pack`: previously, packing negative values for a `uint` parameter would cause them to be represented in signed binary representation via two's complement. Now, it will fail explicitly in this case. However, I don't see a reason to support this functionality. The ABI already explicitly supports signed integers. There's no reason that a smart contract author would choose to store signed values in a `uint` afaict. --------- Co-authored-by: MariusVanDerWijden <m.vanderwijden@live.de>
1 parent ff91c31 commit a6b099f

File tree

4 files changed

+61
-40
lines changed

4 files changed

+61
-40
lines changed

‎accounts/abi/error_handling.go‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,16 @@ import (
2323
)
2424

2525
var (
26-
errBadBool = errors.New("abi: improperly encoded boolean value")
27-
errBadUint8 = errors.New("abi: improperly encoded uint8 value")
28-
errBadUint16 = errors.New("abi: improperly encoded uint16 value")
29-
errBadUint32 = errors.New("abi: improperly encoded uint32 value")
30-
errBadUint64 = errors.New("abi: improperly encoded uint64 value")
31-
errBadInt8 = errors.New("abi: improperly encoded int8 value")
32-
errBadInt16 = errors.New("abi: improperly encoded int16 value")
33-
errBadInt32 = errors.New("abi: improperly encoded int32 value")
34-
errBadInt64 = errors.New("abi: improperly encoded int64 value")
26+
errBadBool = errors.New("abi: improperly encoded boolean value")
27+
errBadUint8 = errors.New("abi: improperly encoded uint8 value")
28+
errBadUint16 = errors.New("abi: improperly encoded uint16 value")
29+
errBadUint32 = errors.New("abi: improperly encoded uint32 value")
30+
errBadUint64 = errors.New("abi: improperly encoded uint64 value")
31+
errBadInt8 = errors.New("abi: improperly encoded int8 value")
32+
errBadInt16 = errors.New("abi: improperly encoded int16 value")
33+
errBadInt32 = errors.New("abi: improperly encoded int32 value")
34+
errBadInt64 = errors.New("abi: improperly encoded int64 value")
35+
errInvalidSign = errors.New("abi: negatively-signed value cannot be packed into uint parameter")
3536
)
3637

3738
// formatSliceString formats the reflection kind with the given slice size

‎accounts/abi/pack.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,16 @@ func packBytesSlice(bytes []byte, l int) []byte {
3737
// t.
3838
func packElement(t Type, reflectValue reflect.Value) ([]byte, error) {
3939
switch t.T {
40-
case IntTy, UintTy:
40+
case UintTy:
41+
// make sure to not pack a negative value into a uint type.
42+
if reflectValue.Kind() == reflect.Ptr {
43+
val := new(big.Int).Set(reflectValue.Interface().(*big.Int))
44+
if val.Sign() == -1 {
45+
return nil, errInvalidSign
46+
}
47+
}
48+
return packNum(reflectValue), nil
49+
case IntTy:
4150
return packNum(reflectValue), nil
4251
case StringTy:
4352
return packBytesSlice([]byte(reflectValue.String()), reflectValue.Len()), nil

‎accounts/abi/pack_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,11 @@ func TestMethodPack(t *testing.T) {
177177
if !bytes.Equal(packed, sig) {
178178
t.Errorf("expected %x got %x", sig, packed)
179179
}
180+
181+
// test that we can't pack a negative value for a parameter that is specified as a uint
182+
if _, err := abi.Pack("send", big.NewInt(-1)); err == nil {
183+
t.Fatal("expected error when trying to pack negative big.Int into uint256 value")
184+
}
180185
}
181186

182187
func TestPackNumber(t *testing.T) {

‎accounts/abi/unpack_test.go‎

Lines changed: 36 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,128 +1014,134 @@ func TestPackAndUnpackIncompatibleNumber(t *testing.T) {
10141014
cases := []struct {
10151015
decodeType string
10161016
inputValue *big.Int
1017-
err error
1017+
unpackErr error
1018+
packErr error
10181019
expectValue interface{}
10191020
}{
10201021
{
10211022
decodeType: "uint8",
10221023
inputValue: big.NewInt(math.MaxUint8 + 1),
1023-
err: errBadUint8,
1024+
unpackErr: errBadUint8,
10241025
},
10251026
{
10261027
decodeType: "uint8",
10271028
inputValue: big.NewInt(math.MaxUint8),
1028-
err: nil,
1029+
unpackErr: nil,
10291030
expectValue: uint8(math.MaxUint8),
10301031
},
10311032
{
10321033
decodeType: "uint16",
10331034
inputValue: big.NewInt(math.MaxUint16 + 1),
1034-
err: errBadUint16,
1035+
unpackErr: errBadUint16,
10351036
},
10361037
{
10371038
decodeType: "uint16",
10381039
inputValue: big.NewInt(math.MaxUint16),
1039-
err: nil,
1040+
unpackErr: nil,
10401041
expectValue: uint16(math.MaxUint16),
10411042
},
10421043
{
10431044
decodeType: "uint32",
10441045
inputValue: big.NewInt(math.MaxUint32 + 1),
1045-
err: errBadUint32,
1046+
unpackErr: errBadUint32,
10461047
},
10471048
{
10481049
decodeType: "uint32",
10491050
inputValue: big.NewInt(math.MaxUint32),
1050-
err: nil,
1051+
unpackErr: nil,
10511052
expectValue: uint32(math.MaxUint32),
10521053
},
10531054
{
10541055
decodeType: "uint64",
10551056
inputValue: maxU64Plus1,
1056-
err: errBadUint64,
1057+
unpackErr: errBadUint64,
10571058
},
10581059
{
10591060
decodeType: "uint64",
10601061
inputValue: maxU64,
1061-
err: nil,
1062+
unpackErr: nil,
10621063
expectValue: uint64(math.MaxUint64),
10631064
},
10641065
{
10651066
decodeType: "uint256",
10661067
inputValue: maxU64Plus1,
1067-
err: nil,
1068+
unpackErr: nil,
10681069
expectValue: maxU64Plus1,
10691070
},
10701071
{
10711072
decodeType: "int8",
10721073
inputValue: big.NewInt(math.MaxInt8 + 1),
1073-
err: errBadInt8,
1074+
unpackErr: errBadInt8,
10741075
},
10751076
{
1076-
decodeType: "int8",
10771077
inputValue: big.NewInt(math.MinInt8 - 1),
1078-
err: errBadInt8,
1078+
packErr: errInvalidSign,
10791079
},
10801080
{
10811081
decodeType: "int8",
10821082
inputValue: big.NewInt(math.MaxInt8),
1083-
err: nil,
1083+
unpackErr: nil,
10841084
expectValue: int8(math.MaxInt8),
10851085
},
10861086
{
10871087
decodeType: "int16",
10881088
inputValue: big.NewInt(math.MaxInt16 + 1),
1089-
err: errBadInt16,
1089+
unpackErr: errBadInt16,
10901090
},
10911091
{
1092-
decodeType: "int16",
10931092
inputValue: big.NewInt(math.MinInt16 - 1),
1094-
err: errBadInt16,
1093+
packErr: errInvalidSign,
10951094
},
10961095
{
10971096
decodeType: "int16",
10981097
inputValue: big.NewInt(math.MaxInt16),
1099-
err: nil,
1098+
unpackErr: nil,
11001099
expectValue: int16(math.MaxInt16),
11011100
},
11021101
{
11031102
decodeType: "int32",
11041103
inputValue: big.NewInt(math.MaxInt32 + 1),
1105-
err: errBadInt32,
1104+
unpackErr: errBadInt32,
11061105
},
11071106
{
1108-
decodeType: "int32",
11091107
inputValue: big.NewInt(math.MinInt32 - 1),
1110-
err: errBadInt32,
1108+
packErr: errInvalidSign,
11111109
},
11121110
{
11131111
decodeType: "int32",
11141112
inputValue: big.NewInt(math.MaxInt32),
1115-
err: nil,
1113+
unpackErr: nil,
11161114
expectValue: int32(math.MaxInt32),
11171115
},
11181116
{
11191117
decodeType: "int64",
11201118
inputValue: new(big.Int).Add(big.NewInt(math.MaxInt64), big.NewInt(1)),
1121-
err: errBadInt64,
1119+
unpackErr: errBadInt64,
11221120
},
11231121
{
1124-
decodeType: "int64",
11251122
inputValue: new(big.Int).Sub(big.NewInt(math.MinInt64), big.NewInt(1)),
1126-
err: errBadInt64,
1123+
packErr: errInvalidSign,
11271124
},
11281125
{
11291126
decodeType: "int64",
11301127
inputValue: big.NewInt(math.MaxInt64),
1131-
err: nil,
1128+
unpackErr: nil,
11321129
expectValue: int64(math.MaxInt64),
11331130
},
11341131
}
11351132
for i, testCase := range cases {
11361133
packed, err := encodeABI.Pack(testCase.inputValue)
1137-
if err != nil {
1138-
panic(err)
1134+
if testCase.packErr != nil {
1135+
if err == nil {
1136+
t.Fatalf("expected packing of testcase input value to fail")
1137+
}
1138+
if err != testCase.packErr {
1139+
t.Fatalf("expected error '%v', got '%v'", testCase.packErr, err)
1140+
}
1141+
continue
1142+
}
1143+
if err != nil && err != testCase.packErr {
1144+
panic(fmt.Errorf("unexpected error packing test-case input: %v", err))
11391145
}
11401146
ty, err := NewType(testCase.decodeType, "", nil)
11411147
if err != nil {
@@ -1145,8 +1151,8 @@ func TestPackAndUnpackIncompatibleNumber(t *testing.T) {
11451151
{Type: ty},
11461152
}
11471153
decoded, err := decodeABI.Unpack(packed)
1148-
if err != testCase.err {
1149-
t.Fatalf("Expected error %v, actual error %v. case %d", testCase.err, err, i)
1154+
if err != testCase.unpackErr {
1155+
t.Fatalf("Expected error %v, actual error %v. case %d", testCase.unpackErr, err, i)
11501156
}
11511157
if err != nil {
11521158
continue

0 commit comments

Comments
 (0)