EIP191: Signature validation in solidity

[jMyles]

Hey friends! For the past few months, we at @nucypher have been finding all the right places to insert effort in order to achieve basic signing and verifying of arbitrary payloads across wallets and dapps. Last year, we sponsored a gitcoin bounty in order to close EIPs 191 and 712, which each deal with signing and verifying using wallet keys.

Now that EIP191 has landed, we are looking for the best place to put the verification logic in solidity.

🧐 Motivation

On-chain verification of EIP191 signatures is clearly an essential piece of this puzzle, and we are trying to head off any new boilerplate solidity being sprayed all over every dapp codebase in the world.

📝 Details

My colleagues @szotov and @cygnusv have already done most (perhaps all) of the legwork, here:

https://github.com/nucypher/nucypher/blame/6cdfcf033270371660e4cb5b1dcebd169574e31c/nucypher/blockchain/eth/sol/source/contracts/lib/SignatureVerifier.sol

(Note: This PR is subject to a subsequent rebase and thus might not be available at that URL. In that event, you can see it at this gist: https://gist.github.com/jMyles/9526dee62674718ab97b93793ac45bfa)

So what do you think? Is OpenZeppelin the right place for the canonical version of this logic?

[cygnusv]

Correct link to SignatureVerifier with EIP191 logic:
https://github.com/nucypher/nucypher/blame/4ea10a651ddf5452cd187d3070e5259133aea4ff/nucypher/blockchain/eth/sol/source/contracts/lib/SignatureVerifier.sol

[nventuro]

Hey there Justin, great talking to you again!

EIP191 definitely sounds like something we'd be interested in: we currently have a somewhat barebones ECDSA library to handle address recovery, but while working on projects with different signing needs, we've also come to the conclusion that some standard signing specification is required. Thank you for suggesting this!

Regarding the EIP itself, you mentioned it 'has landed', but the EIP is listed as a draft, and I didn't find any activity related to it since April 2018 - is this happening somewhere else that I'm not seeing?

Apart from the contract code, we should also have tests for such a module. If I recall correctly you mostly work with Python, whereas our test suite is written in JavaScript and truffle. We (or someone from the community) could probably take up the writing of the tests, but if you already have some test cases developed it'd be great if you could share them.

Finally, I noticed your repo is licensed under AGPL-3.0, whereas we use the MIT license. Would you be willing to contribute your work to OpenZeppelin under this license?

Once again, thank you for this!

[cygnusv]

Hi Nicolás,
re: tests, we currently have a small test in Python:
https://github.com/nucypher/nucypher/blame/4ea10a651ddf5452cd187d3070e5259133aea4ff/tests/blockchain/eth/contracts/lib/test_signature_verifier.py#L152
In case you're interested, I'd be glad to test this with your setting (JS + Truffle).

re: license, I don't think that'd be a problem for us.

[frangio]

SignatureVerifier doesn't seem to provide anything specific to EIP 191, does it? It looks like essentially the same functionality that we have in ECDSA.

Since EIP 191 proposes an alternative to personal_sign, which we have encoded as ECDSA.toEthSignedMessageHash, my understanding is that to support EIP 191 in OpenZeppelin we would have to provide an alternative to toEthSignedMessageHash, or one alternative per "version" in EIP 191.

It seems like EIP 712 is the real deal here. I haven't dug into how it relates to EIP 191, but it seems like that's what we should be looking to support mainly. A lot of activity in Ethereum Magicians.

[cygnusv]

Hi @frangio !
There was a mistake in @jMyles' original post. The correct link for SignatureVerifier is this one:
https://github.com/nucypher/nucypher/blame/4ea10a651ddf5452cd187d3070e5259133aea4ff/nucypher/blockchain/eth/sol/source/contracts/lib/SignatureVerifier.sol

In particular, it provides support for versions 0 and E of EIP191.

[frangio]

@cygnusv Cool! That makes sense now. 🙂

We have to decide what API we want for EIP 191 signing (edit:) encoding. My first thought is to add an EIP191 library with one function per signature version, and additionally to deprecate ECDSA.toEthSignedMessageHash in favor of it.

These are some initial names I came up with but I don't necessarily like them. Suggestions will be appreciated.

Unresolved question: should these functions return the encoded bytes or its hash? Users will likely always hash these values afterwards, I think.

library EIP191Encoder {
    // corresponds to eip 191 version 0x45, i.e. eth_personal_sign
    function encodePersonal(bytes memory) internal returns (bytes memory);

    // corresponds to eip 191 version 0x00
    function encodeValidator(bytes memory) internal returns (bytes memory);
}

In the future, EIP 712 could be one additional function here, but it might have to be multiple functions, and in that case it could live in its own library.

---

Note: to be clear this issue is about encoding, and not signing. Signing should continue being handled by our ECDSA library.

[nventuro]

@jMyles @cygnusv given that you've been working in and sponsoring development of EIPs 191 and 712, could you share what the current status of those are? I haven't been able to find relevant recent activity on the EIPs repo, and I'm worried we may be working with an outdated version of the spec.

[cygnusv]

@frangio Those are good questions. It's true that the actual cryptographic signing only cares about what's the hash value to sign, so we here only have to worry about how to produce the hashes with the correct encoding. I agree with you that users only need the resulting hash, either for signing or verification, so I don't see much value in returning the encoded message, prior to hashing.
What we did was a function that returns the hash of the proper encoded message according to a provided version. You can see it here.

So I imagine something very similar to what you describe, but outputting directly the digests:

library EIP191Digest {
    // corresponds to eip 191 version 0x45, i.e. eth_personal_sign
    function digestPersonal(bytes memory) internal returns (bytes32 digest);

    // corresponds to eip 191 version 0x00
    function digestValidator(bytes memory) internal returns (bytes32 digest);
}

@nventuro Although the status of both EIPs is "draft", both Geth and Parity already implement them. We will anyway ask around to see if there can be further advance on their status.

[frangio]

@cygnusv Do you want to open a PR adding this new library?

[3esmit]

My suggestion:

    function toEthSignedMessageHash(bytes32 hash) internal pure returns (bytes32) {
        return toERC191SignedMessage(byte("E"), bytes("thereum Signed Message:\n32"), abi.encodePacked(hash));
    }

    function toERC191SignedMessage(bytes memory data) internal pure returns (bytes32) {
        return toERC191SignedMessage(0x00, data);
    }

    function toERC191SignedMessage(byte version, bytes memory data) internal pure returns (bytes32) {
        return keccak256(abi.encodePacked(byte(0x19), version, data));
    }

    function toERC191SignedMessage(byte version, bytes memory versionData, bytes memory data) internal pure returns (bytes32) {
        return keccak256(abi.encodePacked(byte(0x19), version, versionData, data));
    }

Implemented here
https://github.com/status-im/account-contracts/blob/bcba3cad3677ed733ad70701be2d63c6fb87b9f3/contracts/cryptography/ECDSA.sol#L92-L102

[frangio]

Are we aware of anyone using EIP 191 version 0x00?

I'm tempted to close this issue since we already have an implementation of version 0x45 in ECDSA.toEthSignedMessageHash (although it doesn't exactly belong in ECDSA it's where we put it originally), and EIP712 (version 0x01) is too generic and thus very difficult for us to provide generic hashing/encoding helpers (I am about to push a helper to generate the domain separator, but that's as far as I can see it going).

[frangio]

We now have support for EIP 712 signing, so I'm going to close this issue as resolved. Let us know if there is anything else about EIP191 that we should support.