erigontech / erigontech/erigon

execution/types: bare rlp.EOL sentinel lets decoders accept non-canonical encodings (BAL account changes)

Open
#23,038 2 comments 0 reactions 1 assignee Claimed by @yperbasis View on GitHub
Glamsterdam tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

Follow-up to #23000, which fixed one instance of a general pattern in our RLP decoders: a bare `rlp.EOL` is used to mean two different things — "the enclosing list ended cleanly" and "a field is absent" — so a truncated payload can be mistaken for a legitimate end.

#23000 fixed the transaction-list instance by driving iteration from the outer list with `MoreDataInList()` and propagating every decode error. The same pattern remains at other sites, and at least one of them accepts non-canonical input today.

## Confirmed: BAL account changes accept non-canonical encodings

EIP-7928 requires all six `AccountChanges` fields. Each field decoder in `execution/types/block_access_list.go` (`decodeSlotChangesList`, `decodeStorageKeys`, `decodeBalanceChanges`, `decodeNonceChanges`, `decodeCodeChanges`, `decodeStorageChanges`) opens with:

```go
if _, err = s.List(); err != nil {
if errors.Is(err, rlp.EOL) {
return nil, nil
}
return nil, err
}
```

When the element's payload is already exhausted, `s.List()` returns a bare `rlp.EOL` and the field silently becomes `nil`. That makes every field effectively optional, so an encoding with trailing empty fields stripped decodes to the **identical** structure and therefore the identical `Hash()`.

The stripped form is not an exotic input: "no code changes" is the common case for an account that only bumped its nonce, so a valid block's BAL can be re-encoded shorter and still decode.

Repro — fails on `main`:

```go
// EIP-7928 requires all six AccountChanges fields. Omitting trailing empty ones
// must not decode.
func TestBALRejectsNonCanonicalTrailingFields(t *testing.T) {
canonical := BlockAccessList{&AccountChanges{
Address: accounts.InternAddress(common.HexToAddress("0xaa")),
NonceChanges: []*NonceChange{{Index: 1, Value: 7}},
}}
canonBytes, err := EncodeBlockAccessListBytes(canonical)
require.NoError(t, err)
require.Equal(t, "dedd9400000000000000000000000000000000000000aac0c0c0c3c20107c0", common.Bytes2Hex(canonBytes))

// Same list with the trailing empty code_changes (0xc0) removed and both
// single-byte list prefixes shrunk to match.
truncated := common.Hex2Bytes("dddc9400000000000000000000000000000000000000aac0c0c0c3c20107")

got, err := DecodeBlockAccessListBytes(truncated)
require.Error(t, err, "non-canonical BAL must be rejected, decoded %d accounts hashing to %x", len(got), got.Hash())
}
```

It decodes without error and re-encodes to the canonical hash `7cd720948b17d5058357c4cb79efdb18569227d09db8ca0af8ee9ff859c33b2f`. Stripping several trailing empty lists at once, and doing the same one level deeper inside `SlotChanges`, behave the same way.

### Impact: latent, not a live consensus split

Worth being precise here, since the decoder-level laxness looks worse than it currently is. `execution/bal/process.go` compares the header's `BlockAccessListHash` — which `engine_server.go` sets to `crypto.Keccak256Hash(bal)` over the **raw** bytes — against `computedBlockBal.Hash()`, which is keccak of the **canonically re-encoded** structure. That comparison always runs while EIP-7928 is active, even when the stored sidecar is absent, so a non-canonically encoded BAL fails it and the block is rejected.

In other words canonicality is enforced incidentally, by a hash comparison whose two sides happen to be derived differently, rather than by the decoder. Two consequences:

- The protection is one refactor away from being lost. Anything that starts hashing the decoded-then-re-encoded form on both sides — a natural-looking cleanup — silently makes the lax decoder consensus-relevant.
- `newPayload` misclassifies these payloads. The decode in `engine_server.go` is explicitly there to reject malformed EIP-7928 input up front, and it accepts them; the block is then rejected later during execution as a confusing BAL hash mismatch rather than as malformed input.

## Sites audited as safe

Not every `checkErrListEnd` caller is affected, so this does not need a blanket rewrite:

- **Uncles** (`decodeUncles`) — `Header.DecodeRLP` wraps every field error, so a truncated header cannot surface a bare `EOL`.
- **Withdrawals** (`decodeWithdrawals`) — `Withdrawal.DecodeRLP` wraps every field error. The list-level EOL-as-absent guard here is legitimate: the withdrawals field really is optional pre-Shanghai.
- **`AccessListTx`** (0x01) — wraps every field error, which is why #23000's regression table correctly omits it; I confirmed it already rejected the truncated payload before that fix.

## Proposed work

- [ ] Make the six `AccountChanges` fields mandatory — distinguish "field absent" from "element payload exhausted" instead of collapsing both to `nil`.
- [ ] Convert the remaining `checkErrListEnd` loops (7 in `block_access_list.go`, plus `decodeUncles` and `decodeWithdrawals`) to the `MoreDataInList()` pattern from #23000 and delete `checkErrListEnd`, so no decoder's control flow depends on the sentinel. Keep the deliberate optional-field behaviour for body-level withdrawals.
- [ ] Add the repro above plus equivalents for the nested `SlotChanges` level.
- [ ] Backport #23000 to `release/3.5` and `release/3.6` — both carry the identical vulnerable `decodeTxns`.

## Note on reachability of #23000 itself

Recording this because it affects backport urgency: the transaction-list bug is peer-reachable on `main`, not only via `erigon import`. `execution/p2p/message_listener.go` still decodes inbound `NewBlockPacket` (→ `Block.DecodeRLP` → `decodeTxns`) for the Polygon/Astrid sync path, which registers observers in `polygon/sync/tip_events.go`. The #21505 removal only covered the Ethereum sentry legacy download path.

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.