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 <me@gakonst.com>

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",

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

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"] }
 

ethers-signers/src/aws/mod.rs

@@ -256,7 +256,7 @@ impl<'a> super::Signer for AwsSigner<'a> {
 
     async fn sign_typed_data<T: Eip712 + Send + Sync>(
         &self,
-        payload: T,
+        payload: &T,
     ) -> Result<EthSig, Self::Error> {
         let hash = payload
             .encode_eip712()

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,
+        transaction::{eip2718::TypedTransaction, eip712::Eip712},
-        TransactionRequest, TxHash, H256, U256,
+        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<T>(&self, payload: &T) -> Result<Signature, LedgerError>
+    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<u8>,
+        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();
+    }
 }

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<T: Eip712 + Send + Sync>(
         &self,
-        payload: T,
+        payload: &T,
     ) -> Result<Signature, Self::Error> {
-        let hash = payload
+        self.sign_typed_struct(payload).await
-            .encode_eip712()
-            .map_err(|e| Self::Error::Eip712Error(e.to_string()))?;
-
-        let sig = self.sign_message(hash).await?;
-
-        Ok(sig)
     }
 
     /// Returns the signer's Ethereum Address

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

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<T: Eip712 + Send + Sync>(
         &self,
-        payload: T,
+        payload: &T,
     ) -> Result<Signature, Self::Error>;
 
     /// Returns the signer's Ethereum Address

ethers-signers/src/wallet/mod.rs

@@ -88,7 +88,7 @@ impl<D: Sync + Send + DigestSigner<Sha256Proxy, RecoverableSignature>> Signer fo
 
     async fn sign_typed_data<T: Eip712 + Send + Sync>(
         &self,
-        payload: T,
+        payload: &T,
     ) -> Result<Signature, Self::Error> {
         let encoded = payload
             .encode_eip712()