fix: add recover_signer that checks according to EIP-2 (#5618)

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,

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<Address, CliqueSignerRec
         header_to_seal.extra_data = Bytes::from(header.extra_data[..signature_start_byte].to_vec());
         header_to_seal.hash_slow()
     };
-    recover_signer(&signature, &seal_hash.0).map_err(CliqueSignerRecoveryError::InvalidSignature)
+
+    // TODO: this is currently unchecked recovery, does this need to be checked w.r.t EIP-2?
+    recover_signer_unchecked(&signature, &seal_hash.0)
+        .map_err(CliqueSignerRecoveryError::InvalidSignature)
 }
 
 /// Returns a new [TxEnv] filled with the transaction's data.

crates/primitives/src/transaction/signature.rs

@@ -5,6 +5,13 @@ use bytes::Buf;
 use reth_codecs::{derive_arbitrary, Compact};
 use serde::{Deserialize, Serialize};
 
+/// The order of the secp256k1 curve, divided by two. Signatures that should be checked according
+/// to EIP-2 should have an S value less than or equal to this.
+const SECP256K1N_HALF: U256 = U256::from_be_bytes([
+    0x7F, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+    0x5D, 0x57, 0x6E, 0x73, 0x57, 0xA4, 0x50, 0x1D, 0xDF, 0xE9, 0x2F, 0x46, 0x68, 0x1B, 0x20, 0xA0,
+]);
+
 /// r, s: Values corresponding to the signature of the
 /// transaction and used to determine the sender of
 /// the transaction; formally Tr and Ts. This is expanded in Appendix F of yellow paper.
@@ -73,7 +80,7 @@ impl Signature {
     pub fn v(&self, chain_id: Option<u64>) -> 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<Address> {
+    /// 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<Address> {
         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<Address> {
+        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());
+    }
 }

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<Address, Error> {
+    ///
+    /// 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<Address, Error> {
         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));
     }
 }

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))