signMessage doesn't work for bytestring data

[localcryptosMichael]

Problem

There is no way to sign arbitrary bytestring data. It's only possible to sign data that can be encoded as UTF-8. This is a problem for dApps with smart contracts that require wallets to sign arbitrary nonsense such as hashes (and which don't support EIP-712/signTypedData). LocalEthereum is one of those dApps.

The problem lies in the fact the data needs to be encoded as JSON before it's encrypted. Converting to JSON requires that the input message be encoded as UTF-8.

Whenever we convert an arbitrary bytestring to UTF-8 and then back, we lose data; e.g.:

const originalHex = '6e4d387c925a647844623762ab3c4a5b3acd9540';
const encoded = Buffer.from(originalHex, 'hex').toString('utf8');
const decoded = Buffer.from(encoded, 'utf8');
const decodedHex = decoded.toString('hex');
console.log(originalHex === decodedHex); // false

And so, the wallet receives a corrupted message to sign if we do something like:

const bytestring = Buffer.from('6e4d387c925a647844623762ab3c4a5b3acd9540', 'hex');
await walletConnector.signMessage([address, bytestring.toString()]);

What web3.js does

WalletConnect's signMessage([address, data]) is similar to web3.eth.personal.sign(data, address), however only the latter works for bytestrings currently.

To sign a bytestring with web3, you can use web3.eth.personal.sign('0xabcd', address).

The specs of web3.eth.personal.sign say that if the data parameter is a plain string, it will be encoded to hex using web3.utils.utf8ToHex. In other words, data is expected to be a hex-encoded bytestring however the function allows UTF-8 strings for convenience.

In transport, the message is always hex-encoded (i.e. if a dApp asks to sign "hello", the web3 library will ask the wallet to sign "0x68656c6c6f").

Proposal 1

WalletConnect could do the same thing as web3. At the top of signMessage, before the _formatRequest call, it could convert params[1] to a hex-encoded string if it isn't already. On the wallet end, it'll decode the message from hex when it gets the eth_sign call request.

walletconnect-monorepo/packages/core/src/index.ts

Lines 536 to 544 in 9496ace

	public async signMessage (params: any[]) {
	if (!this._connected) {
	throw new Error('Session currently disconnected')
	}
	const request = this._formatRequest({
	method: 'eth_sign',
	params
	})

For example:

if (!isHexStrict(params[1])) {
  params[1] = utf8ToHex(params[1])
}
const request = this._formatRequest({ 

(isHexStrict and utf8ToHex are simple functions in web3.utils.)

Caveats

• This is a breaking change. It'll only work if both the dApp and the wallet app are on the same version.
• The wallet won't know whether the message is supposed to be a UTF-8 string, causing UI problems. It could make a guess, but that's ugly. The same problem exists for web3.personal.sign.

Proposal 2

Implement the change on the wallet side only. When the wallet sees a call request eth_sign with a message beginning with "0x", it'll treat it as a bytestring.

This will (a) let the wallet know when it's signing a UTF-8 string; and (b) only cause a breaking change for WC dApps that sign "0xabcd" strings intended to be displayed as UTF-8.

Proposal 3

Create a new method called something like signByteMessage and a new corresponding call request. This will resolve both of the caveats.

[pedrouid]

Thanks @localethereumMichael for the super detailed description. This makes the issue more clear.
I like the Proposal 3. Let's go with that one.

Can you confirm the following is correct:

  public async signByteMessage (params: any[]) {
    if (!this._connected) {
      throw new Error('Session currently disconnected')
    }

    params[1] = convertUtf8ToHex(params[1])

    const request = this._formatRequest({
      method: 'personal_sign',
      params
    })

    try {
      const result = await this._sendCallRequest(request)
      return result
    } catch (error) {
      throw error
    }
  }

[localcryptosMichael]

Sorry, I've confused myself. When referring to eth.personal.sign, I meant eth.sign. Some wallets have implemented them the wrong way around which threw me off (MetaMask issue here).

WalletConnect's current eth_sign behaves the same as MetaMask's eth_personalSign. i.e. the wallet prefixes the message like:

sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message)))

signByteMessage shouldn't be any different there. It should expect wallets to prefix in the same way.

Because I got the naming wrong, the method personal_sign should be renamed. Maybe something like eth_sign_bytes?

Also, the new method doesn't need to support UTF-8 strings because the dApp developer should use signMessage() if they want to sign a normal string; i.e.:

const sigA = await walletConnect.signMessage([address, "hello"]); 
const sigB = await walletConnect.signByteMessage([address, "0x68656c6c6f"]);

console.log(sigA === sigB); // true
// These are the same because hex("hello") = "0x68656c6c6f"

// Doesn't need to work:
// await walletConnect.signByteMessage([address, "hello"]);

I think the convertUtf8ToHex(params[1]) isn't needed. We can instead throw an error if the message isn't hex encoded.

[localcryptosMichael]

I think this might just be a wallet bug. I didn't realize WC is passing over standard Ethereum JSON-RPC requests.

Wallets should be treating eth_sign's message as a bytestring in the eth_sign payload.
See example in https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign

// Request
curl -X POST --data '{"jsonrpc":"2.0","method":"eth_sign","params":["0x9b2055d370f73ec7d8a03e965129118dc8f5bf83", "0xdeadbeaf"],"id":1}'

I've tested the above in a couple of WalletConnect wallets. To JSON-RPC spec, they should sign 0xdeadbeaf as a bytestring, however they are each signing "0xdeadbeaf" as a UTF-8 string.

Wallets should be doing:

sign(keccak256("\x19Ethereum Signed Message:\n" + len(0xdeadbeaf) + 0xdeadbeaf)))
// same as:
sign(keccak256("\x19Ethereum Signed Message:\n" + "4" + 0xdeadbeaf)))

What they are actually doing:

sign(keccak256("\x19Ethereum Signed Message:\n" + len("0xdeadbeaf") + "0xdeadbeaf")))
// same as:
sign(keccak256("\x19Ethereum Signed Message:\n" + "10" + 0x30786465616462656166)))

[pedrouid]

Ok so I've looked into what you have posted and I think that from the WalletConnect SDK, it was as simple as exposing a new method for personal_sign that will hex encode the message on the JSON-RPC request. It's up to the wallets to currently sign and return a spec-complaint signature.

cc @localethereumMichael

So with the last update beta.6 the signMessage stays the same for eth_sign and the new method signPersonalMessage will handle this issue by sanitizing the message as hex encoded and posting a personal_sign request

  public async signPersonalMessage (params: any[]) {
    if (!this._connected) {
      throw new Error('Session currently disconnected')
    }

    if (!isHexStrict(params[1])) {
      params[1] = convertUtf8ToHex(params[1])
    }

    const request = this._formatRequest({
      method: 'personal_sign',
      params
    })

    try {
      const result = await this._sendCallRequest(request)
      return result
    } catch (error) {
      throw error
    }
  }

[pedrouid]

Fixed on #77