erigontech / erigontech/erigon

txpool: malformed sender signature on any tx drops sibling txs in the same Transactions/PooledTransactions packet

Open
#20,811 1 comment 0 reactions 1 assignee Claimed by @yperbasis View on GitHub
Networking
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

## Summary

Same shape of bug as #20809 / [ethereum-bounty/erigon#7](https://github.com/ethereum-bounty/erigon/issues/7), but on the *sender* signature path rather than the EIP-7702 auth-tuple path — and it applies to any transaction type, not just SetCode.

A `Transactions (0x02)` or `PooledTransactions (0x0a)` devp2p packet containing one transaction with an unrecoverable sender signature (e.g. `r=0`) causes Erigon's parser to return `ErrParseTxn`. The batch loop in `pool_txn_packets.go` only swallows `ErrRejected`; on `ErrParseTxn` it aborts, dropping every sibling transaction that had already been parsed in the same packet, and `fetch.go` then kicks the peer.

A malicious peer can craft a cheap dummy tx with `r=0`, splice it into a packet alongside whatever sibling txs they want to suppress, and selectively censor those siblings while looking honest at the gossip layer.

## Code

Sender recovery error path — [`pool_txn_parser.go:436-438`](https://github.com/erigontech/erigon/blob/main/txnprovider/txpool/pool_txn_parser.go#L436-L438):

```go
addr, err := txn.Sender(*signer)
if err != nil {
return 0, fmt.Errorf("%w: recovering sender from signature: %s", ErrParseTxn, err) //nolint
}
```

Batch loops that only continue on `ErrRejected` — [`pool_txn_packets.go:181-187`](https://github.com/erigontech/erigon/blob/main/txnprovider/txpool/pool_txn_packets.go#L181-L187) and [`pool_txn_packets.go:212-218`](https://github.com/erigontech/erigon/blob/main/txnprovider/txpool/pool_txn_packets.go#L212-L218):

```go
pos, err = ctx.ParseTransaction(payload, pos, txnSlots.Txns[i], …)
if err != nil {
if errors.Is(err, ErrRejected) {
txnSlots.Resize(uint(i))
i--
continue
}
return 0, err
}
```

Geth, by contrast, defers sender recovery (typically lazy / at admission), so a malformed sender signature on one tx does not poison its packet siblings.

## Suggested fix

Mirror the EIP-7702 fix in #20809 for sender-sig recovery. Two reasonable options:

1. **Narrow**: change the wrap from `ErrParseTxn` to `ErrRejected` and return `p` (the position past the consumed tx) instead of `0`, so `ParseTransactions` / `ParsePooledTransactions66` skip the bad tx and continue with siblings. The position past the tx is already known by the time Step 8 runs. Bad tx is dropped, siblings survive, peer is not kicked.
2. **Proper**: defer sender recovery to the txpool's `validateTx` / admission stage — closer to Geth's design — so parsing is a pure structural transform that never touches signatures. More invasive.

I'd start with the narrow fix and consider (2) as a separate refactor.

## Repro sketch

The same approach as the test added in #20809 (`TestEIP7702BatchPoisoning`), but with the middle tx being a plain `DynamicFeeTransaction` whose `R` is set to zero post-signing instead of a SetCode tx with a poisoned auth tuple. With the current code `ParseTransactions` returns `ErrParseTxn` and `slots.Txns` ends up populated only up to the bad tx; with a fix all three should survive.

## Scope

Out of scope of #20809 because the bounty issue ([ethereum-bounty/erigon#7](https://github.com/ethereum-bounty/erigon/issues/7)) was scoped to EIP-7702. Filing this so the broader pattern doesn't get lost.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.