signMessage doesn't work for bytestring data about walletconnect-monorepo

[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