Use EIP155 for all signers with empty transaction `chain_id` (#1198)

* remove error when signing with a different chain

 - a chain_id mismatch between the signer and the transaction is valid
   since the behavior is the same between two signers with different
   chain ids
 - a specified chain_id should be signed regardless of the chain_id of
   the signer
 - refactor `sign_hash` to no longer take an `eip155` flag - it now
   _just_ signs hashes. EIP155 is specific to transactions, so we
   now normalize the `v` value in `sign_transaction_sync`

* use signer chain_id for tx in trezor signer

 - use the trezor signer's chain_id if the transaction's chain_id
   doesn't exist
 - sets the chain_id in both `sign_tx` and the Signer implementation's
   `sign_transaction`

* use signer chain_id for tx in ledger signer

 - use the ledger signer's chain_id if the transaction's chain_id
   doesn't exist
 - sets the chain_id in both `sign_tx` and the Signer implementation's
   `sign_transaction`

* prefer transaction chain_id in aws signer

 - uses the signer chain_id if the transaction chain_id doesn't exist
 - refactor `sign_digest_with_eip155` to take an input chain_id, so the
   signer chain_id is not used every time. If we want to enforce
   transaction and signer chain ids to be consistent, this should be
   undone

* add private key signing test for an empty chain_id

* Apply suggestions from code review

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

* Update ethers-signers/src/ledger/mod.rs

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

ethers-signers/src/aws/mod.rs

@@ -203,15 +203,19 @@ impl<'a> AwsSigner<'a> {
     }
 
     /// Sign a digest with this signer's key and add the eip155 `v` value
-    /// corresponding to this signer's chain_id
+    /// corresponding to the input chain_id
     #[instrument(err, skip(digest), fields(digest = %hex::encode(&digest)))]
-    async fn sign_digest_with_eip155(&self, digest: H256) -> Result<EthSig, AwsSignerError> {
+    async fn sign_digest_with_eip155(
+        &self,
+        digest: H256,
+        chain_id: u64,
+    ) -> Result<EthSig, AwsSignerError> {
         let sig = self.sign_digest(digest.into()).await?;
 
         let sig = utils::rsig_from_digest_bytes_trial_recovery(&sig, digest.into(), &self.pubkey);
 
         let mut sig = rsig_to_ethsig(&sig);
-        apply_eip155(&mut sig, self.chain_id);
+        apply_eip155(&mut sig, chain_id);
         Ok(sig)
     }
 }
@@ -230,13 +234,17 @@ impl<'a> super::Signer for AwsSigner<'a> {
         trace!("{:?}", message_hash);
         trace!("{:?}", message);
 
-        self.sign_digest_with_eip155(message_hash).await
+        self.sign_digest_with_eip155(message_hash, self.chain_id).await
     }
 
     #[instrument(err)]
     async fn sign_transaction(&self, tx: &TypedTransaction) -> Result<EthSig, Self::Error> {
+        let mut tx_with_chain = tx.clone();
+        let chain_id = tx_with_chain.chain_id().map(|id| id.as_u64()).unwrap_or(self.chain_id);
+        tx_with_chain.set_chain_id(chain_id);
+
         let sighash = tx.sighash();
-        self.sign_digest_with_eip155(sighash).await
+        self.sign_digest_with_eip155(sighash, chain_id).await
     }
 
     async fn sign_typed_data<T: Eip712 + Send + Sync>(
@@ -245,7 +253,7 @@ impl<'a> super::Signer for AwsSigner<'a> {
     ) -> Result<EthSig, Self::Error> {
         let hash = payload.encode_eip712().map_err(|e| Self::Error::Eip712Error(e.to_string()))?;
 
-        let digest = self.sign_digest_with_eip155(hash.into()).await?;
+        let digest = self.sign_digest_with_eip155(hash.into(), self.chain_id).await?;
 
         Ok(digest)
     }

ethers-signers/src/ledger/app.rs

@@ -117,8 +117,13 @@ impl LedgerEthereum {
 
     /// Signs an Ethereum transaction (requires confirmation on the ledger)
     pub async fn sign_tx(&self, tx: &TypedTransaction) -> Result<Signature, LedgerError> {
+        let mut tx_with_chain = tx.clone();
+        if tx_with_chain.chain_id().is_none() {
+            // in the case we don't have a chain_id, let's use the signer chain id instead
+            tx_with_chain.set_chain_id(self.chain_id);
+        }
         let mut payload = Self::path_to_bytes(&self.derivation);
-        payload.extend_from_slice(tx.rlp().as_ref());
+        payload.extend_from_slice(tx_with_chain.rlp().as_ref());
         self.sign_payload(INS::SIGN, payload).await
     }
 

ethers-signers/src/ledger/mod.rs

@@ -25,7 +25,12 @@ impl Signer for LedgerEthereum {
 
     /// Signs the transaction
     async fn sign_transaction(&self, message: &TypedTransaction) -> Result<Signature, Self::Error> {
-        self.sign_tx(message).await
+        let mut tx_with_chain = message.clone();
+        if tx_with_chain.chain_id().is_none() {
+            // in the case we don't have a chain_id, let's use the signer chain id instead
+            tx_with_chain.set_chain_id(self.chain_id);
+        }
+        self.sign_tx(tx_with_chain).await
     }
 
     /// Signs a EIP712 derived struct

ethers-signers/src/trezor/app.rs

@@ -160,6 +160,8 @@ impl TrezorEthereum {
 
         let transaction = TrezorTransaction::load(tx)?;
 
+        let chain_id = tx.chain_id().map(|id| id.as_u64()).unwrap_or(self.chain_id);
+
         let signature = match tx {
             TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => client.ethereum_sign_tx(
                 arr_path,
@@ -169,7 +171,7 @@ impl TrezorEthereum {
                 transaction.to,
                 transaction.value,
                 transaction.data,
-                self.chain_id,
+                chain_id,
             )?,
             TypedTransaction::Eip1559(eip1559_tx) => client.ethereum_sign_eip1559_tx(
                 arr_path,
@@ -178,7 +180,7 @@ impl TrezorEthereum {
                 transaction.to,
                 transaction.value,
                 transaction.data,
-                self.chain_id,
+                chain_id,
                 transaction.max_fee_per_gas,
                 transaction.max_priority_fee_per_gas,
                 transaction.access_list,

ethers-signers/src/trezor/mod.rs

@@ -25,7 +25,12 @@ impl Signer for TrezorEthereum {
 
     /// Signs the transaction
     async fn sign_transaction(&self, message: &TypedTransaction) -> Result<Signature, Self::Error> {
-        self.sign_tx(message).await
+        let mut tx_with_chain = message.clone();
+        if tx_with_chain.chain_id().is_none() {
+            // in the case we don't have a chain_id, let's use the signer chain id instead
+            tx_with_chain.set_chain_id(self.chain_id);
+        }
+        self.sign_tx(&tx_with_chain).await
     }
 
     /// Signs a EIP712 derived struct

ethers-signers/src/wallet/mod.rs

@@ -18,7 +18,7 @@ use ethers_core::{
     },
     types::{
         transaction::{eip2718::TypedTransaction, eip712::Eip712},
-        Address, Signature, H256, U256, U64,
+        Address, Signature, H256, U256,
     },
     utils::hash_message,
 };
@@ -79,27 +79,16 @@ impl<D: Sync + Send + DigestSigner<Sha256Proxy, RecoverableSignature>> Signer fo
         let message = message.as_ref();
         let message_hash = hash_message(message);
 
-        Ok(self.sign_hash(message_hash, false))
+        Ok(self.sign_hash(message_hash))
     }
 
     async fn sign_transaction(&self, tx: &TypedTransaction) -> Result<Signature, Self::Error> {
-        let chain_id = tx.chain_id();
+        let mut tx_with_chain = tx.clone();
-        match chain_id {
+        if tx_with_chain.chain_id() == None {
-            Some(id) => {
+            // in the case we don't have a chain_id, let's use the signer chain id instead
-                if U64::from(self.chain_id) != id {
+            tx_with_chain.set_chain_id(self.chain_id);
-                    return Err(WalletError::InvalidTransactionError(
-                        "transaction chain_id does not match the signer".to_string(),
-                    ))
-                }
-                Ok(self.sign_transaction_sync(tx))
-            }
-            None => {
-                // in the case we don't have a chain_id, let's use the signer chain id instead
-                let mut tx_with_chain = tx.clone();
-                tx_with_chain.set_chain_id(self.chain_id);
-                Ok(self.sign_transaction_sync(&tx_with_chain))
-            }
         }
+        Ok(self.sign_transaction_sync(&tx_with_chain))
     }
 
     async fn sign_typed_data<T: Eip712 + Send + Sync>(
@@ -109,7 +98,7 @@ impl<D: Sync + Send + DigestSigner<Sha256Proxy, RecoverableSignature>> Signer fo
         let encoded =
             payload.encode_eip712().map_err(|e| Self::Error::Eip712Error(e.to_string()))?;
 
-        Ok(self.sign_hash(H256::from(encoded), false))
+        Ok(self.sign_hash(H256::from(encoded)))
     }
 
     fn address(&self) -> Address {
@@ -129,23 +118,24 @@ impl<D: Sync + Send + DigestSigner<Sha256Proxy, RecoverableSignature>> Signer fo
 }
 
 impl<D: DigestSigner<Sha256Proxy, RecoverableSignature>> Wallet<D> {
-    /// Synchronously signs the provided transaction.
+    /// Synchronously signs the provided transaction, normalizing the signature `v` value with
+    /// EIP-155 using the transaction's `chain_id`.
     pub fn sign_transaction_sync(&self, tx: &TypedTransaction) -> Signature {
         let sighash = tx.sighash();
-        self.sign_hash(sighash, true)
+        let chain_id = tx.chain_id().map(|id| id.as_u64()).unwrap_or(self.chain_id);
+        let mut sig = self.sign_hash(sighash);
+
+        // sign_hash sets `v` to recid + 27, so we need to subtract 27 before normalizing
+        sig.v = to_eip155_v(sig.v as u8 - 27, chain_id);
+        sig
     }
 
-    /// Signs the provided hash and proceeds to normalize the `v` value of the
+    /// Signs the provided hash.
-    /// signature with EIP-155 if the flag is set to true.
+    pub fn sign_hash(&self, hash: H256) -> Signature {
-    pub fn sign_hash(&self, hash: H256, eip155: bool) -> Signature {
         let recoverable_sig: RecoverableSignature =
             self.signer.sign_digest(Sha256Proxy::from(hash));
 
-        let v = if eip155 {
+        let v = u8::from(recoverable_sig.recovery_id()) as u64 + 27;
-            to_eip155_v(recoverable_sig.recovery_id(), self.chain_id)
-        } else {
-            u8::from(recoverable_sig.recovery_id()) as u64 + 27
-        };
 
         let r_bytes: FieldBytes<Secp256k1> = recoverable_sig.r().into();
         let s_bytes: FieldBytes<Secp256k1> = recoverable_sig.s().into();

ethers-signers/src/wallet/private_key.rs

@@ -46,8 +46,6 @@ pub enum WalletError {
     /// Error type from Eip712Error message
     #[error("error encoding eip712 struct: {0:?}")]
     Eip712Error(String),
-    #[error("invalid transaction: {0:?}")]
-    InvalidTransactionError(String),
 }
 
 impl Clone for Wallet<SigningKey> {
@@ -221,6 +219,39 @@ mod tests {
         assert!(sig.verify(sighash, wallet.address).is_ok());
     }
 
+    #[tokio::test]
+    #[cfg(not(feature = "celo"))]
+    async fn signs_tx_empty_chain_id() {
+        use crate::TypedTransaction;
+        use ethers_core::types::TransactionRequest;
+        // retrieved test vector from:
+        // https://web3js.readthedocs.io/en/v1.2.0/web3-eth-accounts.html#eth-accounts-signtransaction
+        let tx: TypedTransaction = TransactionRequest {
+            from: None,
+            to: Some("F0109fC8DF283027b6285cc889F5aA624EaC1F55".parse::<Address>().unwrap().into()),
+            value: Some(1_000_000_000.into()),
+            gas: Some(2_000_000.into()),
+            nonce: Some(0.into()),
+            gas_price: Some(21_000_000_000u128.into()),
+            data: None,
+            chain_id: None,
+        }
+        .into();
+        let wallet: Wallet<SigningKey> =
+            "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318".parse().unwrap();
+        let wallet = wallet.with_chain_id(1u64);
+
+        // this should populate the tx chain_id as the signer's chain_id (1) before signing
+        let sig = wallet.sign_transaction(&tx).await.unwrap();
+
+        // since we initialize with None we need to re-set the chain_id for the sighash to be
+        // correct
+        let mut tx = tx;
+        tx.set_chain_id(1);
+        let sighash = tx.sighash();
+        assert!(sig.verify(sighash, wallet.address).is_ok());
+    }
+
     #[test]
     fn key_to_address() {
         let wallet: Wallet<SigningKey> =