Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions crates/common/types/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1891,23 +1891,27 @@ mod serde_impl {
chain_id: value.chain_id,
address: value.address,
nonce: value.nonce,
y_parity: value.y_parity,
y_parity: U256::from(value.y_parity),
r: value.r_signature,
s: value.s_signature,
}
}
}

impl From<AuthorizationTupleEntry> for AuthorizationTuple {
fn from(entry: AuthorizationTupleEntry) -> AuthorizationTuple {
AuthorizationTuple {
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,
Comment on lines +1901 to 1912
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!

s_signature: entry.s,
}
})
}
}

Expand Down Expand Up @@ -2448,8 +2452,9 @@ mod serde_impl {
"authorizationList",
)?
.into_iter()
.map(AuthorizationTuple::from)
.collect::<Vec<_>>(),
.map(AuthorizationTuple::try_from)
.collect::<Result<Vec<_>, _>>()
.map_err(serde::de::Error::custom)?,
signature_y_parity: u8::from_str_radix(
deserialize_field::<String, D>(&mut map, "yParity")?.trim_start_matches("0x"),
16,
Expand Down Expand Up @@ -2833,8 +2838,9 @@ mod serde_impl {
.authorization_list
.unwrap_or_default()
.into_iter()
.map(AuthorizationTuple::from)
.collect(),
.map(AuthorizationTuple::try_from)
.collect::<Result<Vec<_>, _>>()
.map_err(GenericTransactionError::InvalidField)?,
..Default::default()
})
}
Expand Down Expand Up @@ -3546,7 +3552,7 @@ mod tests {
chain_id: U256::from(65536999),
address: H160::from_str("0x000a52D537c4150ec274dcE3962a0d179B7E71B1").unwrap(),
nonce: 2,
y_parity: U256::one(),
y_parity: 1,
r_signature: U256::from(22),
s_signature: U256::from(37),
}],
Expand Down
38 changes: 36 additions & 2 deletions crates/common/types/tx_fields.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ pub struct AuthorizationTuple {
deserialize_with = "crate::serde_utils::u64::deser_hex_or_dec_str"
)]
pub nonce: u64,
#[rkyv(with = crate::rkyv_utils::U256Wrapper)]
pub y_parity: U256,
// EIP-7702 bounds y_parity to < 2**8; a u8 makes that a type invariant and lets
// RLP decoding reject any out-of-range value, matching geth's `uint8`.
pub y_parity: u8,
#[serde(rename = "r")]
#[rkyv(with = crate::rkyv_utils::U256Wrapper)]
pub r_signature: U256,
Expand Down Expand Up @@ -87,3 +88,36 @@ impl RLPDecode for AuthorizationTuple {
))
}
}

#[cfg(test)]
mod eip7702_y_parity_tests {
//! Regression test for the `eip7702-auth-y-parity-bound` finding. EIP-7702 bounds
//! an authorization tuple's `y_parity` to `< 2**8`; geth models it as a `u8` and
//! rejects an out-of-range value at RLP decode. ethrex must do the same, otherwise
//! an L1 block carrying a type-4 transaction whose `y_parity >= 256` is accepted
//! here but rejected by other clients (consensus split). The authorization-tuple
//! decode is the chokepoint: a type-4 transaction decodes its `authorization_list`
//! through it.
use super::*;

#[test]
fn authorization_tuple_rejects_y_parity_at_or_above_256() {
// Hand-build the RLP of an authorization tuple whose `y_parity` field encodes
// 256 (two bytes, 0x01 0x00), independent of the field's Rust type.
let mut buf = Vec::new();
Encoder::new(&mut buf)
.encode_field(&U256::zero()) // chain_id
.encode_field(&Address::zero()) // address
.encode_field(&0u64) // nonce
.encode_field(&U256::from(256u64)) // y_parity = 256, out of the < 2**8 bound
.encode_field(&U256::one()) // r_signature
.encode_field(&U256::one()) // s_signature
.finish();

let result = AuthorizationTuple::decode(&buf);
assert!(
result.is_err(),
"authorization tuple with y_parity >= 2**8 must be rejected at decode, got: {result:?}"
);
}
}
5 changes: 3 additions & 2 deletions crates/vm/backends/levm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2791,8 +2791,9 @@ fn vm_from_generic<'a>(
.collect(),
authorization_list: authorization_list
.iter()
.map(|auth| Into::<AuthorizationTuple>::into(auth.clone()))
.collect(),
.map(|auth| AuthorizationTuple::try_from(auth.clone()))
.collect::<Result<Vec<_>, _>>()
.map_err(|_| InternalError::TypeConversion)?,
..Default::default()
}),
None => Transaction::EIP1559Transaction(EIP1559Transaction {
Expand Down
5 changes: 2 additions & 3 deletions crates/vm/levm/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ pub fn eip7702_recover_address(
if auth_tuple.r_signature > *SECP256K1_ORDER || U256::zero() >= auth_tuple.r_signature {
return Ok(None);
}
if auth_tuple.y_parity != U256::one() && auth_tuple.y_parity != U256::zero() {
if auth_tuple.y_parity != 1 && auth_tuple.y_parity != 0 {
return Ok(None);
}

Expand All @@ -224,8 +224,7 @@ pub fn eip7702_recover_address(
(auth_tuple.chain_id, auth_tuple.address, auth_tuple.nonce).encode(&mut rlp_buf);
let msg = crypto.keccak256(&rlp_buf);

let y_parity: u8 =
TryInto::<u8>::try_into(auth_tuple.y_parity).map_err(|_| InternalError::TypeConversion)?;
let y_parity: u8 = auth_tuple.y_parity;

let mut sig = [0u8; 65];
sig[..32].copy_from_slice(&auth_tuple.r_signature.to_big_endian());
Expand Down
Loading