MetaMask / MetaMask/eth-sig-util

Personal sign decoding and hashing inconsistency

Open
#60 1 comment 2 reactions 0 assignees View on GitHub
bug
Dominant language
TypeScript
Stars
583
Forks
228
Avg merge
11h 22m
Merged PRs (30d)
7

Description

# 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
```js
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
```js
// ...
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.

Contributor guide

No contributing guide indexed for this repository

Research direction

The issue points to personalSign in eth-sig-util and ethUtil.toBuffer in ethereumjs-util; start by checking those referenced implementations and comparing hashing inputs for hex-looking, numeric, and mixed text. Before changing behavior, establish the intended compatibility outcome and add tests that make the chosen treatment and its breaking-change implications explicit.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
cryptography
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.