Once caveat, this now displays data passed in to eth_sign with UTF-8 encoding. Many people using eth_sign are going to prefer the data displayed as hex (i.e. IDEX, ForkDelta, me).

Maybe we could merge #4039 in with this as well?

Forcing a user to see hex seems like a good way for an attacker to obscure a challenge provided by another site, so I'm not sure this should be rolled together with #4039.

@danfinlay good point.

Maybe we can make hex display optional if and only if the data being signed is a valid 32 byte hex string. This greatly reduces the attack surface for obfuscating challenges.

Personally I think the ultimate solution is for the client application to send a list of items and their ABI encoding to MM Array<Item<String,enum{uint,string,etc...}>> and have metamask show that data to the user. Then the user can sign the more-readable data, and MM can hash the data using the soliditySha3 function, then MM will produce a signature for prefix + soliditySha3(data).

I know this has been hashed out every which way, but this proposal is backwards-compatible for almost all contracts, much nicer for users, and has a significantly reduced attack surface.

Hey @FlySwatter as we discussed earlier I would prefer if this is not merged in until EIP712 is finalized. Right now the signed messages conforming to EIP191 (for instance in simple-multisig) cannot be signed using Metamask if Metamask forces the prefix. Ideally these kinds of offline signed messages should be using EIP712 but since that has not been finalized we're in a bit of a limbo. EIP712 does look like it's in pretty good shape though so hopefully will be finalized soon.

Marking DONOTMERGE until we finalize EIP712 for those reasons.

@danfinlay How are we looking for this to be merged now that EIP712 has been merged?
Its still a bit confusing with multiple signers behaving in non spec compliant ways (w.r.t the prefix being added inside the signer). Our code either must attempt to discover MM is implementing eth_sign or request the user to specify (so we add the prefix before calling eth_sign which is not to spec). This gets trickier as MM adds hardware support which does prefix correctly (Ledger and Trezor).

One thing I have noticed is the inconsistencies with eth_sign personal_sign and the encoding of the message to be signed. We recently worked with Trezor to support signing hex messages (with prefix added on the device, so you can't be tricked into signing a transaction). This was broken since it used utf8 as an encoding which means you can't sign a keccak256 message (pretty common scenario). Trezor has added support for this. I am concerned this PR might result in the same issues?

It also appears that the MM Trezor integration only supports personal_sign and not eth_sign #5218.

It would be a happy day when we can truly be agnostic to the underlying eth_sign provider!

@danfinlay If the hesitation with changing eth_sign's implementation is backward compatibility, you can do the migration in steps:

1. deprecate the current behavior of eth_sign printing a helpful warning to the console.log about how the behavior is going to change in ~1-2months time. At the same time, add another JSON RPC endpoint called eth_sign_v2 with a spec-compliant implementation of eth_sign and ask developers to move over to using this endpoint.
2. After ~1-2months, change eth_sign endpoint to also be spec compliant. New developers can then make the correct assumptions about eth_sign. Legacy apps continue to use eth_sign_v2 or it can later be deprecated in favor of eth_sign.

It's a bit of a dance, but if Metamask doesn't make this change, everyone building ontop of it will need to implement some hacky solution (e.g Ethers.js Web3 Provider: https://github.com/ethers-io/ethers.js/blob/master/providers/web3-provider.js#L54) which introduces new bugs, like the one @dekz mentioned about some hashes becoming unsignable b/c of the forced UTF8 encoding/decoding in personal_sign.

I would also suggest just dropping eth_sign support since it has had such a shaky history of changing specs and incompatible implementations

@kumavis that would make Metamask inconsistent with every other provider. Perhaps its worth just discussing all the things that are a problem with eth_sign and how it relates to Metamask.

I would suggest modifying the eth_sign behaviour in Metamask to be consistent with the JSON RPC definition of eth_sign. It seems to be equivalent to what you call personal_sign (which most providers don't define and has no spec).

Other tools and libraries are required to write work arounds for Metamask since it is different from other providers. E.g Ethers.js, 0x.js. I think we can all agree that this is a problem.

For reference in this discussion, here is what is defined in the JSON RPC interface for eth_sign.

The sign method calculates an Ethereum specific signature with: sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))).

By adding a prefix to the message makes the calculated signature recognisable as an Ethereum specific signature. This prevents misuse where a malicious DApp can sign arbitrary data (e.g. transaction) and use the signature to impersonate the victim.

Parameters
account, message

DATA, 20 Bytes - address.
DATA, N Bytes - message to sign.
Returns
DATA: Signature

So it takes in message and returns sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))). That is it prefixes internally on the signer before signing and the message is sent over the wire without the prefix.

This is how Trezor and Ledger both operate, where they prefix on the device itself.

If your concern is breaking dApps that are requiring signed hashes without a prefix then perhaps you can migrate them to eth_sign_v1. I completely understand that eth_sign and the prefix changed from the time MM was started, and dApps were created which used this original signing implementation. But continuing to support this old insecure behaviour comes at a cost of work arounds in other tools and confusion for developers.

personal_sign has already caused issues with MM support of hardware devices because it converts into ascii or utf8. This encoding cannot represent a hash since there are bytes which aren't valid in uft8
and get replaced with error characters. When passed to the hardware signer it signs these error characters which results in an invalid signature for a hash. We've worked with trezor to fix this trezor/connect#216 but there is currently no way to sign a hash on a Trezor device via MM (since personal_sign always converts to ascii/utf8).

I'll be at devcon and @ricmoo and @fabioberger and I plan on setting aside some time to discuss Providers in general and a compliance test suite. We'd love for you to attend as well.

For now, this implementation of eth_sign has become a foundation for a considerable amount of the ecosystem, and the error message we added to it has seemed to adequately mitigate the security concerns.

For those reasons of non-breaking and reduced security concerns, we are closing for now. Petitions to change this behavior should be discussed in #2215.

This is so fascinating and i wish someone could break it down to help me understand so i can finally do this myself...