Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Hardware wallets don't support signing arbitrary messages: "Unknown binary data" #6083

Closed
politician opened this issue Jul 18, 2017 · 13 comments · Fixed by #8868
Closed

Hardware wallets don't support signing arbitrary messages: "Unknown binary data" #6083

politician opened this issue Jul 18, 2017 · 13 comments · Fixed by #8868
Assignees
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M7-signer 🔏 Trusted signer. P7-nicetohave 🐕 Issue is worth doing eventually.
Milestone

Comments

@politician
Copy link

politician commented Jul 18, 2017

I'm using Parity/1.6.8/1.17.0/ma
I'm using the latest Parity Chrome extension
I'm using a Nano Ledger S with Ethereum app v1.0.8
I'm browsing the Dapp https://etherdelta.github.io

If I try to place a buy or a sell order on a pair, I get the following error and I cannot confirm request (it doesn't even come up on the nano like it should do)
screen shot 2017-07-18 at 18 43 21

When I go to my parity UI, I can see in the signer this request and when I try to confirm it from there, it throws the error "[-32021] Account password is invalid or account does not exist." while the expected behaviour is to prompt on my hardware wallet to confirm transaction.
screen shot 2017-07-18 at 18 52 12

To be noted that I can fill existing buy or sell orders, but cannot place them. It works like a charm with the default parity wallet without using a hardware wallet.

@politician politician changed the title Needs password to make a tx when connected with hardware wallet and Parity chrome on Dapp Needs password to make a tx when connected with hardware wallet and Parity chrome Jul 18, 2017
@5chdn
Copy link
Contributor

5chdn commented Jul 19, 2017

Just to make sure, is contract support enabled on your ledger?

Does your ledger show up in accounts list as colored identicon or gray?

@5chdn 5chdn added F2-bug 🐞 The client fails to follow expected behavior. M7-signer 🔏 Trusted signer. labels Jul 19, 2017
@politician
Copy link
Author

politician commented Jul 19, 2017

Yes contract support is enabled. Ledger shows up as a colored icon.
Food for thoughts: I believe Etherdelta handles placing a buy/sell order differently than actually buying or selling into an existing order. It seems like it's not doing a transaction but rather authorizing Etherdelta to make this transaction later. @frenchhoudini @zackcoburn could help understand this.

@zackcoburn
Copy link

zackcoburn commented Jul 19, 2017 via email

@alfiehub
Copy link

alfiehub commented Aug 12, 2017

I also have the same problem using Parity 1.7.0 and EtherDelta.
If anyone has any pointers to where the problem might lie I'll be glad to help out.

@tomusdrw
Copy link
Collaborator

Looks like hardware support for signatures is missing, somewhere over here:
https://github.com/paritytech/parity/blob/604ea5d684b05f63ab2a32f68fdf50b8bbe38498/rpc/src/v1/helpers/dispatch.rs#L516

I'll have a look.

@tomusdrw
Copy link
Collaborator

Since the hardware wallets don't support signing arbitrary messages yet it's not possible to implement that.
Please re-open if the hardware supports this.

@tomusdrw tomusdrw added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. and removed F2-bug 🐞 The client fails to follow expected behavior. labels Aug 16, 2017
@5chdn 5chdn changed the title Needs password to make a tx when connected with hardware wallet and Parity chrome Hardware wallets don't support signing arbitrary messages Aug 17, 2017
@5chdn 5chdn changed the title Hardware wallets don't support signing arbitrary messages Hardware wallets don't support signing arbitrary messages: "Unknown binary data" Aug 17, 2017
@btchip
Copy link

btchip commented Aug 18, 2017

It should support personal signing (see https://github.com/LedgerHQ/ledger-node-js-api/blob/master/src/ledger-eth.js#L113) - is this what you're looking for ?

@arkpar
Copy link
Collaborator

arkpar commented Aug 18, 2017

@btchip Interesting, this could use some documentation at https://raw.githubusercontent.com/LedgerHQ/blue-app-eth/master/doc/ethapp.asc

What is the minimum required eth app version that supports this? Is the message presented on the device display hex-encoded?

@5chdn 5chdn reopened this Aug 18, 2017
@btchip
Copy link

btchip commented Aug 18, 2017

that's right, I'll update the documentation. This message has been added into version 1.0.8 - the device displays a sha256 hash of the message sent to be signed (to handle the case where a long binary message is sent)

@5chdn 5chdn added the P7-nicetohave 🐕 Issue is worth doing eventually. label Sep 4, 2017
@5chdn
Copy link
Contributor

5chdn commented Sep 9, 2017

This is not limited to hardware wallets, isn't it?

normal

@mrbrdo
Copy link

mrbrdo commented Sep 20, 2017

This is crucial to support using EtherDelta with Ledger Nano through Parity (I don't think there is any other alternative right now to use ED with a hardware wallet)!

👍 @arkpar

@5chdn 5chdn added this to the 1.9 milestone Oct 5, 2017
@5chdn 5chdn modified the milestones: 1.9, 1.10 Jan 5, 2018
@5chdn 5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018
@5chdn 5chdn modified the milestones: 1.11, 1.12 Mar 1, 2018
@5chdn 5chdn modified the milestones: 1.12, 1.13 Apr 24, 2018
@folsen
Copy link
Contributor

folsen commented May 19, 2018

@tomusdrw This isn't implemented on the client side yet right? I don't believe we necessarily want to do any UI changes, but we should probably have this ability on node-side. If it's not done then maybe @niklasad1 can pick it up?

@niklasad1 niklasad1 self-assigned this Jun 5, 2018
@niklasad1
Copy link
Collaborator

Update, I got it working but it will take some time to get it PR ready!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M7-signer 🔏 Trusted signer. P7-nicetohave 🐕 Issue is worth doing eventually.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants