fix(l1): bound EIP-7702 authorization y_parity to a single byte#6753
fix(l1): bound EIP-7702 authorization y_parity to a single byte#6753ElFantasma wants to merge 1 commit into
Conversation
|
🤖 Kimi Code ReviewThis PR correctly fixes a consensus-critical EIP-7702 compliance issue by enforcing the Summary of changes:
Review feedback: crates/common/types/tx_fields.rs:44-47 crates/common/types/transaction.rs:1907-1910 crates/common/types/transaction.rs:2450-2454 crates/vm/levm/src/utils.rs:218 crates/vm/levm/src/utils.rs:227 crates/common/types/tx_fields.rs:88-121 (test) Minor observations:
Verdict: Approving. This is a clean, security-focused fix that prevents a consensus split by enforcing EIP-7702 constraints at the type system and deserialization boundaries. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
Aside from that, the change itself looks correct and consensus-hardening: decoding I couldn’t run the full test suite in this environment because the sandbox blocks Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR fixes a consensus split risk in EIP-7702 (type-4) transaction processing:
Confidence Score: 4/5Safe to merge; the change is well-scoped, closes a real consensus-split bug, and all affected call sites have been updated consistently. The structural change is correct and the regression test directly exercises the chokepoint. The only remaining rough edge is the TryFrom::Error = String on the JSON-RPC conversion path, which causes callers to discard the diagnostic message — a style concern that does not affect correctness. No files require special attention; transaction.rs has the minor untyped-error pattern noted above, but it does not affect the correctness of the fix.
|
| Filename | Overview |
|---|---|
| crates/common/types/tx_fields.rs | Core structural fix: y_parity narrowed from U256 to u8; RLP decode now rejects >= 256 automatically. Regression test correctly verifies the rejection. |
| crates/common/types/transaction.rs | From upgraded to TryFrom with String error type; all three call sites (serde deserialization, GenericTransaction conversion, vm_from_generic) correctly propagate the failure. The untyped String error is a minor style point. |
| crates/vm/backends/levm/mod.rs | vm_from_generic now collects authorization list with try_from; TypeConversion error correctly surfaced if an out-of-range entry slips through earlier validation. |
| crates/vm/levm/src/utils.rs | eip7702_recover_address simplified: infallible u8 assignment replaces the former TryInto. The 2..=255 → Ok(None) guard is unchanged and still correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming type-4 tx bytes] --> B[RLPDecode AuthorizationTuple]
B -->|y_parity gt 255| C[RLPDecodeError - rejects block]
B -->|y_parity 0-255| D[AuthorizationTuple - y_parity as u8]
D --> E[eip7702_recover_address]
E -->|y_parity 2-255| F[Ok None - tuple skipped, tx valid]
E -->|y_parity 0 or 1| G[ECDSA recovery - Ok Some address]
H[JSON-RPC eth_sendTransaction] --> I[Deserialize GenericTransaction - AuthorizationTupleEntry y_parity as U256]
I --> J[TryFrom into AuthorizationTuple]
J -->|y_parity gt 255| K[Error propagated to caller]
J -->|y_parity 0-255| D
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
crates/common/types/transaction.rs:1901-1912
**Untyped `String` error makes the `TryFrom` contract opaque**
`TryFrom<AuthorizationTupleEntry>` declares `type Error = String`, which callers must either discard (`map_err(|_| …)`) or forward as a raw string. `vm_from_generic` already discards the message with `|_|`, so the descriptive string is lost at the only call site that propagates an `InternalError`. Consider promoting this to a dedicated variant (e.g., `GenericTransactionError::InvalidYParity`) or at least a `Box<dyn std::error::Error>`, so callers can preserve structure without losing the diagnostic.
Reviews (1): Last reviewed commit: "fix(l1): bound EIP-7702 authorization y_..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl TryFrom<AuthorizationTupleEntry> for AuthorizationTuple { | ||
| type Error = String; | ||
|
|
||
| // EIP-7702 bounds y_parity to < 2**8; reject anything that doesn't fit a u8. | ||
| fn try_from(entry: AuthorizationTupleEntry) -> Result<AuthorizationTuple, Self::Error> { | ||
| Ok(AuthorizationTuple { | ||
| chain_id: entry.chain_id, | ||
| address: entry.address, | ||
| nonce: entry.nonce, | ||
| y_parity: entry.y_parity, | ||
| y_parity: TryInto::<u8>::try_into(entry.y_parity) | ||
| .map_err(|_| "authorization tuple y_parity exceeds 2**8".to_string())?, | ||
| r_signature: entry.r, |
There was a problem hiding this comment.
String error makes the TryFrom contract opaque
TryFrom<AuthorizationTupleEntry> declares type Error = String, which callers must either discard (map_err(|_| …)) or forward as a raw string. vm_from_generic already discards the message with |_|, so the descriptive string is lost at the only call site that propagates an InternalError. Consider promoting this to a dedicated variant (e.g., GenericTransactionError::InvalidYParity) or at least a Box<dyn std::error::Error>, so callers can preserve structure without losing the diagnostic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/types/transaction.rs
Line: 1901-1912
Comment:
**Untyped `String` error makes the `TryFrom` contract opaque**
`TryFrom<AuthorizationTupleEntry>` declares `type Error = String`, which callers must either discard (`map_err(|_| …)`) or forward as a raw string. `vm_from_generic` already discards the message with `|_|`, so the descriptive string is lost at the only call site that propagates an `InternalError`. Consider promoting this to a dedicated variant (e.g., `GenericTransactionError::InvalidYParity`) or at least a `Box<dyn std::error::Error>`, so callers can preserve structure without losing the diagnostic.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewNow I have enough context to write the full review. Code Review: PR #6753 —
|
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
|
Ran the EIP-7702 Hive EELS suite against this branch (image built from the branch):
This is the RLP block-import path that this fix touches. Notable coverage in the run:
Note: v5.3.0 doesn't appear to ship a fixture named specifically for Also running |
|
Follow-up: |
1 participant
Summary
EIP-7702 bounds an authorization tuple's
y_parityto< 2**8(it must fit in a single byte); geth models it as auint8and rejects an out-of-range value at RLP decode, which makes the whole transaction — and any block containing it — invalid.ethrex typed
y_parityasU256and decoded it with no bound, so a value like256decoded successfully. At executioneip7702_recover_addressreturnsOk(None)for any non-0/1y_parityand the authorization tuple is silently skipped (the transaction stays valid). The result: a type-4 transaction withy_parity >= 2**8is accepted and executed by ethrex but rejected at decode by other clients → an L1 block carrying it splits ethrex from the network.Note the bound is purely a field-size rule:
y_parityvalues in2..=255are in range, fail signature recovery, and are skipped (tuple ignored, tx valid) — matching geth. Only>= 2**8diverges, and only that is changed here.Fix
Type the field as
u8(crates/common/types/tx_fields.rs), making the< 2**8bound a type invariant:u8now rejects>= 256automatically, matching geth'suint8.From<AuthorizationTupleEntry> for AuthorizationTuplebecomesTryFrom, so the JSON-RPC input path (eth_sendTransaction) also rejects out-of-rangey_parityinstead of silently truncating. Callers (the tx deserializer,GenericTransactionconversion, andvm_from_generic) thread the fallible conversion.eip7702_recover_addresssimplifies to au8comparison and drops a now-redundanttry_into.The
2..=255→skip behavior is unchanged.Test
Adds
authorization_tuple_rejects_y_parity_at_or_above_256, which hand-builds the RLP of an authorization tuple withy_parity = 256(type-agnostic) and asserts decode rejects it — the chokepoint a type-4 transaction'sauthorization_listdecodes through. Confirmed red on the pre-fix code (decodedy_parity: 256) and green after.