LedgerHQ / app-ethereum

Ethereum wallet application for Ledger devices
Apache License 2.0
204 stars 224 forks source link

Support for EIP-712 message signing #105

Closed Mrtenz closed 11 months ago

Mrtenz commented 4 years ago

It seems to be not possible to sign EIP-712-based messages right now, because the message is prepended with \x19Ethereum Signed Message:\n here: https://github.com/LedgerHQ/app-ethereum/blob/8d0544bf68906f2a47a9013409792bf986272028/src_features/signMessage/cmd_signMessage.c#L57

This is already implemented by MetaMask, and it would be nice to be able to sign EIP-712 messages with Ledger devices as well.

TamtamHero commented 4 years ago

We're currently testing it, an app update should be available soon® (cf: https://github.com/LedgerHQ/app-ethereum/tree/eip712_v0)

A2be commented 3 years ago

What is current status on Ledger's implementation of EIP712 to support transaction signing?

MetaMask, Gnosis Safe multisig, and many others support it, and without it, the security case for using a Ledger in concert with these other wallets goes down significantly. Pinging @TamtamHero

TamtamHero commented 3 years ago

This is supported already: https://github.com/LedgerHQ/app-ethereum/issues/105

A2be commented 3 years ago

@TamtamHero : It appears EIP-712 compatibility is also not working with the Gnosis Safe Multisig, per discussion on Gnosis #safe Discord ( [https://discord.com/channels/477106835862716416/477391201708802058/801466336873283594] ).

Maybe you and @tobias_s from Gnosis could chat to see if the Gnosis dev team is aware that Ledger think EIP-712 is correctly implemented.

tschubotz commented 3 years ago

This is supported already: #105

@TamtamHero You linked this exact same isse (#105) Did you mean to link another issue in your comment? :)

That would be great news if Ledger supports EIP712! With which Firmware update did that go live?

TamtamHero commented 3 years ago

indeed, my bad! https://github.com/LedgerHQ/app-ethereum/blob/master/doc/ethapp.asc#sign-eth-eip-712 Available since https://github.com/LedgerHQ/app-ethereum/commit/91e4b4577a839d3b66c1bb9345584916c71be818 So, since Eth v1.6.0

tschubotz commented 3 years ago

Oh nice! Would a user actually be able to review the different EIP712 fields on their Ledger display? I still just see the hash to sign, and not the human readable message fields. Now I'm wondering if I do anything wrong.

Mrtenz commented 3 years ago

Is this implemented in Ledger.js, or do we have to manually send the data to the device for now? Either way, sweet that this has been implemented.

TamtamHero commented 3 years ago

For implementation version 0, the domain hash and message hash are provided to the device, which displays them and returns the signature So, for the time being, the data is not displayed on the device.

The call is available from ledger.js: https://github.com/LedgerHQ/ledgerjs/tree/master/packages/hw-app-eth#signeip712hashedmessage

A2be commented 3 years ago

With the data not displayed on the device, the user who wants to carefully validate their signature on, say, a Gnosis Safe Multisig transaction approval/signing, cannot actually validate that they are signing the correct transaction.

So my encouragement for you is to definitely fix it so that the user can use the Ledger display device to usefully, and securely, approve an EIP-712 transaction.

SCBuergel commented 3 years ago

Bump.

This is a serious concern for e.g. Gnosis Safe users (reminder: if you want to claim your wallet protects a large amount of ETH and Ethereum based value then you must support Gnosis Safe) who are currently unable to verify the tx idahoan on screen - not even with hex code. All you currently see us a hash. So right now you need a laptop to verify the hash and that laptop is the device you didn't trust in the first place and therefore got a hardware wallet.

SCBuergel commented 3 years ago

Ok the issue seems resolved, see above. I suggest to close this issue.

Xeonus commented 3 years ago

Now that Uniswap v3 uses EIP-712 for signing transactions like LP migration, can you update users on the status of this issue? Otherwise anyone using Metamask together with Ledger is stuck on Uniswap v2 for the time-being.

pscott commented 3 years ago

@Xeonus EIP-712 is already supported by the Ledger. Metamask has not updated its hw-app-eth dependency though. You can track the actual issue on their repo.

Thanks! :D

mocolicious commented 3 years ago

Update to 2.0 firmware and works like a charm https://github.com/MetaMask/metamask-extension/issues/10240#issuecomment-833936849

gorgos commented 3 years ago

Still waiting for the support to show all data on the device. That's the whole point of the display not to sign random things.

pscott commented 3 years ago

Still waiting for the support to show all data on the device. That's the whole point of the display not to sign random things.

This is on our radar. Unfortunately we have higher priority issues :)

In the meantime, feel free to check out the already existing EIP712 interal plugin and contribute! :)

mazurroman commented 2 years ago

Bump. This is a serious concern to us. Is there an ETA on resolving this issue?

ghost commented 2 years ago

Unfortunately no, still WIP on our side, we can't give any ETA yet, just hoping for somewhere in Q1 2022 at the moment.

Kalesberg commented 2 years ago

Any updates on this issue?