search

RabbyHub / Rabby

The game-changing wallet for Ethereum and all EVM chains
https://rabby.io
Other
1.35k stars 379 forks source link

Support clear-sign for EIP712 on Ledger #1816

Open 0x398 opened 11 months ago

0x398 commented 11 months ago

Ledger show only hashes when user prompted to sign EIP712 with Rabby and there is no easy way to verify what these hashes correspond to. Signing hashes is dangerous, because it might be phishy permit to transfer WETH, WBTC or monkey without expiration. Ledger supports displaying EIP712 domain and data on the device screen when signing with Ledger Live. Rabby should too! Might be related: https://pypi.org/project/eip712-clearsign/

vvvvvv1vvvvvv commented 11 months ago

Why not verify it via Rabby UI?

0x398 commented 11 months ago

Point of using hardware wallet is no need to trust Rabby UI (or any other software wallet). Why use hardware wallet at all if I will trust software wallet?

vvvvvv1vvvvvv commented 11 months ago

Got your point, does Ledger hardware support this? As you said Ledger supports displaying EIP712 domain and data when signing with Ledger Live, it displays on Ledger Live right? So Ledger Live is software wallet in this case

0x398 commented 11 months ago

Yes, hardware does support this. When I use Ledger Live it displays full EIP712 data on the device. So, I do not need to trust Ledger Live in this case, only to Ledger device.

vvvvvv1vvvvvv commented 11 months ago

OK, will check how Ledger Live make it

vvvvvv1vvvvvv commented 11 months ago

btw, you need to click the button on device several times in this way right? compare with hex hash only one time, don't you think it's too much?

0x398 commented 11 months ago

compare with hex hash only one time, don't you think it's too much?

No, I do not think this is too much. But if somebody thinks so, they can just disable it in the device settings and trust theirs software wallet.

0x398 commented 11 months ago

Maybe it is even disabled by default, @vvvvvv1vvvvvv check ETH application settings (on the device) when you look into this.

vvvvvv1vvvvvv commented 7 months ago

will revert in #2026 since we found this will cause some issues with EIP-712 signature, will also report to Ledger team to fix them

0x398 commented 7 months ago

Could you rather add switch to ask user whether they want to use clear sign or blind sign? Compromising on security to workaround some rather rare bug (I personally had 0 issues with clear sign since it was merged) seems like a very bad decision to me. Adding switch so user can use clear sign but disable it if there are problems seems to be a lot better solution.

vvvvvv1vvvvvv commented 7 months ago

Have to temporary revert since other user meet the issue, no worry, will make it back after Ledger fix the issue, for track: https://github.com/LedgerHQ/ledger-live/issues/6065

vvvvvv1vvvvvv commented 7 months ago

You can use specify version until we make it back, download here: https://github.com/RabbyHub/Rabby/releases/tag/v0.92.49 will update in this issue if Ledger fix it