Use personal_sign instead of eth_sign for JSON-RPC

[ricmoo]

Background

During frontier the eth_sign operation was used to sign raw digests, which was insecure (see EIP-191 for an explanation), so it was changed to use prefixed messages at the launch of homestead (the official Ethereum chain). However, some wallets, libraries and clients were operating off of older documentation and implemented the eth_sign using the insecure method.

So a new method, personal_sign was introduced to make the distinction between signing digests and signing prefixed messages.

Problem

This resulted in a divide between:

1. eth_sign which signs digests (insecure)
2. eth_sign which signed prefixed messages (ok)
3. personal_sign which signs prefixed messages (ok; identical to the second)

Not all clients behave the same, and various combinations exist:

• only supports 1
• only supports 2
• only supports 3
• supports 1 and 3
• supports 2 and 3 (more confusion ensues, the order for the method parameters are reversed, but some platforms attempt to "guess" if only one of the parameters appears to be an address; yes this is terrible :p)

Yet, event more confusion, as these are difficult and/or impossible to detect which combination the current environment is using. Using any of the above calls can have the following outcome:

• if 1, the client signs an insecure un-prefixed digest.
• if the call does not exist, some clients silently swallow the call
• if the call does not exist, some clients raise an error with the "unsupported method code"
• if the calls is identified as insecure (i.e. 1) or ambiguous (i.e. 2), an error is thrown, but not with "unsupported method code", but instead as a human readable string

And keep in mind that a fully functional eth_sign and personal_sign can return an error if the user rejects in the UI, so it is important to be able to distinguish between "detected method not supported" and "user rejected", especially since I would ideally use personal_sign first and fallback onto eth_sign, if there are still a number of platforms that do not support the former.

Fixing things; I need your help

These days, it appears most clients support the personal_sign method, but I would like some help identifying each client and how it operates today. So, if this issue matters to you, the best way to help us move forward is by testing your platform combination and including information about:

• the platform (MetaMask in browser? Trust on Android? What version?)
• what happens if each of eth_sign and personal_message are called?
• if they fail, what are the full errors of calling these methods?
• what is the full error if the user rejects signing (if the method is supported)

[alfetopito]

Sharing what we have tested until now.

For context, our app uses eth_signTypedData_v4 by default when available, and falls back to eth_sign when not.
(Relevant code path for referece in our app and here is the lib path where ethersjs is used)

So far, we have identified 2 cases where we need to resort to eth_sign:

1. With Ledger, loaded via Metamask.
   In this case, since personal_sign is already used, we had no problems (with ethersjs, we had with MM but that's a different issue =P)
2. With TrustWallet dApp browser on Android
   eth_sign does not work, personal_sign has to be used instead. Thus the PR Use personal_sign when injected web3provider is TrustWallet #1542
   As mentioned in the PR, eth_sign doesn't actually fail. It signs the message, but the decoded data is not what was signed.
   For the version, latest ethersjs (5.1.2), TrustWallet latest from May 1st 2021 (1.29.3 Android)

---

As a side note, I noticed that TrustWallet web3 provider does support eth_signTypedData_v3 which is enough for our use case. Any chance ethers.js could expose another signing method, or offer the option or even fallback to v3 when v4 is not available?

---

Not relevant to this issue but listing here for completeness the other cases that we have working with eth_signTypedData_v4(_signTypedData):

1. Metamask browser extension
2. Metamask mobile dapp browser
3. Metamask WalletConnect
4. TrustWallet WalletConnect

I intend to start testing tomorrow two new wallets:

1. Status dapp browser
2. Crypto.com Defi wallet dapp browser

We have more wallets to test in the backlog that I don't know yet when I'll be able to get to, such as:

• Coinbase
• Portis
• Fortmatic
• Argent
• Trezor
• imToken
• Gnosis Safe
• MyEtherWallet
• MyCrypto
• Rainbow
• Pillar
• 1inch
• Huobi

I'll report back here with any new findings we have

[ricmoo]

Egad. Good point. I will update the initial issue with that additional case; eth_sign “works”, but without the prefix…

This change will be rolled into a minor version bump, so anyone relying on legacy eth_sign behaviour can control it via the package.json.

[ricmoo]

The problem is the v3 typed data signing (i believe?) is incompatible with v4, so basically doubles the size of the typed data library, which is already quite large for a feature that is rarely used. It is probably a good idea to open an issue against TrustWallet to add v4 support though.

If you can provide a concise difference between v3 and v4, I might consider it if the differences are easy to abstract.

Thanks for the list and looking forward to the testing. :)

[alfetopito]

If you can provide a concise difference between v3 and v4, I might consider it if the differences are easy to abstract.

For that, I'll paraphrase a co-worker (from here:

Looking at MetaMask/metamask-extension#6868 it appears that v3 and v4 work the same way, but v4 adds support for nested structs and arrays of complex types.

Which points to the MM change where v4 support is added.

So, yeah, depends on the data being hashed AFAICT.

[alfetopito]

Reporting my findings with next 2 wallets:

Status wallet

I can say that the dApp browser "works" with v4, so eth_sign work around is not needed. Although the work around is in place already.
Actually, doesn't quite work... It hangs on the signature, but that's unrelated to ethers.js

Crypto.com Defi Wallet

It doesn't have a dApp browser as I thought.
Thus the only way is through WalletConnect, which doesn't work at all. I'm able to perform regular transactions without any issues, but no form of signature request work.
There's no prompt at the wallet using _singTypedMessage nor singMessage methods. I can't see into the wallet, so no idea what's going on there, but there's no failure nor anything. It just hangs and never replies.

I assume this has nothing to do with ethers.js either.

[alfetopito]

Two more to the list:

imToken

Flawless both on dApp browser and WalletConnect.
Which means it worked with _signTypedMessage, so I have no data regarding signMessage.

TokenPocket

No issues using dApp browser (same as above, using v4).
With WC, v4 is unsupported. It returns no error to the dApp though. It simply displays a UI message in the mobile app.
With eth_sign, also doesn't work neither reports the error to the dApp, and there's no feedback at all.

[ricmoo]

Is WC WalletConnect? I can try reaching out to them and getting it to at least throw the json-rpc “unsupported method” error.

Did personal_sign work on WC?

[alfetopito]

WC WalletConnect

Ah yes, sorry for the lack of context. Your assumption is correct.

Did personal_sign work on WC?

I've only tried signMessage.

Thinking about a way to easily try that with WalletConnet...

For trying it out with dApp browser, I've been using https://github.com/BboyAkers/js-eth-personal-sign-examples

But that doesn't have WC support.

I could hard code it on ethersjs and copy it over to our dApp, or do it in our exported lib and use yalc to run that locally.

I think the last option might be more practical and the least amount of effort

No more time for that today, unfortunately. Will get back to it tomorrow.

[ricmoo]

No worries. :)

Basically, my ideal situation would be to move to personal_sign, and if that fails (due to not being implemented, being rejected by the user should instantly reject) fall-back onto eth_sign, if that even makes sense. If enough things don’t meaningfully implement eth_sign With prefixes I’d love to drop it altogether.

Thanks for your help with this! :)

[alfetopito]

I see your point and I think it's reasonable.
I just have no idea how many wallets are out there in the wild and how much will be broken if that's done, given the lack of standards in the implementation.

In my case specifically, eth_sign is only the fallback, and I wish wallets would support eth_signTypedData_v4, which provides a better UX anyway.

For instance, I just tested a new wallet which is the counter argument to the proposed change (replace personal_sign with eth_sign).

Fantom wallet (opera)

It works like an injected wallet in the Opera browser.
eth_signTypedData_v4 failed, so I used the fallback to eth_sign and, it worked :)

So, I assume it does not use personal_sign.

I've only tested in our app, since the approach used in https://github.com/BboyAkers/js-eth-personal-sign-examples didn't work.
I guess it does something weird with how the wallet is injected. Did not spend time debugging it.

[PierreJeanjacquot]

Hi, here is the conclusion of my tests with Portis

using [email protected]

eth_sign

• signs prefixed utf8 message (2)

personal_sign

• signs prefixed hex message (passing non hex encoded message will return an error)
• accept both [address, hexEncodedMessage] or [hexEncodedMessage, address] as params

[ricmoo]

This was addressed in 5.5.0 and the solution and legacy work-arounds described in #1542 and #1840.

Thanks! :)