add status.im condition for eth_sign

[flexsurfer]

Add status.im condition for eth_sign. Status.im doesn't support eth_sign for security reason

[ricmoo]

But it does support personal_sign?

What is the security concern that eth_sign has that personal_sign doesn’t? (Just curious)

Also, do they both throw in the event it is unsupported? Ideally I would like to try one and if an "unsupported error" is returned try the other...

[ricmoo]

Oh! Also, if you know someone on status.im, can you tag them here, so they get notified. :)

[flexsurfer]

What is the security concern that eth_sign has that personal_sign doesn’t? (Just curious)

https://medium.com/metamask/the-new-secure-way-to-sign-data-in-your-browser-6af9dd2a1527

Oh! Also, if you know someone on status.im, can you tag them here, so they get notified. :)

yes sure, @flexsurfer ;)

[ricmoo]

yes sure, @flexsurfer ;)

LOL! Perfect, I was hoping so. ;)

If the only difference is the prefixing, then that is also what eth_sign should be doing too, but I fully agree I would prefer any account-based operation move out of the eth_ namespace.

What happens in status if eth_sign is called? I need to see what metamask does these days... What I would ideally like is to be able to use:

return this.send("eth_sign", blah).then((result) => {
    processResult(result);
}, (error) => {
    if (hasSomeObviousWayToDetectUnsupportedOperation(error)) {
        this.send("personal_signMessage", blah).then((result) => {
            processResult(result);
        });
    }
});

For example, will the JSON-RPC response with error code -32601?

I need to try out the most popular wallets to make sure this works.

For now, I can add logic similar to what you've proposed in this PR, but am looking for something more scalable, especially with v6 on its way.

Thanks! :)

[ricmoo]

Fixed in 5.0.30. Try it out and let me know if there are any issues. :)

[kenshyx]

Hey guys, I think that the signer.signMessage should always use personal_sign for consistency instead of eth_sign. I've tested the current api with the WalletConnect provider from a couple of mobile wallets(trust, rainbow, argent, imToken, metamask, walleth) and also from https://test.walletconnect.org/. Most of them return a bad address signature recovery, only walleth and imToken returned the correct address on utils.verifyMessage. With personal_sign all of the signature recoveries were correct, it also enables human readable tx :)

[kenshyx]

@ricmoo any thoughts on this?

[ricmoo]

@kenshyx I would need to investigate further. I am also curious why those other clients return wrong hashes; are they using the legacy implementation of eth_sign, in which does not EIP-191 prefix the message? If so, then there are massive security holes in those systems.

If you'd like to open new issue with your findings, that'd be awesome. Could you include a table of each client you tried, the message you signed and the result of eth_sign and personal_sign?

I would ideally like a way to detect whether a client supports personal_sign, but the problem is historically some clients just hang and never respond. If personal_sign is a hard fail, and I can fall back onto eth_sign, that is fine. But we really need a good list of what clients support which, and in the event they don't support it whether how they fail (return an error vs hang indefinitely).

[ricmoo]

(also, make sure the discrepancy isn't a bug in the WalletConnect library you are using :))

[tetratorus]

@ricmoo I know this is closed, but this is an issue for non-metamask/non-status wallets (like https://github.com/torusresearch/torus-embed), I understand that there's no reliable way to find out if a provider supports personal_sign so it's hard to make a decision on what the default should be, but in the meantime perhaps we could add a way to override this behavior somewhere, since that would help make all these wallets compatible.

[pcowgill]

@ricmoo We're also running into this issue as it relates to WalletConnect, moving from our dapp just supporting MetaMask to supporting WalletConnect too, which I think is a very common use case.

This earlier comment in this thread summarizes our situation well.

What about a new optional method that always uses personal_sign, that could have an underscore prefix for now to indicate it may change?

[ricmoo]

I am still very open to making this a wider change. I need the following from people with access to larger sets of environments. A list with:

• each platform/environment/backend
• What happens if eth_sign is used; does it silently swallow it? Does it throw an error? What is the content of the error? What error is received if the user clicks reject?
• What happens if personal_sign is used; does it silently swallow it? Does it throw an error? What is the content of the error? What error is received if the user clicks reject?
• Does any error message collide or look confusingly similar to other errors emitted for other reasons?

Basically, I want a safe way to detect which to use and when to fallback. As a prime example, it cannot just use the logic "if you fail, fallback onto the other", since if a person is asked to eth_sign and a popup occurs, to which they reject, that is a legitimate fail that should end the workflow, the personal_sign should not be fallen back on. But I need to have some idea on what various clients currently do... :)

Ideally I'd love to make this process "just work". And maybe now its safe to move to personal_sign, but last time I looked into it, everything was all over the map with no ability to inspect... :s

[alfetopito]

I'm very interested in this subject as I'm currently facing issues with TrustWallet dApp browser.

For a little context, our dApps default is to use eth_signTypedData_v4 which is not supported by some wallets: gnosis/cowswap#563

An attempt to fix is to fallback to eth_sign gnosis/cowswap#576

But, finally found out that eth_sign has the problem reported in this issue, just like status.im and Metamask.

The problem I see writing a solution to rule them all ™️ as mentioned, is that (in TrustWallet's case at least) eth_sign is present and it does sign the message without errors, but the result is garbage.
So we'd have to resort to hard coding like MM and status.

This is a snipped from a co-worker who found out about the eth_sign issue: (disregard the data contents as most isn't relevant, what matters is the receiver value which is the address that signs the message)

import { OrderKind, SigningScheme, decodeSignatureOwner } from "./src/ts";
const domain = {
  name: "Gnosis Protocol",
  version: "v2",
  chainId: 1,
  verifyingContract: "0x3328f5f2cEcAF00a2443082B657CedEAf70bfAEf",
};
const order = {
  sellToken: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2",
  buyToken: "0x6B175474E89094C44Da98b954EedeAC495271d0F",
  sellAmount: "98800574115482334",
  buyAmount: "329621405635476313512",
  validTo: 1620081463,
  appData: "0x0000000000000000000000000000000000000000000000000000000000000000",
  feeAmount: "1199425884517666",
  kind: OrderKind.SELL,
  receiver: "0x4CBc3B6B16Ac4218169078aDd37947B76127E10B",
  partiallyFillable: false,
};
const signature =
  "0x3c1c130149a870dc481065ea37af5514625f9d8265926f9a7657f0f95ad2e279328f2722fc11f1cae11a2fb5748f94c8463e9c3f15a4d065dcb188eb17b9a6a71c";
console.log(`Owner: ${order.receiver}`);
for (const signingScheme of [SigningScheme.EIP712, SigningScheme.ETHSIGN]) {
  const recoveredOwner = decodeSignatureOwner(
    domain,
    order,
    signingScheme,
    signature,
  );
  console.log(`Recovered ${SigningScheme[signingScheme]}: ${recoveredOwner}`);
}

Output:

Owner: 0x4CBc3B6B16Ac4218169078aDd37947B76127E10B
Recovered EIP712: 0x4CBc3B6B16Ac4218169078aDd37947B76127E10B
Recovered ETHSIGN: 0xAa047c8Aa547f75198a7936823EbcB240f4CA1c7

[ricmoo]

Please see #1544.

Add any environments you have access to. :)