Replace eth_sign behavior with eth_personalSign behavior

[danfinlay]

We are actually doing eth_sign and eth_personalSign both wrong.

personal_* methods are meant to take a password, and never be exposed to Dapps.

eth_sign is still doing the deprecated signing method, and is basically only supported because EtherDelta still uses it.

Eventually we should make our eth_sign method do what our personal_sign method currently does, which is what Geth and Parity both do anyways.

[bwheeler96]

@danfinlay what does this mean for developers who simply want to sign a blob of data with the "\x19Ethereum Signed Message:\n" + msg.length prefix?

I feel like I've tried every possible combination of persona.sign, eth.sign, prefixed, unprefixed, etc, and I still can not get a message signed the same way I can in my truffle tests.

Example using TestRPC:

testrpc -m 'candy maple cake sugar pudding cream honey rich smooth crumble sweet treat'

Test private key: c87509a1c067bbde78beb793e6fa76530b6382a4c0241e5e4a9ec0a0f44dc0d3

hash: 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470

Truffle and/or backend:

// ==== Truffle/Backend
let Web3 = require('web3')
let web3 = new web3(new Web3.providers.HttpProvider('http://localhost:8545'))
web3.eth.sign(web3.eth.coinbase, hash)

// => '0xb78228cafc42162e1b6a4b3da87da1fe7f34a4411b59c834f09317dceb7d017f09d8e0304d145f85fc9e88d0cf30bc52aa4e4e22ba42c1269d1e4b32aaf98fa701'

MetaMask:

web3.eth.sign(web3.eth.coinbase, hash, console.log)
// 0x4a35a4b58d7e3316d00a8f12d386730c426884d206914e69b2e5c1bac2f3a213262fe88593566feb361f3dace8052a51235996d6b4bb6f8392edc02ebb9a97b71c

Doesn't really seem like there is an exhaustive guide for producing consistent results of message signing. I'd be happy to write one up, if I had that knowledge.

Will future versions of metamask make it easier to sign data in this way, consistently?

[danfinlay]

The sign methods have been horribly abused by basically every client.

I think we’ve made our best effort at showing how to get these signatures in this sample dapp:
https://github.com/danfinlay/js-eth-personal-sign-examples

It relies on a module we maitain called eth-sig-util. I can’t comment on truffle’s sign approach, except that seriously, every client signs differently, and it’s awful, and that’s why I’m hoping to turn a new, better specified page with eth_signTypedData.

[danfinlay]

I wouldn’t be surprised if truffle’s sign method is equivalent to MetaMask’s personal sign method.

[dryruner]

Has this been implemented already? I tested web3.eth.sign() in my chrome console with metamask enabled, it seems it doesn't prepend the "\x19Ethereum Signed Message:\n${msg.length}" prefix. However web3.personal.sign() does.

Rather confused as testrpc and geth and metamask has different behaviors, what is the standard expected behavior?

[bwheeler96]

what is the standard expected behavior?

@johnweldon there is no standard, don't expect anything.

MetaMask eth_sign will not prepend the message prefix. Use personal.sign for that, but keep in mind, personal_sign only takes hex data and displays it as UTF-8, so if you need to sign arbitrary data you gotta hash it, sign it, and recover it in the contract.

Best to read the source code in the metamask core repository to fully understand.

[dryruner]

Cool thank you @bwheeler96 , can you share me a code pointer to signing in metamask?

Also as regards to the title in this topic - would metamask make eth_sign signing like eth_personalSign in future?

Thanks!

[danfinlay]

can you share me a code pointer to signing in metamask?

Here are examples of every currently supported MetaMask signing method.

would metamask make eth_sign signing like eth_personalSign in future?

Yes, we need to do this, it is just going to be painful for the Dapps that have come to rely on eth_sign, but they have ignored the security warnings for over a year at this point, so maybe it's about time.

[bwheeler96]

@danfinlay FWIW I think nearly every smart contract at this point is designed to use the prefixed signature, just the frontends aren't updated. IDEX being a current example.

[danfinlay]

Alright @bwheeler96, I agree it's long overdue. I've opened a PR for team review:
#4117

[bdresser]

blocked by EIP 712

[dekz]

@danfinlay you closed #4117 and referenced this issue.

personal_* methods are meant to take a password, and never be exposed to Dapps.

eth_sign is still doing the deprecated signing method, and is basically only supported because EtherDelta still uses it.

Is this still Metamask's position that all dApps going forward must create work arounds for Metamask's old implementation of eth_sign?

From a developer perspective this is what we see:

1. There was issues with eth_sign behaviour which Metamask implemented
2. Spec is clarified to increase security of eth_sign by prefixing the message
3. Metamask continues to use the old eth_sign behaviour basically only supported because EtherDelta still uses it
4. CoinbaseWallet and other web3 wallets copy this behaviour to be compatible with Metamask (are they implementing the checks to prevent an ethereum transaction from being signed?)
5. Developers play a whack-a-mole for these wallets which may change behaviour at any time.

Are we just biding out time until EIP712 is greatly adopted, and keeping eth_sign just for EtherDelta?

You probably have the analytics of who's using eth_sign to be able to speak to how many dApps this change could break, so it would be great to include that in any reasoning. I personally don't agree that the most downloaded Web3 Wallet having a broken spec for one dApp is a good enough reason.

[Azier777]

im still trying to understand this. im just so confuzed

[danfinlay]

It is not the case that only EtherDelta uses eth_sign, it turns out it is actually fairly popular, especially among developers who are implementing simple MVP signing methods, which I know because we had complaints when eth_sign wasn't available in our mobile app.

Those developers indicate it is useful to have a low-level signing method available like eth_sign, and I no longer believe that it is security critical to remove it, as we've found that adding an intimidating warning was enough to prevent users from signing messages from untrusted sources.

For that reason, the only thing keeping me from closing this issue now is the possibility of broader ecosystem compatibility. If it's true that developers are playing whack-a-mole, and if it's true that most other wallets implement this method another way, I think we would consider this.

On the other hand, we haven't seen serious outcry over this at all. I'm fairly sure that abstraction libraries like wallet-connect and ethers.js are handling the edge cases for most developers, making the compatibility argument a bit weaker.

I'm pretty sure we don't have metrics on dapp usage of this method, but I will check and get back here if we do. I would also welcome any compatibility table someone wanted to present demonstrating how different clients are handling these methods.

Without evidence of broader incompatibility and developer preference, I am inclined to support continuing the current behavior, and potentially closing this issue as wontfix.

[danfinlay]

Agreed. Closing as a duplicate of #1930.