Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support clear-sign for EIP712 on Ledger #1816

Open
0x398 opened this issue Oct 23, 2023 · 12 comments
Open

Support clear-sign for EIP712 on Ledger #1816

0x398 opened this issue Oct 23, 2023 · 12 comments

Comments

@0x398
Copy link

0x398 commented Oct 23, 2023

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
Copy link
Member

Why not verify it via Rabby UI?

@0x398
Copy link
Author

0x398 commented Oct 24, 2023

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
Copy link
Member

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
Copy link
Author

0x398 commented Oct 24, 2023

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
Copy link
Member

vvvvvv1vvvvvv commented Oct 24, 2023

OK, will check how Ledger Live make it

@vvvvvv1vvvvvv
Copy link
Member

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
Copy link
Author

0x398 commented Oct 24, 2023

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
Copy link
Author

0x398 commented Oct 24, 2023

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

@vvvvvv1vvvvvv
Copy link
Member

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

@vvvvvv1vvvvvv vvvvvv1vvvvvv reopened this Jan 31, 2024
@0x398
Copy link
Author

0x398 commented Jan 31, 2024

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
Copy link
Member

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

@vvvvvv1vvvvvv
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants