pcaversaccio / snekmate

State-of-the-art, highly opinionated, hyper-optimised, and secure 🐍Vyper smart contract building blocks.
https://snekmate.com
GNU Affero General Public License v3.0
502 stars 56 forks source link

πŸ’₯ Version `0x00` of EIP-191 in ECDSA Library Implementation #78

Closed YamenMerhi closed 1 year ago

YamenMerhi commented 1 year ago

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:

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))
pcaversaccio commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

Sounds like a plan πŸ‘