openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

Implement `0x00` version of EIP191 in ECDSA Library

Open YamenMerhi opened this issue 3 years ago • 5 comments

🧐 Motivation

In the current ECDSA library, there is support to construct a message to sign according to the EIP191 Standard with the following:

  • The version 0x45 (E) with toEthSignedMessage(..) function
  • The version 0x01 with the toTypedDataHash(..) function. But we are missing the 0x00 version.
Screen Shot 2022-10-19 at 10 17 27 PM

📝 Details

After looking at this closed issue, saw a comment from @frangio stating that 0x00 version is not that used, which is true because people are misusing the standard, they are using the 0x45 version for everything: signing message for offchain verification, for smart contract execution based on signatures, which is dangerous.

As people could be easily tricked to sign a normal message, we should have a different mechanism, then different handling for execution based on signatures, and that's why we should make people use the 0x00 version for this case. + some projects are starting to use it like xenium and the lsp-smart-contracts (planning to use it).

I am suggesting to add a new function toDataWithIntendedValidator(...) to be compatible with the version 0x00 taking 2 parameters, <address validator> and <bytes dataToSign>.

The function would be:

function toDataWithIntendedValidator(address validator, bytes memory dataToSign) internal pure returns (bytes32) {
        return keccak256(abi.encodePacked("\x19\x00", validator, dataToSign));
}

If you're okay with it I am happy to open the PR, otherwise you can close the issue 😄

YamenMerhi avatar Oct 19 '22 19:10 YamenMerhi

Hello @YamenMerhi

That is an interesting proposal. Do you know any wallet that include support for version 0x00 signing ?

Amxx avatar Oct 21 '22 09:10 Amxx

Hey @Amxx

As I stated in my message, we are planning to use it in the LSP6KeyManager contract, also when I met nick.eth in DevCon he mentioned that he uses it for xenium. Also when I checked argent, their old multisig use it with a weird combination of 0x45 version.

With the rise of smart contract-based accounts and wallets, I think it will start to be used more and more, and we should push to have it as anyone could be tricked to sign a 0x45 message and end up having a smart contract execution based on this signature where people thinks its just for offchain or SIWE purposes.

I made a PR to the EIP191 standard to make it more comprehensible and the LUKSO team is also developing a library EIP191-Signer that allows signing different versions of EIP191 messages (0x45 and 0x00). Hopefully these steps and tools will make 0x00 version more adopted.

YamenMerhi avatar Oct 21 '22 15:10 YamenMerhi

Thanks for all this information!

I was only saying that because many devs still make the error of not using the "preparation" function, and not understanding the difference between rawsign and personalsign. I hope adding this new function doesn't add to the confusion.

Also, I want us to test it, and for that we'll need to produce the corresponding signature, hopefully without using some low level rawsign.

Amxx avatar Oct 24 '22 12:10 Amxx

@Amxx Definitely we want to avoid confusion as much as possible. If you want to create a list of things to test, change, do please do, and let's sync on it 😀

YamenMerhi avatar Oct 25 '22 12:10 YamenMerhi

Any updates @Amxx @frangio 🙏

YamenMerhi avatar Jan 02 '23 13:01 YamenMerhi

Sorry for the silence. This is simple enough, I'm not opposed to a PR implementing it.

frangio avatar Feb 21 '23 15:02 frangio

Okay will do a PR, thanks! @frangio

YamenMerhi avatar Feb 21 '23 15:02 YamenMerhi