Sign messages in a way that real-world wallets expect

[chmanie]

While testing the MultiSig implementation @JamesLefrere and I encountered that the ecsign method used in createSignatures (in test-helpers.js) works differently from the ones used by web3 or ethers (or any wallet implementation, really). The latter prepend a prefix ("\x19Ethereum Signed Message:\n" + len(message)) to the message hash which is to sign, while the former does not. The prefix can also be found in the ERC191 specification: ethereum/EIPs#191.

In order to add support across all wallets (including MetaMask and Hardware wallets) we should use this method of signing as it's the only one those support.

The proposed change fixes the MultiSig (executeTaskChange) for us when using it in the browser (with an ethers wallet), although it breaks a whole lot of other tests. Maybe we're doing something stupid on the Solidity side of things so we'd like for someone more proficient in this to help us out (@elenadimitrova, @area?).

[elenadimitrova]

What you are proposing uses the geth introduced personal_sign method introduced in late 2016 which ethers enforces when Metamask is used.
This has problems (the new encoding function not being deterministic or injective) explained very well in the "Transactions and bytestrings" section of ethereum/EIPs#712

Many discussions have happened since then and honestly I still don't see there being a consensus although EIP 712 seems close to consensus. Some notes on useful references below for further discussion:

• Effectively the above EIP 712 proposes eth_signTypedData JSON RPC method which seems to converge with EIP 191
• Metamask either already supports this or is close to (I haven't had time to check)
• ethers has had the eth_signTypedData brought up before and it hasn't been rejected Personal API methods ethers-io/ethers.js#98
• web3 - has an open PR that will likely be merged Add personal.signTypedData web3/web3.js#1073
• ganache istanbul chainId opcode returns bad value trufflesuite/ganache#515

[area]

So, EIP712 - once it gets support - is definitely the way to go. Much better from a UX perspective (it's less opaque what one is signing), and hopefully everyone will implement in the same way. Once it's accepted and begins to gain adoption, we can deprecate the implementation here, and force everyone to move over to it.

Until then, I have added the requested prefix to the messages being signed that the contract expects to receive. In order to make things easier, I have hashed the data being passed, then prepending \x19Ethereum Signed Message:\n32" to the hash, and then signing that message. This makes things even more opaque than they were (if a user inspects the message they are signing, it is 32 random bytes, and if they wish to independently verify they are signing what they intend to, they have to do this hashing themselves). However, this makes it a lot easier to verify the signature on-chain, because the length of the message having the prefix added is always 32 bytes. If this was not done, then we would have to generate the ASCII representation of the length on-chain. This would be a string, so variable length, but we wouldn't be able to directly keccack256 it, because the first byte of a variable length string is the string's length, so it would require some further assembly-level jiggery-pokery.

I have also added support for Trezor-style signing, where the message length is not an ASCII-string, but is encoded as a varint (represented by \x20, rather than 32 in the prefix above). This requires an extra parameter to be passed to executeTaskChange for each signature (the _mode parameter) but the correct value for this parameter for each signature can, in principle, be obtained by the client submitting the transaction (by running ECRecover against each possibility, and seeing if the recovered address is the manager or required counterparty).

Yes, this is all a much worse solution than we would have come up with in a vacuum, but I'm pretty sure it's the best we can do for now and maintain compatibility with the wallets we want to support. Roll on EIP712.

[area]

The word on the street from @JamesLefrere is that this solves his problems, so good to go from my perspective.