From d5de795382dc56ca1142d02d64954cd7f6086ce0 Mon Sep 17 00:00:00 2001 From: Dan Cline Date: Mon, 2 May 2022 14:51:25 -0400 Subject: [PATCH] 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 * Update ethers-signers/src/ledger/mod.rs Co-authored-by: Georgios Konstantopoulos Co-authored-by: Matthias Seitz --- ethers-signers/src/aws/mod.rs | 20 +++++++--- ethers-signers/src/ledger/app.rs | 7 +++- ethers-signers/src/ledger/mod.rs | 7 +++- ethers-signers/src/trezor/app.rs | 6 ++- ethers-signers/src/trezor/mod.rs | 7 +++- ethers-signers/src/wallet/mod.rs | 48 ++++++++++-------------- ethers-signers/src/wallet/private_key.rs | 35 ++++++++++++++++- 7 files changed, 88 insertions(+), 42 deletions(-) diff --git a/ethers-signers/src/aws/mod.rs b/ethers-signers/src/aws/mod.rs index 798cb2db..784f59d6 100644 --- a/ethers-signers/src/aws/mod.rs +++ b/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 { + async fn sign_digest_with_eip155( + &self, + digest: H256, + chain_id: u64, + ) -> Result { 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 { + 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( @@ -245,7 +253,7 @@ impl<'a> super::Signer for AwsSigner<'a> { ) -> Result { 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) } diff --git a/ethers-signers/src/ledger/app.rs b/ethers-signers/src/ledger/app.rs index 346231fc..02fa0c87 100644 --- a/ethers-signers/src/ledger/app.rs +++ b/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 { + 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 } diff --git a/ethers-signers/src/ledger/mod.rs b/ethers-signers/src/ledger/mod.rs index 9f24e13b..d233e898 100644 --- a/ethers-signers/src/ledger/mod.rs +++ b/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 { - 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 diff --git a/ethers-signers/src/trezor/app.rs b/ethers-signers/src/trezor/app.rs index d67d6b22..5994c79a 100644 --- a/ethers-signers/src/trezor/app.rs +++ b/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, diff --git a/ethers-signers/src/trezor/mod.rs b/ethers-signers/src/trezor/mod.rs index 060959b6..4bf0e5b5 100644 --- a/ethers-signers/src/trezor/mod.rs +++ b/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 { - 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 diff --git a/ethers-signers/src/wallet/mod.rs b/ethers-signers/src/wallet/mod.rs index efde9cf2..5f44ef6f 100644 --- a/ethers-signers/src/wallet/mod.rs +++ b/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> 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 { - let chain_id = tx.chain_id(); - match chain_id { - Some(id) => { - if U64::from(self.chain_id) != 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)) - } + let mut tx_with_chain = tx.clone(); + if tx_with_chain.chain_id() == 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); } + Ok(self.sign_transaction_sync(&tx_with_chain)) } async fn sign_typed_data( @@ -109,7 +98,7 @@ impl> 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> Signer fo } impl> Wallet { - /// 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 - /// signature with EIP-155 if the flag is set to true. - pub fn sign_hash(&self, hash: H256, eip155: bool) -> Signature { + /// Signs the provided hash. + pub fn sign_hash(&self, hash: H256) -> Signature { let recoverable_sig: RecoverableSignature = self.signer.sign_digest(Sha256Proxy::from(hash)); - let v = if eip155 { - to_eip155_v(recoverable_sig.recovery_id(), self.chain_id) - } else { - u8::from(recoverable_sig.recovery_id()) as u64 + 27 - }; + let v = u8::from(recoverable_sig.recovery_id()) as u64 + 27; let r_bytes: FieldBytes = recoverable_sig.r().into(); let s_bytes: FieldBytes = recoverable_sig.s().into(); diff --git a/ethers-signers/src/wallet/private_key.rs b/ethers-signers/src/wallet/private_key.rs index eb3f2cfb..d2bd75fa 100644 --- a/ethers-signers/src/wallet/private_key.rs +++ b/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 { @@ -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::
().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 = + "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 =