enforce low S in transaction signature only in tx pool (#1575)

Co-authored-by: vanjatomic <vanja.tomic@vechain.org>
Co-authored-by: paologalligit <paolo.galli@vechain.org>

Author: 3 people

api/transactions/transactions_test.go

@@ -323,8 +323,10 @@ func sendTxWithBadFormat(t *testing.T) {
 }
 
 func sendTxThatCannotBeAcceptedInLocalMempool(t *testing.T) {
-	tx := tx.NewBuilder(tx.TypeLegacy).Build()
-	rlpTx, err := tx.MarshalBinary()
+	trx := tx.NewBuilder(tx.TypeLegacy).Build()
+	trx = tx.MustSign(trx, genesis.DevAccounts()[0].PrivateKey)
+
+	rlpTx, err := trx.MarshalBinary()
 	if err != nil {
 		t.Fatal(err)
 	}

comm/handle_rpc.go

@@ -10,6 +10,8 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/vechain/thor/v2/txpool"
+
 	"github.com/ethereum/go-ethereum/rlp"
 	"github.com/pkg/errors"
 
@@ -20,6 +22,8 @@ import (
 	"github.com/vechain/thor/v2/tx"
 )
 
+var maxTxSize = uint32(txpool.MaxTxSize + 1024)
+
 // peer will be disconnected if error returned
 func (c *Communicator) handleRPC(peer *Peer, msg *p2p.Msg, write func(any), txsToSync *txsToSync) (err error) {
 	log := peer.logger.New("msg", proto.MsgName(msg.Code))
@@ -66,6 +70,9 @@ func (c *Communicator) handleRPC(peer *Peer, msg *p2p.Msg, write func(any), txsT
 		}
 		write(&struct{}{})
 	case proto.MsgNewTx:
+		if msg.Size > maxTxSize {
+			return errors.New("payload size: exceeds limit")
+		}
 		var newTx *tx.Transaction
 		if err := msg.Decode(&newTx); err != nil {
 			return errors.WithMessage(err, "decode msg")

comm/handle_rpc_test.go

@@ -0,0 +1,161 @@
+// Copyright (c) 2025 The VeChainThor developers
+
+// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying
+// file LICENSE or <https://www.gnu.org/licenses/lgpl-3.0.html>
+
+package comm
+
+import (
+	"bytes"
+	"math/big"
+	"testing"
+	"time"
+
+	"github.com/ethereum/go-ethereum/rlp"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/vechain/thor/v2/comm/proto"
+	"github.com/vechain/thor/v2/genesis"
+	"github.com/vechain/thor/v2/p2p"
+	"github.com/vechain/thor/v2/p2p/discover"
+	"github.com/vechain/thor/v2/test/testchain"
+	"github.com/vechain/thor/v2/thor"
+	"github.com/vechain/thor/v2/tx"
+	"github.com/vechain/thor/v2/txpool"
+)
+
+func TestHandleRPC_MsgNewTx(t *testing.T) {
+	chain, err := testchain.NewWithFork(&thor.SoloFork, 180)
+	require.NoError(t, err)
+	repo := chain.Repo()
+
+	pool := txpool.New(repo, chain.Stater(), txpool.Options{
+		Limit:           10000,
+		LimitPerAccount: 16,
+		MaxLifetime:     10 * time.Minute,
+	}, &thor.SoloFork)
+	defer pool.Close()
+
+	comm := New(repo, pool)
+	peer := newPeer(p2p.NewPeer(discover.NodeID{}, "test", nil), stubMsgReadWriter{})
+
+	to, _ := thor.ParseAddress("0x7567d83b7b8d80addcb281a71d54fc7b3364ffed")
+	chainTag := repo.ChainTag()
+	testTx := tx.NewBuilder(tx.TypeLegacy).
+		ChainTag(chainTag).
+		BlockRef(tx.NewBlockRef(0)).
+		Expiration(100).
+		GasPriceCoef(0).
+		Gas(21000).
+		Nonce(1).
+		Clause(tx.NewClause(&to).WithValue(big.NewInt(1))).
+		Build()
+	testTx = tx.MustSign(testTx, genesis.DevAccounts()[0].PrivateKey)
+	txHash := testTx.Hash()
+	txID := testTx.ID()
+
+	txData, err := rlp.EncodeToBytes(testTx)
+	require.NoError(t, err)
+
+	t.Run("valid transaction", func(t *testing.T) {
+		writeCalled := false
+		var writtenData any
+
+		write := func(data any) {
+			writeCalled = true
+			writtenData = data
+		}
+
+		msg := &p2p.Msg{
+			Code:    proto.MsgNewTx,
+			Size:    uint32(len(txData)),
+			Payload: bytes.NewReader(txData),
+		}
+
+		txsToSync := &txsToSync{}
+
+		err := comm.handleRPC(peer, msg, write, txsToSync)
+		assert.NoError(t, err)
+
+		assert.True(t, peer.IsTransactionKnown(txHash), "transaction should be marked on peer")
+
+		addedTx := pool.Get(txID)
+		assert.NotNil(t, addedTx, "transaction should be added to pool")
+		if addedTx != nil {
+			assert.Equal(t, testTx.Hash(), addedTx.Hash(), "added transaction should match")
+		}
+
+		assert.True(t, writeCalled, "write function should be called")
+		assert.Equal(t, &struct{}{}, writtenData, "write should be called with empty struct")
+	})
+
+	t.Run("transaction exceeds size limit", func(t *testing.T) {
+		writeCalled := false
+
+		write := func(data any) {
+			writeCalled = true
+		}
+
+		msg := &p2p.Msg{
+			Code:    proto.MsgNewTx,
+			Size:    maxTxSize + 1,
+			Payload: bytes.NewReader(txData),
+		}
+
+		txsToSync := &txsToSync{}
+
+		err := comm.handleRPC(peer, msg, write, txsToSync)
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "payload size: exceeds limit")
+
+		assert.False(t, writeCalled, "write should not be called on error")
+	})
+
+	t.Run("decode error", func(t *testing.T) {
+		writeCalled := false
+
+		write := func(data any) {
+			writeCalled = true
+		}
+
+		invalidData := []byte{0x01, 0x02, 0x03}
+		msg := &p2p.Msg{
+			Code:    proto.MsgNewTx,
+			Size:    uint32(len(invalidData)),
+			Payload: bytes.NewReader(invalidData),
+		}
+
+		txsToSync := &txsToSync{}
+
+		err := comm.handleRPC(peer, msg, write, txsToSync)
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "decode msg")
+
+		assert.False(t, writeCalled, "write should not be called on error")
+	})
+
+	t.Run("transaction at size limit boundary", func(t *testing.T) {
+		writeCalled := false
+		var writtenData any
+
+		write := func(data any) {
+			writeCalled = true
+			writtenData = data
+		}
+
+		msg := &p2p.Msg{
+			Code:    proto.MsgNewTx,
+			Size:    maxTxSize,
+			Payload: bytes.NewReader(txData),
+		}
+
+		txsToSync := &txsToSync{}
+
+		err := comm.handleRPC(peer, msg, write, txsToSync)
+		assert.NoError(t, err)
+
+		assert.True(t, writeCalled, "write function should be called")
+		assert.Equal(t, &struct{}{}, writtenData, "write should be called with empty struct")
+	})
+}

p2p/peer_error.go

@@ -89,7 +89,7 @@ var discReasonToString = [...]string{
 }
 
 func (d DiscReason) String() string {
-	if len(discReasonToString) < int(d) {
+	if len(discReasonToString) <= int(d) || discReasonToString[d] == "" {
 		return fmt.Sprintf("unknown disconnect reason %d", d)
 	}
 	return discReasonToString[d]

tx/transaction.go

@@ -26,9 +26,21 @@ import (
 
 var (
 	ErrTxTypeNotSupported = errors.New("transaction type not supported")
+	ErrHighSInSignature   = errors.New("invalid signature: S value is out of range")
 
 	errIntrinsicGasOverflow = errors.New("intrinsic gas overflow")
 	errShortTypedTx         = errors.New("typed transaction too short")
+
+	// secp256k1HalfN is N/2 precomputed as 32-byte big-endian array for efficient low-s check.
+	// N = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
+	// N/2 = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0
+	secp256k1HalfN = func() [32]byte {
+		n := crypto.S256().Params().N
+		halfN := new(big.Int).Rsh(n, 1)
+		var result [32]byte
+		halfN.FillBytes(result[:])
+		return result
+	}()
 )
 
 type Type = byte
@@ -627,6 +639,32 @@ func (t *Transaction) validateSignatureLength() error {
 	return nil
 }
 
+// EnforceSignatureLowS checks that the S value in the signature are <= secp256k1 N/2.
+// This is not required for consensus, but a protection against signature malleability.
+func (t *Transaction) EnforceSignatureLowS() error {
+	if err := t.validateSignatureLength(); err != nil {
+		return err
+	}
+
+	sig := t.body.signature()
+
+	// Check first signature's S (bytes 32:64)
+	if len(sig) == 65 {
+		if bytes.Compare(sig[32:64], secp256k1HalfN[:]) > 0 {
+			return ErrHighSInSignature
+		}
+	}
+
+	// Check delegator signature's S if present (bytes 97:129)
+	if len(sig) == 130 {
+		if bytes.Compare(sig[65+32:65+64], secp256k1HalfN[:]) > 0 {
+			return ErrHighSInSignature
+		}
+	}
+
+	return nil
+}
+
 // IntrinsicGas calculate intrinsic gas cost for tx with such clauses.
 func IntrinsicGas(clauses ...*Clause) (uint64, error) {
 	if len(clauses) == 0 {

tx/transaction_test.go

@@ -791,3 +791,57 @@ func TestOverallGasPrice_LegacyWithProvedWork(t *testing.T) {
 	res := trx.OverallGasPrice(baseGasPrice, provedWork)
 	assert.NotNil(t, res)
 }
+
+var (
+	// precomputed secp256k1 N/2 as bytes for benchmark
+	benchHalfNBytes = func() [32]byte {
+		n := crypto.S256().Params().N
+		halfN := new(big.Int).Rsh(n, 1)
+		var result [32]byte
+		halfN.FillBytes(result[:])
+		return result
+	}()
+	// precomputed as big.Int for benchmark
+	benchHalfNBigInt = new(big.Int).Rsh(crypto.S256().Params().N, 1)
+)
+
+func BenchmarkLowSCheck(b *testing.B) {
+	// Generate a realistic S value (32 bytes, close to halfN)
+	sBytes := make([]byte, 32)
+	copy(sBytes, []byte{
+		0x7F, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+		0x5D, 0x57, 0x6E, 0x73, 0x57, 0xA4, 0x50, 0x1D,
+		0xDF, 0xE9, 0x2F, 0x46, 0x68, 0x1B, 0x20, 0x00,
+	})
+
+	b.Run("BigInt", func(b *testing.B) {
+		halfN := benchHalfNBigInt
+		b.ReportAllocs()
+		b.ResetTimer()
+		for b.Loop() {
+			s := new(big.Int).SetBytes(sBytes)
+			_ = s.Cmp(halfN) > 0
+		}
+	})
+
+	b.Run("BigInt_Reuse", func(b *testing.B) {
+		halfN := benchHalfNBigInt
+		s := new(big.Int)
+		b.ReportAllocs()
+		b.ResetTimer()
+		for b.Loop() {
+			s.SetBytes(sBytes)
+			_ = s.Cmp(halfN) > 0
+		}
+	})
+
+	b.Run("BytesCompare", func(b *testing.B) {
+		halfN := benchHalfNBytes[:]
+		b.ReportAllocs()
+		b.ResetTimer()
+		for b.Loop() {
+			_ = bytes.Compare(sBytes, halfN) > 0
+		}
+	})
+}

txpool/tx_pool.go

@@ -240,10 +240,6 @@ func (p *TxPool) add(newTx *tx.Transaction, rejectNonExecutable bool, localSubmi
 			metricBadTxGauge().AddWithLabel(1, map[string]string{"source": source})
 		}
 	}()
-	txTypeString := "Legacy"
-	if newTx.Type() == tx.TypeDynamicFee {
-		txTypeString = "DynamicFee"
-	}
 
 	if p.all.ContainsHash(newTx.Hash()) {
 		// tx already in the pool
@@ -274,7 +270,13 @@ func (p *TxPool) add(newTx *tx.Transaction, rejectNonExecutable bool, localSubmi
 	}
 
 	atomic.AddUint32(&p.addedAfterWash, 1)
+
+	txTypeString := "Legacy"
+	if newTx.Type() == tx.TypeDynamicFee {
+		txTypeString = "DynamicFee"
+	}
 	metricTxPoolGauge().AddWithLabel(1, map[string]string{"source": source, "type": txTypeString})
+
 	return nil
 }
 
@@ -697,6 +699,10 @@ func (p *TxPool) Len() int {
 
 // validateTxBasics runs static validation on a transaction.
 func (p *TxPool) validateTxBasics(trx *tx.Transaction) error {
+	if err := trx.EnforceSignatureLowS(); err != nil {
+		return badTxError{err.Error()}
+	}
+
 	if trx.ChainTag() != p.repo.ChainTag() {
 		return badTxError{"chain tag mismatch"}
 	}