search

trezor / trezor-mcu

:lock: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU Lesser General Public License v3.0
317 stars 255 forks source link

Ethereum: support SignMessage/Verify Message #163

Closed prusnak closed 7 years ago

prusnak commented 7 years ago

more info: https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign

jhoenicke commented 7 years ago

I'd like to start implementing this.

Do you think it is better to make three new API calls (EthereumSignMessage/EthereumVerifyMessage/EthereumMessageSignature) or reuse the current ones and allow coinname='Ethereum' there. Alternatively, one could reuse MessageSignature and only add Sign/Verify.

I'd go for three new messages.

The signing code is similar but uses keccak instead of sha2

prusnak commented 7 years ago

Let's use new messages like we do for EthereumSignTx and EthereumGetAddress.

jhoenicke commented 7 years ago

There seems to be confusion, whether there should be the "\x19Ethereum Signed Message:\n${messagelen}" header. I decided to not include it, so that the signatures matches what MyEtherWallet or etherscan.io do.

saleemrashid commented 7 years ago

@jhoenicke Doesn't that allow you to sign Ethereum transactions?

jhoenicke commented 7 years ago

@saleemrashid Yes, the situtation is kind of confusing. It looks like the eth_sign functionality in geth/parity is the library and the user tools should call it with the prefix added to the message. However, neither myetherwallet nor etherscan.io do this at the moment. So you can sign a transaction using the sign message feature.

Also the proposed prefix is really strange. You have to sign "\x19Ethereum Signed Message:\n12Some Message". Yes the 12 is in ascii in decimal without any separator added after it.

prusnak commented 7 years ago

@jhoenicke What is the state of this feature? I'd like to release new firmware soon and this would be a nice addition. Is there anything I could help with? (Research? Testing? etc.)

jhoenicke commented 7 years ago

The code is in my ethersign branches in trezor-common, trezor-mcu, and python-trezor. The python-trezor code needs to be updated for the new trezorctl style though.

The code is finished, it signs the raw message without any prefix. Maybe we should at least have a flag that allows to use the Geth prefix, that Etherscan.io allows when verifying messages.

unit test is still missing.

prusnak commented 7 years ago

According to https://github.com/ethereum/go-ethereum/blob/9e5f03b6c487175cc5aa1224e5e12fd573f483a7/internal/ethapi/api.go#L361 it seems they are indeed using ASCII for length, sigh.

I think we should sign WITH prefix and use the correct prefix (=varint one from Bitcoin) and educate others to do the same.

I started an issue here: https://github.com/ethereum/go-ethereum/issues/14794

prusnak commented 7 years ago

Merged your changes in https://github.com/trezor/trezor-mcu/commit/c5e927fac2d8c2b7d4f80d4c9d67b9f05b00d51c

Added prefix (+other maintenance changes) in https://github.com/trezor/trezor-mcu/commit/b0ac3a2af197ea023b133be34857b43a6ef374ba

prusnak commented 7 years ago

I am going to close this issue, let's see how the discussion in the other thread evolves.

prusnak commented 7 years ago

Adding commits where this is implemented in trezor-common/python-trezor for reference:

https://github.com/trezor/trezor-common/commit/b29b98d69ba43571dcbe54dc927aa3ecd2b95113 https://github.com/trezor/python-trezor/commit/23ab43d6129612111c949ae97e3623236c625e0d

Ivshti commented 6 years ago

For anyone wondering how to do this in solidity: https://github.com/AdExBlockchain/adex-core/commit/814bb917fab219c4e11154794aff130451c3046f#diff-a395ac9d4c11715998c09afc5f4ae246R248

Because of the way bitcoin variant length integer works, any number under 254 resolves to a single byte, which you can put in the string with "\x" and then the hex representation of the byte. For example, for a 32, you would do "\x20" (20 being the hex representation of 32)