accounts/abi: error when packing negative values in unsigned types (#31790)

This is an alternative approach to
https://github.com/ethereum/go-ethereum/pull/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>

accounts/abi/error_handling.go

@@ -23,15 +23,16 @@ 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")
+	errBadUint8    = errors.New("abi: improperly encoded uint8 value")
-	errBadUint16 = errors.New("abi: improperly encoded uint16 value")
+	errBadUint16   = errors.New("abi: improperly encoded uint16 value")
-	errBadUint32 = errors.New("abi: improperly encoded uint32 value")
+	errBadUint32   = errors.New("abi: improperly encoded uint32 value")
-	errBadUint64 = errors.New("abi: improperly encoded uint64 value")
+	errBadUint64   = errors.New("abi: improperly encoded uint64 value")
-	errBadInt8   = errors.New("abi: improperly encoded int8 value")
+	errBadInt8     = errors.New("abi: improperly encoded int8 value")
-	errBadInt16  = errors.New("abi: improperly encoded int16 value")
+	errBadInt16    = errors.New("abi: improperly encoded int16 value")
-	errBadInt32  = errors.New("abi: improperly encoded int32 value")
+	errBadInt32    = errors.New("abi: improperly encoded int32 value")
-	errBadInt64  = errors.New("abi: improperly encoded int64 value")
+	errBadInt64    = errors.New("abi: improperly encoded int64 value")
+	errInvalidSign = errors.New("abi: negatively-signed value cannot be packed into uint parameter")
 )
 
 // formatSliceString formats the reflection kind with the given slice size

accounts/abi/pack.go

@@ -37,7 +37,16 @@ func packBytesSlice(bytes []byte, l int) []byte {
 // t.
 func packElement(t Type, reflectValue reflect.Value) ([]byte, error) {
 	switch t.T {
-	case IntTy, UintTy:
+	case UintTy:
+		// make sure to not pack a negative value into a uint type.
+		if reflectValue.Kind() == reflect.Ptr {
+			val := new(big.Int).Set(reflectValue.Interface().(*big.Int))
+			if val.Sign() == -1 {
+				return nil, errInvalidSign
+			}
+		}
+		return packNum(reflectValue), nil
+	case IntTy:
 		return packNum(reflectValue), nil
 	case StringTy:
 		return packBytesSlice([]byte(reflectValue.String()), reflectValue.Len()), nil

accounts/abi/pack_test.go

@@ -177,6 +177,11 @@ func TestMethodPack(t *testing.T) {
 	if !bytes.Equal(packed, sig) {
 		t.Errorf("expected %x got %x", sig, packed)
 	}
+
+	// test that we can't pack a negative value for a parameter that is specified as a uint
+	if _, err := abi.Pack("send", big.NewInt(-1)); err == nil {
+		t.Fatal("expected error when trying to pack negative big.Int into uint256 value")
+	}
 }
 
 func TestPackNumber(t *testing.T) {

accounts/abi/unpack_test.go

@@ -1014,128 +1014,134 @@ func TestPackAndUnpackIncompatibleNumber(t *testing.T) {
 	cases := []struct {
 		decodeType  string
 		inputValue  *big.Int
-		err         error
+		unpackErr   error
+		packErr     error
 		expectValue interface{}
 	}{
 		{
 			decodeType: "uint8",
 			inputValue: big.NewInt(math.MaxUint8 + 1),
-			err:        errBadUint8,
+			unpackErr:  errBadUint8,
 		},
 		{
 			decodeType:  "uint8",
 			inputValue:  big.NewInt(math.MaxUint8),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: uint8(math.MaxUint8),
 		},
 		{
 			decodeType: "uint16",
 			inputValue: big.NewInt(math.MaxUint16 + 1),
-			err:        errBadUint16,
+			unpackErr:  errBadUint16,
 		},
 		{
 			decodeType:  "uint16",
 			inputValue:  big.NewInt(math.MaxUint16),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: uint16(math.MaxUint16),
 		},
 		{
 			decodeType: "uint32",
 			inputValue: big.NewInt(math.MaxUint32 + 1),
-			err:        errBadUint32,
+			unpackErr:  errBadUint32,
 		},
 		{
 			decodeType:  "uint32",
 			inputValue:  big.NewInt(math.MaxUint32),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: uint32(math.MaxUint32),
 		},
 		{
 			decodeType: "uint64",
 			inputValue: maxU64Plus1,
-			err:        errBadUint64,
+			unpackErr:  errBadUint64,
 		},
 		{
 			decodeType:  "uint64",
 			inputValue:  maxU64,
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: uint64(math.MaxUint64),
 		},
 		{
 			decodeType:  "uint256",
 			inputValue:  maxU64Plus1,
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: maxU64Plus1,
 		},
 		{
 			decodeType: "int8",
 			inputValue: big.NewInt(math.MaxInt8 + 1),
-			err:        errBadInt8,
+			unpackErr:  errBadInt8,
 		},
 		{
-			decodeType: "int8",
 			inputValue: big.NewInt(math.MinInt8 - 1),
-			err:        errBadInt8,
+			packErr:    errInvalidSign,
 		},
 		{
 			decodeType:  "int8",
 			inputValue:  big.NewInt(math.MaxInt8),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: int8(math.MaxInt8),
 		},
 		{
 			decodeType: "int16",
 			inputValue: big.NewInt(math.MaxInt16 + 1),
-			err:        errBadInt16,
+			unpackErr:  errBadInt16,
 		},
 		{
-			decodeType: "int16",
 			inputValue: big.NewInt(math.MinInt16 - 1),
-			err:        errBadInt16,
+			packErr:    errInvalidSign,
 		},
 		{
 			decodeType:  "int16",
 			inputValue:  big.NewInt(math.MaxInt16),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: int16(math.MaxInt16),
 		},
 		{
 			decodeType: "int32",
 			inputValue: big.NewInt(math.MaxInt32 + 1),
-			err:        errBadInt32,
+			unpackErr:  errBadInt32,
 		},
 		{
-			decodeType: "int32",
 			inputValue: big.NewInt(math.MinInt32 - 1),
-			err:        errBadInt32,
+			packErr:    errInvalidSign,
 		},
 		{
 			decodeType:  "int32",
 			inputValue:  big.NewInt(math.MaxInt32),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: int32(math.MaxInt32),
 		},
 		{
 			decodeType: "int64",
 			inputValue: new(big.Int).Add(big.NewInt(math.MaxInt64), big.NewInt(1)),
-			err:        errBadInt64,
+			unpackErr:  errBadInt64,
 		},
 		{
-			decodeType: "int64",
 			inputValue: new(big.Int).Sub(big.NewInt(math.MinInt64), big.NewInt(1)),
-			err:        errBadInt64,
+			packErr:    errInvalidSign,
 		},
 		{
 			decodeType:  "int64",
 			inputValue:  big.NewInt(math.MaxInt64),
-			err:         nil,
+			unpackErr:   nil,
 			expectValue: int64(math.MaxInt64),
 		},
 	}
 	for i, testCase := range cases {
 		packed, err := encodeABI.Pack(testCase.inputValue)
-		if err != nil {
+		if testCase.packErr != nil {
-			panic(err)
+			if err == nil {
+				t.Fatalf("expected packing of testcase input value to fail")
+			}
+			if err != testCase.packErr {
+				t.Fatalf("expected error '%v', got '%v'", testCase.packErr, err)
+			}
+			continue
+		}
+		if err != nil && err != testCase.packErr {
+			panic(fmt.Errorf("unexpected error packing test-case input: %v", err))
 		}
 		ty, err := NewType(testCase.decodeType, "", nil)
 		if err != nil {
@@ -1145,8 +1151,8 @@ func TestPackAndUnpackIncompatibleNumber(t *testing.T) {
 			{Type: ty},
 		}
 		decoded, err := decodeABI.Unpack(packed)
-		if err != testCase.err {
+		if err != testCase.unpackErr {
-			t.Fatalf("Expected error %v, actual error %v. case %d", testCase.err, err, i)
+			t.Fatalf("Expected error %v, actual error %v. case %d", testCase.unpackErr, err, i)
 		}
 		if err != nil {
 			continue