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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💥 Version 0x00 of EIP-191 in ECDSA Library Implementation #78

Closed
YamenMerhi opened this issue Feb 19, 2023 · 3 comments · Fixed by #81
Closed

💥 Version 0x00 of EIP-191 in ECDSA Library Implementation #78

YamenMerhi opened this issue Feb 19, 2023 · 3 comments · Fixed by #81
Assignees
Labels
feature 💥 New feature or request
Milestone

Comments

@YamenMerhi
Copy link
Contributor

Describe the desired feature:

🧐 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 to_eth_signed_message_hash(..) function
  • The version 0x01 with the to_typed_data_hash(..) function.
    But we are missing the 0x00 version.

Screen Shot 2022-10-19 at 10 17 27 PM

📝 Details

Version0x00 version is not that used because people are misusing the standard, they are using the 0x45 version for everything: signing messages for off-chain verification, for smart contract execution based on signatures, which is dangerous.

As people could be easily tricked into signing a normal message, thinking it is for login purposes, and then end up having execution based on this signature, so 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.

I am suggesting adding a new function to_data_with_intended_validator or to_data_with_intended_validator_hash to be compatible with the version 0x00 taking 2 parameters, <address validator> and <bytes dataToSign>.

A library that supports the case: EIP-191 Signer.
The Opened issue in OpenZeppelin: Implement 0x00 version of EIP191 in ECDSA Library

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

Code example that solves the feature:

@external
@pure
def to_data_with_intended_validator(validator: address, data_to_sign: Bytes[1024]=b"") -> bytes32:
    """
    @dev Returns an intended validator signed data according to 
    0x00 version of EIP-191.
    @param validator The address validating the signature
    @param struct_hash The data passed to be signed
    @return bytes32 The 32-byte intended validator signed data.
    """
    return keccak256(concat(b"\x19\x00", validator, data_to_sign))
@YamenMerhi YamenMerhi added the feature 💥 New feature or request label Feb 19, 2023
@pcaversaccio pcaversaccio changed the title [Feature-Request]: Implement 0x00 version of EIP191 in ECDSA Library 💥 Implement 0x00 Version of EIP-191 in ECDSA Library Feb 19, 2023
@pcaversaccio pcaversaccio added this to the 0.0.1 milestone Feb 19, 2023
@pcaversaccio
Copy link
Owner

pcaversaccio commented Feb 19, 2023

@YamenMerhi thanks for raising this point - actually I find it a reasonable addition to 🐍 snekmate. I would actually propose the following solution (ignoring the function docstrings):

# @dev A Vyper contract cannot call directly between two `external` functions.
# To bypass this, we can use an interface.
interface IECDSA:
    def to_data_with_intended_validator_hash(validator: address, data: Bytes[1024]) -> bytes32: pure


@external
@view
def to_data_with_intended_validator_hash_self(data: Bytes[1024]) -> bytes32:
    return IECDSA(self).to_data_with_intended_validator_hash(self, data)


@external
@pure
def to_data_with_intended_validator_hash(validator: address, data: Bytes[1024]) -> bytes32:
    return keccak256(concat(b"\x19\x00", convert(validator, bytes20), data))

This includes two functions to_data_with_intended_validator_hash_self and to_data_with_intended_validator_hash, where one uses directly the contract address as validator address. If you agree with this, you can PR that, or we can discuss here about alternative solutions.

PS: I removed the default param =b"" since otherwise, it can change the function selector, which I don't want. https://twitter.com/pcaversaccio/status/1621587151254163462

@YamenMerhi
Copy link
Contributor Author

@pcaversaccio Your code makes sense, will do the PR. 👍

Also, I don't mind the additional function, as in most of the cases of data with the intended validator, it will be used by some sort of a smart contract account that will validate the signature based on self.

@pcaversaccio
Copy link
Owner

Sounds like a plan 👍

@YamenMerhi YamenMerhi changed the title 💥 Implement 0x00 Version of EIP-191 in ECDSA Library 💥 Version 0x00 of EIP-191 in ECDSA Library Implementation Feb 21, 2023
@pcaversaccio pcaversaccio linked a pull request Feb 22, 2023 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 💥 New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants