Personal sign decoding and hashing inconsistency

[pazams]

personal sign decoding and hashing inconsistency

Description

Every personal-sign message in MM starts with the user approving a readable text. Whether it's words, numbers or emojis, it's all just text, which is supposed to make sense somehow to the user approving it. It is therefore expected all these messages would be treated the same when hashed to the personal message format.

Unfortunately, this is not the case. What happens is that number formated messages and mixed characters messages are decoded from utf8 encoding. However, hex formated messages are have an exception, they are decoded with hex encoding.

The hex decoding seems to be coincidental. Otherwise, why wouldn't more text format, e.g. numbers, not be decoded in a special way also?

Why this happens

MetaMask uses this logic to sign personal sign messages: https://github.com/MetaMask/eth-sig-util/blob/0b8495020f0b97bda8c3c9d57d725b499d242d8c/index.js#L250

  personalSign: function (privateKey, msgParams) {
    var message = ethUtil.toBuffer(msgParams.data)
    var msgHash = ethUtil.hashPersonalMessage(message)
    var sig = ethUtil.ecsign(msgHash, privateKey)
    var serialized = ethUtil.bufferToHex(this.concatSig(sig.v, sig.r, sig.s))
    return serialized
  },

That logic works, but ethUtil.toBuffer might behave differently than expected from the caller. That function will always receive a string in this case, but will try to auto-detect hex and then decode it as hex instead of utf8 https://github.com/ethereumjs/ethereumjs-util/blob/v6.0.0/index.js#L157

// ...
      if (exports.isHexString(v)) {
        v = Buffer.from(exports.padToEven(exports.stripHexPrefix(v)), 'hex')
      } else {
        v = Buffer.from(v)
      }
// ...

Summary

The correct behaviour is debatable. I strongly believe hex should not be dealt differently. As the code in geth suggest, https://github.com/ethereum/go-ethereum/blob/0c5f8c078abca7dc5954e30f307495a5c41c5f6c/accounts/accounts.go#L193 the function that hases the message is called TextAndHash, meaning the data is supposed to represent text.

In any case, changing the behaviour now will be too big of a breaking change for apps that rely on this. At this point, we should just accept that hex-appearing texts are dealt differently than any other text format, including numbers.

[dougdyson]

Hello! Is there a workaround for passing a hash as a message to personal_sign without the hash being rehashed by MM? Thanks!