diff --git a/crates/primitives/src/lib.rs b/crates/primitives/src/lib.rs index 2f40f2a197..ca6a5c1d4f 100644 --- a/crates/primitives/src/lib.rs +++ b/crates/primitives/src/lib.rs @@ -95,7 +95,7 @@ pub use transaction::{ }; pub use transaction::{ - util::secp256k1::{public_key_to_address, recover_signer, sign_message}, + util::secp256k1::{public_key_to_address, recover_signer_unchecked, sign_message}, AccessList, AccessListItem, FromRecoveredTransaction, IntoRecoveredTransaction, InvalidTransactionError, Signature, Transaction, TransactionKind, TransactionMeta, TransactionSigned, TransactionSignedEcRecovered, TransactionSignedNoHash, TxEip1559, TxEip2930, diff --git a/crates/primitives/src/revm/env.rs b/crates/primitives/src/revm/env.rs index 634cc6db1c..3d890bc71e 100644 --- a/crates/primitives/src/revm/env.rs +++ b/crates/primitives/src/revm/env.rs @@ -1,6 +1,6 @@ use crate::{ constants::{BEACON_ROOTS_ADDRESS, SYSTEM_ADDRESS}, - recover_signer, + recover_signer_unchecked, revm::config::revm_spec, revm_primitives::{AnalysisKind, BlockEnv, CfgEnv, Env, SpecId, TransactTo, TxEnv}, Address, Bytes, Chain, ChainSpec, Head, Header, Transaction, TransactionKind, @@ -129,7 +129,10 @@ pub fn recover_header_signer(header: &Header) -> Result) -> u64 { #[cfg(feature = "optimism")] if self.r.is_zero() && self.s.is_zero() { - return 0 + return 0; } if let Some(chain_id) = chain_id { @@ -100,7 +107,7 @@ impl Signature { } else { // non-EIP-155 legacy scheme, v = 27 for even y-parity, v = 28 for odd y-parity if v != 27 && v != 28 { - return Err(RlpError::Custom("invalid Ethereum signature (V is not 27 or 28)")) + return Err(RlpError::Custom("invalid Ethereum signature (V is not 27 or 28)")); } let odd_y_parity = v == 28; Ok((Signature { r, s, odd_y_parity }, None)) @@ -128,8 +135,13 @@ impl Signature { }) } - /// Recover signer address from message hash. - pub fn recover_signer(&self, hash: B256) -> Option
{ + /// Recover signer from message hash, _without ensuring that the signature has a low `s` + /// value_. + /// + /// Using this for signature validation will succeed, even if the signature is malleable or not + /// compliant with EIP-2. This is provided for compatibility with old signatures which have + /// large `s` values. + pub fn recover_signer_unchecked(&self, hash: B256) -> Option
{ let mut sig: [u8; 65] = [0; 65]; sig[0..32].copy_from_slice(&self.r.to_be_bytes::<32>()); @@ -138,7 +150,20 @@ impl Signature { // NOTE: we are removing error from underlying crypto library as it will restrain primitive // errors and we care only if recovery is passing or not. - secp256k1::recover_signer(&sig, &hash.0).ok() + secp256k1::recover_signer_unchecked(&sig, &hash.0).ok() + } + + /// Recover signer address from message hash. This ensures that the signature S value is + /// greater than `secp256k1n / 2`, as specified in + /// [EIP-2](https://eips.ethereum.org/EIPS/eip-2). + /// + /// If the S value is too large, then this will return `None` + pub fn recover_signer(&self, hash: B256) -> Option
{ + if self.s > SECP256K1N_HALF { + return None; + } + + self.recover_signer_unchecked(hash) } /// Turn this signature into its byte @@ -166,7 +191,8 @@ impl Signature { #[cfg(test)] mod tests { - use crate::{Address, Signature, B256, U256}; + use crate::{transaction::signature::SECP256K1N_HALF, Address, Signature, B256, U256}; + use alloy_primitives::hex; use bytes::BytesMut; use std::str::FromStr; @@ -286,4 +312,26 @@ mod tests { assert!(signature.size() >= 65); } + + #[test] + fn eip_2_reject_high_s_value() { + // This pre-homestead transaction has a high `s` value and should be rejected by the + // `recover_signer` method: + // https://etherscan.io/getRawTx?tx=0x9e6e19637bb625a8ff3d052b7c2fe57dc78c55a15d258d77c43d5a9c160b0384 + // + // Block number: 46170 + let raw_tx = hex!("f86d8085746a52880082520894c93f2250589a6563f5359051c1ea25746549f0d889208686e75e903bc000801ba034b6fdc33ea520e8123cf5ac4a9ff476f639cab68980cd9366ccae7aef437ea0a0e517caa5f50e27ca0d1e9a92c503b4ccb039680c6d9d0c71203ed611ea4feb33"); + let tx = crate::transaction::TransactionSigned::decode_enveloped(&mut &raw_tx[..]).unwrap(); + let signature = tx.signature(); + + // make sure we know it's greater than SECP256K1N_HALF + assert!(signature.s > SECP256K1N_HALF); + + // recover signer, expect failure + let hash = tx.hash(); + assert!(signature.recover_signer(hash).is_none()); + + // use unchecked, ensure it succeeds (the signature is valid if not for EIP-2) + assert!(signature.recover_signer_unchecked(hash).is_some()); + } } diff --git a/crates/primitives/src/transaction/util.rs b/crates/primitives/src/transaction/util.rs index ce180e2dd3..638064c12f 100644 --- a/crates/primitives/src/transaction/util.rs +++ b/crates/primitives/src/transaction/util.rs @@ -11,7 +11,10 @@ pub(crate) mod secp256k1 { /// Recovers the address of the sender using secp256k1 pubkey recovery. /// /// Converts the public key into an ethereum address by hashing the public key with keccak256. - pub fn recover_signer(sig: &[u8; 65], msg: &[u8; 32]) -> Result { + /// + /// This does not ensure that the `s` value in the signature is low, and _just_ wraps the + /// underlying secp256k1 library. + pub fn recover_signer_unchecked(sig: &[u8; 65], msg: &[u8; 32]) -> Result { let sig = RecoverableSignature::from_compact(&sig[0..64], RecoveryId::from_i32(sig[64] as i32)?)?; @@ -55,6 +58,6 @@ mod tests { let hash = hex!("47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad"); let out = address!("c08b5542d177ac6686946920409741463a15dddb"); - assert_eq!(secp256k1::recover_signer(&sig, &hash), Ok(out)); + assert_eq!(secp256k1::recover_signer_unchecked(&sig, &hash), Ok(out)); } } diff --git a/crates/stages/src/stages/sender_recovery.rs b/crates/stages/src/stages/sender_recovery.rs index a7b19ca57a..a758b9b6bc 100644 --- a/crates/stages/src/stages/sender_recovery.rs +++ b/crates/stages/src/stages/sender_recovery.rs @@ -198,9 +198,14 @@ fn recover_sender( let tx = transaction.value().expect("value to be formated"); tx.transaction.encode_without_signature(rlp_buf); + // We call [Signature::recover_signer_unchecked] because transactions run in the pipeline are + // known to be valid - this means that we do not need to check whether or not the `s` value is + // greater than `secp256k1n / 2` if past EIP-2. There are transactions pre-homestead which have + // large `s` values, so using [Signature::recover_signer] here would not be + // backwards-compatible. let sender = tx .signature - .recover_signer(keccak256(rlp_buf)) + .recover_signer_unchecked(keccak256(rlp_buf)) .ok_or(SenderRecoveryStageError::FailedRecovery(FailedSenderRecoveryError { tx: tx_id }))?; Ok((tx_id, sender))