Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement 0x00 version of EIP191 in ECDSA Library #3772

Closed
YamenMerhi opened this issue Oct 19, 2022 · 7 comments 路 Fixed by #4063
Closed

Implement 0x00 version of EIP191 in ECDSA Library #3772

YamenMerhi opened this issue Oct 19, 2022 · 7 comments 路 Fixed by #4063

Comments

@YamenMerhi
Copy link
Contributor

馃 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 馃槃

@Amxx
Copy link
Collaborator

Amxx commented Oct 21, 2022

Hello @YamenMerhi

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

@YamenMerhi
Copy link
Contributor Author

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.

@Amxx
Copy link
Collaborator

Amxx commented Oct 24, 2022

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.

@YamenMerhi
Copy link
Contributor Author

@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
Copy link
Contributor Author

Any updates @Amxx @frangio 馃檹

@frangio
Copy link
Contributor

frangio commented Feb 21, 2023

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

@YamenMerhi
Copy link
Contributor Author

Okay will do a PR, thanks! @frangio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants