Address recovered from EIP712 struct signature differs between Ledger and Wallet implementation

[sebastinez]

Hi folks,

Working on using the ability to sign a struct with the new Eip712 trait and derive macro but it found a divergence between how the Ledger and the Wallet implementation signs the encoded struct hash, and the signee address that a smart contract returns using a ecrecover function

/// The encoded eip712 payload to be signed.
[141, 121, 46, 2, 93, 58, 190, 150, 20, 195, 88, 218, 130, 150, 197, 99, 184, 149, 178, 157, 188, 156, 138, 181, 49, 221, 19, 132, 203, 97, 135, 151]

/// The Signature that gets returned by signing the payload with a LocalWallet
{
  r: 79865770893275451639175808178507952192629851718732781884621688237624443345747,
  s: 42733071147199090845628331540487276759362293016997477521278730816100529639948,
  v: 27,
}

/// The address of a keystore file vs the recovered address from a solidity smart contract
0x589aa5f2e23da3cce0e13289c5d6fab2646c2aad = 0x589aa5f2e23da3cce0e13289c5d6fab2646c2aad

/// The Signature that gets returned by signing the payload with a Ledger HW
{
  r: 28154581424745245939411447948177125479178002747754055933896495342865066545614,
  s: 14878669590408705366957081261318383581626635578079553970907333265222038138021,
  v: 28,
}

///The address of the Ledger HW does not match the recovered address..
0x1CF47b2EE0BF03de1A22De223B2B94Cd9580d942 != 0xac79765881e2de8f54de06dd22a3db96a53b5daf

The smart contract function which I'm using to return the signee:

    function _recover(
        bytes32 hash,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public pure returns (address) {
        require(
            uint256(s) <=
                0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0,
            "ECDSA: invalid signature 's' value"
        );
        require(v == 27 || v == 28, "ECDSA: invalid signature 'v' value");

        // If the signature is valid (and not malleable), return the signer address
        address signer = ecrecover(hash, v, r, s);
        require(signer != address(0), "ECDSA: invalid signature");

        return signer;
    }

I saw that in the LedgerEthereum implementation the sign_message function has some prepending.

ethers-rs/ethers-signers/src/ledger/app.rs

Line 129 in c3528b7

pub async fn sign_message<S: AsRef<[u8]>>(&self, message: S) -> Result<Signature, LedgerError> {

It seems it is necessary for the Ledger HW to be able to sign the message. Could it be possible that this diverges the final recovered address?

I'm trying to fix this, but since it works using the LocalWallet implementation it seems to be an issue with the Ledger implementation.

I add @Ryanmtate to this issue, so he is up to date.

[Ryanmtate]

Reading through the ledger app-ethereum I found some documentation on ledger's sign eth eip712 method.

Going to see if I can use this information to fix the sign_typed_data method.

[prestwich]

@Ryanmtate please include a firmware check to prevent attempted signing on outdated devices

This command has been supported since firmware version 1.5.0

[Ryanmtate]

@Ryanmtate please include a firmware check to prevent attempted signing on outdated devices

This command has been supported since firmware version 1.5.0

I noticed a method to return the ethereum app version, but don't immediately know how to check the ledger firmware version.

We could enforce that the ethereum app version must be greater than v1.6.0, which might suffice for preventing unsupported use in older devices.

Will add this check at the very least.

[gakonst]

Fixed in #518. @sebastinez mentioned that on Nano X there seems to be a problem:

thread 'ledger::app::tests::test_sign_tx' panicked at 'range end index 33 out of range for slice of length 4', ethers-signers/src/ledger/app.rs:189:40

I tested manually on Nano S and it seemed to work fine. @prestwich does coin-ledger work with nano X?