Skip to content

fix: reject non-canonical leading-zero integer tx fields - #44

Open
spokodev wants to merge 1 commit into
paulmillr:mainfrom
spokodev:fix/tx-non-canonical-leading-zero
Open

fix: reject non-canonical leading-zero integer tx fields#44
spokodev wants to merge 1 commit into
paulmillr:mainfrom
spokodev:fix/tx-non-canonical-leading-zero

Conversation

@spokodev

Copy link
Copy Markdown

Problem

Ethereum scalar fields (nonce, gasPrice, value, gasLimit, chainId, the EIP-1559/4844 fee fields, r and s) are RLP byte strings with no leading zeros: a zero value is the empty string, and any other value's first byte is non-zero.

The transaction decoder (U64BE / U256BE in src/core/tx-internal.ts) folds the raw field bytes into a bigint and silently ignores leading zeros. So a leading-zero-padded field decodes to the same logical transaction but re-serializes to different bytes, changing the transaction hash. A signer, indexer or mempool built on this library then computes a hash that no canonical client agrees with, and accepts transactions that mainnet rejects.

Evidence

The official Ethereum TransactionTests (already vendored under test/vectors) declare a mandatory exception for these encodings on every fork:

  • ttGasPrice/TransactionWithLeadingZerosGasPrice -> LeadingZerosGasPrice
  • ttEIP1559/maxFeePerGas00prefix -> 1559LeadingZerosBaseFee
  • ttNonce/TransactionWithLeadingZerosNonce -> LeadingZerosNonce
  • ttRSValue/TransactionWithRvaluePrefixed00BigInt -> LeadingZerosR

The suite was skipping exactly these vectors through SKIPPED_ERRORS.ethereum_tests.

Fix

Wrap the variable-width scalar coders so they reject a leading zero byte, and un-skip the six vectors. yParity (fixed width) and byte-string fields (to, data, access-list keys, blob hashes) are left untouched, since those may legitimately start with 0x00. The empty-string zero (0x80) still decodes to 0n.

Tests

  • Red baseline: with the vectors un-skipped and the source reverted, the suite fails with Missing expected exception: throws(ttGasPrice/TransactionWithLeadingZerosGasPrice).
  • With the fix: full suite green (429 tests), all official leading-zero vectors rejected, canonical transactions round-trip byte-identical, sender recovery unchanged.

Ethereum scalar fields (nonce, gasPrice, value, gasLimit, chainId, the
EIP-1559/4844 fee fields, r and s) are RLP byte strings without leading
zeros: a zero value is the empty string and any other value's first byte
is non-zero. The transaction decoder folded the raw bytes into a bigint
and silently ignored leading zeros, so a leading-zero-padded field
decoded to the same logical transaction but re-serialized to different
bytes, changing its hash.

The official Ethereum TransactionTests reject these encodings
(ttGasPrice/TransactionWithLeadingZerosGasPrice,
ttEIP1559/maxFeePerGas00prefix, ttNonce/TransactionWithLeadingZerosNonce,
ttRSValue/TransactionWithRvaluePrefixed00BigInt and more), and the test
suite was skipping exactly those vectors. Reject leading-zero scalar
fields on decode and un-skip the vectors.
@paulmillr

Copy link
Copy Markdown
Owner

A signer, indexer or mempool built on this library then computes a hash that no canonical client agrees with, and accepts transactions that mainnet rejects

Is that purely theoretic or do you have a real example of that?

@spokodev

Copy link
Copy Markdown
Author

Honestly, I do not have a real-world exploit to point to. This is spec conformance: Ethereum RLP requires canonical (no leading zero) integer encoding, and the official TransactionTests mark these inputs invalid (those are the vectors I un-skipped). geth and ethers reject them, while before this change micro-eth-signer accepted and round-tripped them. So the divergence is concrete (a value mainnet rejects parses cleanly here), but I am not aware of an active attack relying on it. If you consider that not worth guarding, that is a fair call and I am fine to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants