From 9db36e59fb99c30990d6cff09f27f9bf40abbdfc Mon Sep 17 00:00:00 2001 From: Sebastian Martinez Date: Tue, 19 Oct 2021 10:52:36 +0200 Subject: [PATCH] fix: eip712 signing with ledger hw (#518) * fix: eip712 signing with ledger hw This commit fixes the way a EIP712 derived struct is sent to the Ledger HW to obtain the signature. It also checks if the Ledger ETH app is at least 1.6.0 since this is a requirement. * fix: apply cargo fmt * fix: revert ledger eip712 test * refactor: remove unused code * test: add ledger eip712 test * fix: cargo fmt * fix: move the logic to app.rs * feat: make sign_typed_data take a reference * chore: ensure abigen is enabled in ledger tests we need this to compile ethers-contract as a standalone package * test: ensure that the ledger sig verifies * test: pass foo_bar by ref Co-authored-by: Georgios Konstantopoulos --- Cargo.lock | 4 ++ ethers-contract/tests/contract.rs | 2 +- ethers-signers/Cargo.toml | 4 ++ ethers-signers/src/aws/mod.rs | 2 +- ethers-signers/src/ledger/app.rs | 86 ++++++++++++++++++++++++++++-- ethers-signers/src/ledger/mod.rs | 11 ++-- ethers-signers/src/ledger/types.rs | 7 +++ ethers-signers/src/lib.rs | 2 +- ethers-signers/src/wallet/mod.rs | 2 +- 9 files changed, 104 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5151aebb..9cdfa439 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1025,13 +1025,17 @@ dependencies = [ "coins-ledger", "elliptic-curve", "eth-keystore", + "ethers-contract", "ethers-core", + "ethers-derive-eip712", "futures-executor", "futures-util", "hex", "rand 0.8.4", "rusoto_core", "rusoto_kms", + "semver 1.0.4", + "serde_json", "sha2 0.9.8", "spki", "tempfile", diff --git a/ethers-contract/tests/contract.rs b/ethers-contract/tests/contract.rs index 81e5dd5c..78417423 100644 --- a/ethers-contract/tests/contract.rs +++ b/ethers-contract/tests/contract.rs @@ -660,7 +660,7 @@ mod eth_tests { }; let sig = wallet - .sign_typed_data(foo_bar.clone()) + .sign_typed_data(&foo_bar) .await .expect("failed to sign typed data"); diff --git a/ethers-signers/Cargo.toml b/ethers-signers/Cargo.toml index 8f5eb082..a0f4e4c7 100644 --- a/ethers-signers/Cargo.toml +++ b/ethers-signers/Cargo.toml @@ -27,6 +27,7 @@ rand = { version = "0.8.4", default-features = false } yubihsm = { version = "0.39.0", features = ["secp256k1", "http", "usb"], optional = true } futures-util = "0.3.17" futures-executor = "0.3.17" +semver = "1.0.4" # aws rusoto_core = { version = "0.47.0", optional = true } @@ -39,6 +40,9 @@ spki = { version = "0.4.1", optional = true } eth-keystore = { version = "0.3.0" } [dev-dependencies] +ethers-contract = { version = "^0.5.0", path = "../ethers-contract", features = ["eip712", "abigen"]} +ethers-derive-eip712 = { version = "0.1.0", path = "../ethers-core/ethers-derive-eip712" } +serde_json = { version = "1.0.64" } tracing-subscriber = "0.2.25" yubihsm = { version = "0.39.0", features = ["secp256k1", "usb", "mockhsm"] } diff --git a/ethers-signers/src/aws/mod.rs b/ethers-signers/src/aws/mod.rs index 36b4faec..e1b550f1 100644 --- a/ethers-signers/src/aws/mod.rs +++ b/ethers-signers/src/aws/mod.rs @@ -256,7 +256,7 @@ impl<'a> super::Signer for AwsSigner<'a> { async fn sign_typed_data( &self, - payload: T, + payload: &T, ) -> Result { let hash = payload .encode_eip712() diff --git a/ethers-signers/src/ledger/app.rs b/ethers-signers/src/ledger/app.rs index 8770bdbd..f17aac1e 100644 --- a/ethers-signers/src/ledger/app.rs +++ b/ethers-signers/src/ledger/app.rs @@ -8,8 +8,8 @@ use futures_util::lock::Mutex; use ethers_core::{ types::{ - transaction::eip2718::TypedTransaction, Address, NameOrAddress, Signature, Transaction, - TransactionRequest, TxHash, H256, U256, + transaction::{eip2718::TypedTransaction, eip712::Eip712}, + Address, NameOrAddress, Signature, Transaction, TransactionRequest, TxHash, H256, U256, }, utils::keccak256, }; @@ -29,6 +29,8 @@ pub struct LedgerEthereum { pub(crate) address: Address, } +const EIP712_MIN_VERSION: &str = ">=1.6.0"; + impl LedgerEthereum { /// Instantiate the application by acquiring a lock on the ledger device. /// @@ -136,7 +138,38 @@ impl LedgerEthereum { self.sign_payload(INS::SIGN_PERSONAL_MESSAGE, payload).await } - // Helper function for signing either transaction data or personal messages + /// Signs an EIP712 encoded domain separator and message + pub async fn sign_typed_struct(&self, payload: &T) -> Result + where + T: Eip712, + { + // See comment for v1.6.0 requirement + // https://github.com/LedgerHQ/app-ethereum/issues/105#issuecomment-765316999 + let req = semver::VersionReq::parse(EIP712_MIN_VERSION)?; + let version = semver::Version::parse(&self.version().await?)?; + + // Enforce app version is greater than EIP712_MIN_VERSION + if !req.matches(&version) { + return Err(LedgerError::UnsupportedAppVersion( + EIP712_MIN_VERSION.to_string(), + )); + } + + let domain_separator = payload + .domain_separator() + .map_err(|e| LedgerError::Eip712Error(e.to_string()))?; + let struct_hash = payload + .struct_hash() + .map_err(|e| LedgerError::Eip712Error(e.to_string()))?; + + let mut payload = Self::path_to_bytes(&self.derivation); + payload.extend_from_slice(&domain_separator); + payload.extend_from_slice(&struct_hash); + + self.sign_payload(INS::SIGN_ETH_EIP_712, payload).await + } + + // Helper function for signing either transaction data, personal messages or EIP712 derived structs pub async fn sign_payload( &self, command: INS, @@ -200,9 +233,30 @@ impl LedgerEthereum { mod tests { use super::*; use crate::Signer; - use ethers_core::types::{Address, TransactionRequest}; + use ethers_contract::EthAbiType; + use ethers_core::types::{ + transaction::eip712::Eip712, Address, TransactionRequest, I256, U256, + }; + use ethers_derive_eip712::*; use std::str::FromStr; + #[derive(Debug, Clone, Eip712, EthAbiType)] + #[eip712( + name = "Eip712Test", + version = "1", + chain_id = 1, + verifying_contract = "0x0000000000000000000000000000000000000001", + salt = "eip712-test-75F0CCte" + )] + struct FooBar { + foo: I256, + bar: U256, + fizz: Vec, + buzz: [u8; 32], + far: String, + out: Address, + } + #[tokio::test] #[ignore] // Replace this with your ETH addresses. @@ -269,4 +323,28 @@ mod tests { let addr = ledger.get_address().await.unwrap(); sig.verify(message, addr).unwrap(); } + + #[tokio::test] + #[ignore] + async fn test_sign_eip712_struct() { + let ledger = LedgerEthereum::new(DerivationType::LedgerLive(0), 1u64) + .await + .unwrap(); + + let foo_bar = FooBar { + foo: I256::from(10), + bar: U256::from(20), + fizz: b"fizz".to_vec(), + buzz: keccak256("buzz"), + far: String::from("space"), + out: Address::from([0; 20]), + }; + + let sig = ledger + .sign_typed_struct(&foo_bar) + .await + .expect("failed to sign typed data"); + let foo_bar_hash = foo_bar.encode_eip712().unwrap(); + sig.verify(foo_bar_hash, ledger.address).unwrap(); + } } diff --git a/ethers-signers/src/ledger/mod.rs b/ethers-signers/src/ledger/mod.rs index f2f42fe3..5d9096e2 100644 --- a/ethers-signers/src/ledger/mod.rs +++ b/ethers-signers/src/ledger/mod.rs @@ -27,17 +27,12 @@ impl Signer for LedgerEthereum { self.sign_tx(message).await } + /// Signs a EIP712 derived struct async fn sign_typed_data( &self, - payload: T, + payload: &T, ) -> Result { - let hash = payload - .encode_eip712() - .map_err(|e| Self::Error::Eip712Error(e.to_string()))?; - - let sig = self.sign_message(hash).await?; - - Ok(sig) + self.sign_typed_struct(payload).await } /// Returns the signer's Ethereum Address diff --git a/ethers-signers/src/ledger/types.rs b/ethers-signers/src/ledger/types.rs index 31cee31a..fd85e59a 100644 --- a/ethers-signers/src/ledger/types.rs +++ b/ethers-signers/src/ledger/types.rs @@ -42,9 +42,15 @@ pub enum LedgerError { #[error(transparent)] /// Error when converting from a hex string HexError(#[from] hex::FromHexError), + #[error(transparent)] + /// Error when converting a semver requirement + SemVerError(#[from] semver::Error), /// Error type from Eip712Error message #[error("error encoding eip712 struct: {0:?}")] Eip712Error(String), + /// Error when signing EIP712 struct with not compatible Ledger ETH app + #[error("Ledger ethereum app requires at least version: {0:?}")] + UnsupportedAppVersion(String), } pub const P1_FIRST: u8 = 0x00; @@ -57,6 +63,7 @@ pub enum INS { SIGN = 0x04, GET_APP_CONFIGURATION = 0x06, SIGN_PERSONAL_MESSAGE = 0x08, + SIGN_ETH_EIP_712 = 0x0C, } #[repr(u8)] diff --git a/ethers-signers/src/lib.rs b/ethers-signers/src/lib.rs index 3436a239..95430378 100644 --- a/ethers-signers/src/lib.rs +++ b/ethers-signers/src/lib.rs @@ -99,7 +99,7 @@ pub trait Signer: std::fmt::Debug + Send + Sync { /// Payload must implement Eip712 trait. async fn sign_typed_data( &self, - payload: T, + payload: &T, ) -> Result; /// Returns the signer's Ethereum Address diff --git a/ethers-signers/src/wallet/mod.rs b/ethers-signers/src/wallet/mod.rs index 4331b389..ad170d6e 100644 --- a/ethers-signers/src/wallet/mod.rs +++ b/ethers-signers/src/wallet/mod.rs @@ -88,7 +88,7 @@ impl> Signer fo async fn sign_typed_data( &self, - payload: T, + payload: &T, ) -> Result { let encoded = payload .encode_eip712()