Change ERC1271 to bytes32 hash

[frozeman]

This changes the proposal to use a bytes32 hash for the signature to prevent the requirement of specific implementation checks within this signature verification functions.

Those can be either done off-chain, or in other smart contract functions, and funnel those to this function to verify the signature.

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

• The PR edits only existing draft PRs.
• The build passes.
• Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
• If matching on email address, the email address is the one publicly listed on your GitHub profile.

[eip-automerger]

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

• EIP 1271 requires approval from one of (@izqui, @PhABC, @abandeali1, @catageek, @shrugs, @frangio)

[pedrouid]

LGTM 👍

[PhABC]

Comment should be changed to bytes32,bytes

[PhABC]

LGTM otherwise!

[frozeman]

fixed

[3esmit]

I think this change is bad. I have use-case where I need to pass the whole data to the contract, not just it hash, because the data defines if the signature can be validated or not. E.g. Multisig thresholds for different actions.

This change seems limiting, and would make things difficult if a contract wants to have special checks on the data type before authorizing, requiring extra call from the signer to the contract to validate the hash, to then be accepted in ERC1271.

While with data as bytes, the contract could do necessary checks against signer.

[3esmit]

Just to clarify... This change would make this not possible:

contract ACLAccount is Controlled, BytesSigner {

    mapping(bytes4 => address) public acl;

    //(...) other external an public functions

    function isValidSignature(
        bytes memory _data,
        bytes memory _signature
    )
        public
        view
        returns (bytes4 magicValue)
    {
        (bytes4 sig, ) = abi.decode(_data, [bytes4, bytes]);
        address signer = acl[sig];
        if(signer == 0){
            signer = controller;
        }
        if(isContract(signer)){
            return BytesSigner(signer).isValidSignature(_data, _signature);
        } else {
            return signer == ECDSA.recover(ECDSA.toERC191SignedMessage(address(this), _data), _signature) ? MAGICVALUE : bytes4(0xffffffff);
        }
    }
}

After the change, this would be the direct way of solving the ACL for signatures:

contract ACLAccount is Controlled, Bytes32Signer {

    mapping(bytes4 => address) public acl;
    mapping(bytes32 => address) signer;
    //(...) other external an public functions

    function selectSigner(bytes memory _data) external {
        (bytes4 sig, ) = abi.decode(data, [bytes4, bytes]);
        signer[keccak256(_data)] = acl[sig];
    }

    function isValidSignature(
        bytes32 _dataHash,
        bytes memory _signature
    )
        public
        view
        returns (bytes4 magicValue)
    {
        address signer = signer[_dataHash];
        if(signer == 0){
            signer = controller;
        }
        if(isContract(signer)){
            return Bytes32Signer(signer).isValidSignature(_dataHash, _signature);
        } else {
            return signer == ECDSA.recover(_dataHash, _signature) ? MAGICVALUE : bytes4(0xffffffff);
        }
    }
}

It would also to be possible to include the data in the "signature field" for the ACLed Bytes32Signer, and then pass only the signature to the authorized signer, however this would suggest the rename of "_signature" field to "_proof", such as in the example below:

contract ACLAccount is Controlled, Bytes32Signer {

    mapping(bytes4 => address) public acl;

    //(...) other external an public functions

    function isValidSignature(
        bytes32 _dataHash,
        bytes memory _proof
    )
        public
        view
        returns (bytes4 magicValue)
    {
        (bytes memory signature, bytes memory data) = abi.decode(_proof, [bytes, bytes]);
        require(keccak256(data) == _dataHash, "Bad signature");
        (bytes4 sig, ) = abi.decode(data, [bytes4, bytes]);
        address signer = acl[sig];
        if(signer == 0){
            signer = controller;
        }
        if(isContract(signer)){
            return Bytes32Signer(signer).isValidSignature(_dataHash, signature);
        } else {
            return signer == ECDSA.recover(_dataHash, signature) ? MAGICVALUE : bytes4(0xffffffff);
        }
    }
}

[pedrouid]

We need to define what is the goal of this ERC because we won't be able to fulfill all use-cases in a single method

However IMO this ERC should serve for Dapps to autonomously support any smart contract wallet without prior knowledge of its interface.

Additionally using bytes makes the assumption that its eth_sign and removes support for EIP-712 signatures

If some use-cases require more validation prior to hashing then IMO they should use a different method to support it.

Currently as part of WalletConnect development I'm faced with a lot of reports that Smart Wallet support is lacking and I have to explain to users and Dapps that they must support non-standard validation for each one of the wallets and on top of that they can't use EIP-712 signatures

If we use bytes32 hashes for validation then any smart wallet can follow the standard and any dapp can immediately validate both eth_sign and EIP-712 signatures for any smart wallet without prior knowledge

I hope this demonstrates how much more valuable is to use bytes32 hash over arbitrary byes data

[pedrouid]

PS - also we should aim ERC-1271 to provide the same level of functionality as ecrecover does off-chain... introducing additional validation seems to be out-of-scope of this standard IMO

[frozeman]

@3esmit i think your example is a good way to circumvent adding bytes to ERC1271.
Any extra validation scheme should be standardised separately. I agree with @pedrouid here.

[frozeman]

@PhABC you need to approve for it to auto merge.

[PhABC]

Agreed as well. Supporting bytes would require more consideration on how the bytes array passed must be structured, which is complex and would be better served by a separate ERC.