From 2e947b7a0041f087ce4945303f3dd267b6296a14 Mon Sep 17 00:00:00 2001
From: protolambda <proto@protolambda.com>
Date: Sat, 27 Jan 2024 14:16:20 -0600
Subject: [PATCH] core/types: fix and test handling of faulty nil-returning
 signer (#28879)

This adds an error if the signer returns a nil value for one of the signature value fields.
---
 core/types/transaction.go              |  5 +++
 core/types/transaction_signing_test.go | 52 ++++++++++++++++++++++++++
 core/types/tx_blob_test.go             |  9 ++++-
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/core/types/transaction.go b/core/types/transaction.go
index 9ec0199a0..7d2e9d532 100644
--- a/core/types/transaction.go
+++ b/core/types/transaction.go
@@ -19,6 +19,7 @@ package types
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"math/big"
 	"sync/atomic"
@@ -320,6 +321,7 @@ func (tx *Transaction) Cost() *big.Int {
 
 // RawSignatureValues returns the V, R, S signature values of the transaction.
 // The return values should not be modified by the caller.
+// The return values may be nil or zero, if the transaction is unsigned.
 func (tx *Transaction) RawSignatureValues() (v, r, s *big.Int) {
 	return tx.inner.rawSignatureValues()
 }
@@ -508,6 +510,9 @@ func (tx *Transaction) WithSignature(signer Signer, sig []byte) (*Transaction, e
 	if err != nil {
 		return nil, err
 	}
+	if r == nil || s == nil || v == nil {
+		return nil, fmt.Errorf("%w: r: %s, s: %s, v: %s", ErrInvalidSig, r, s, v)
+	}
 	cpy := tx.inner.copy()
 	cpy.setSignatureValues(signer.ChainID(), v, r, s)
 	return &Transaction{inner: cpy, time: tx.time}, nil
diff --git a/core/types/transaction_signing_test.go b/core/types/transaction_signing_test.go
index 2a9ceb095..61b78fe02 100644
--- a/core/types/transaction_signing_test.go
+++ b/core/types/transaction_signing_test.go
@@ -18,11 +18,13 @@ package types
 
 import (
 	"errors"
+	"fmt"
 	"math/big"
 	"testing"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/crypto"
+	"github.com/ethereum/go-ethereum/params"
 	"github.com/ethereum/go-ethereum/rlp"
 )
 
@@ -136,3 +138,53 @@ func TestChainId(t *testing.T) {
 		t.Error("expected no error")
 	}
 }
+
+type nilSigner struct {
+	v, r, s *big.Int
+	Signer
+}
+
+func (ns *nilSigner) SignatureValues(tx *Transaction, sig []byte) (r, s, v *big.Int, err error) {
+	return ns.v, ns.r, ns.s, nil
+}
+
+// TestNilSigner ensures a faulty Signer implementation does not result in nil signature values or panics.
+func TestNilSigner(t *testing.T) {
+	key, _ := crypto.GenerateKey()
+	innerSigner := LatestSignerForChainID(big.NewInt(1))
+	for i, signer := range []Signer{
+		&nilSigner{v: nil, r: nil, s: nil, Signer: innerSigner},
+		&nilSigner{v: big.NewInt(1), r: big.NewInt(1), s: nil, Signer: innerSigner},
+		&nilSigner{v: big.NewInt(1), r: nil, s: big.NewInt(1), Signer: innerSigner},
+		&nilSigner{v: nil, r: big.NewInt(1), s: big.NewInt(1), Signer: innerSigner},
+	} {
+		t.Run(fmt.Sprintf("signer_%d", i), func(t *testing.T) {
+			t.Run("legacy", func(t *testing.T) {
+				legacyTx := createTestLegacyTxInner()
+				_, err := SignNewTx(key, signer, legacyTx)
+				if !errors.Is(err, ErrInvalidSig) {
+					t.Fatal("expected signature values error, no nil result or panic")
+				}
+			})
+			// test Blob tx specifically, since the signature value types changed
+			t.Run("blobtx", func(t *testing.T) {
+				blobtx := createEmptyBlobTxInner(false)
+				_, err := SignNewTx(key, signer, blobtx)
+				if !errors.Is(err, ErrInvalidSig) {
+					t.Fatal("expected signature values error, no nil result or panic")
+				}
+			})
+		})
+	}
+}
+
+func createTestLegacyTxInner() *LegacyTx {
+	return &LegacyTx{
+		Nonce:    uint64(0),
+		To:       nil,
+		Value:    big.NewInt(0),
+		Gas:      params.TxGas,
+		GasPrice: big.NewInt(params.GWei),
+		Data:     nil,
+	}
+}
diff --git a/core/types/tx_blob_test.go b/core/types/tx_blob_test.go
index 44ac48cc6..25d09e31c 100644
--- a/core/types/tx_blob_test.go
+++ b/core/types/tx_blob_test.go
@@ -65,6 +65,12 @@ var (
 )
 
 func createEmptyBlobTx(key *ecdsa.PrivateKey, withSidecar bool) *Transaction {
+	blobtx := createEmptyBlobTxInner(withSidecar)
+	signer := NewCancunSigner(blobtx.ChainID.ToBig())
+	return MustSignNewTx(key, signer, blobtx)
+}
+
+func createEmptyBlobTxInner(withSidecar bool) *BlobTx {
 	sidecar := &BlobTxSidecar{
 		Blobs:       []kzg4844.Blob{emptyBlob},
 		Commitments: []kzg4844.Commitment{emptyBlobCommit},
@@ -85,6 +91,5 @@ func createEmptyBlobTx(key *ecdsa.PrivateKey, withSidecar bool) *Transaction {
 	if withSidecar {
 		blobtx.Sidecar = sidecar
 	}
-	signer := NewCancunSigner(blobtx.ChainID.ToBig())
-	return MustSignNewTx(key, signer, blobtx)
+	return blobtx
 }