[hebao] ETH_SIGN signature refine

[kongliangzhong]

    function verifySignature(
        bytes32 signHash,
        address signer,
        bytes   memory signature
        )
        internal
        view
        returns (bool)
    {
        uint signatureTypeOffset = signature.length.sub(1);
        SignatureType signatureType = SignatureType(signature.toUint8(signatureTypeOffset));

        bytes memory stripped = signature.slice(0, signatureTypeOffset);

        if (signatureType == SignatureType.WALLET) {
            return verifyERC1271Signature(signHash, signer, stripped);
        } else if (signatureType == SignatureType.EIP_712) {
            return recoverECDSASigner(signHash, stripped) == signer;
        } else if (signatureType == SignatureType.ETH_SIGN) {
            bytes32 hash = keccak256(
                abi.encodePacked("\x19Ethereum Signed Message:\n32", signHash)
            );
            return recoverECDSASigner(hash, stripped) == signer;
        } else {
            return false;
        }
    }

As shown above, when signatureType == ETH_SIGN, the signHash is the hash of origin message,
In this way, there will be some warnings in metamask, and a user can not see the source text when
they do the signing.

I think we should change the flowing code:

        } else if (signatureType == SignatureType.ETH_SIGN) {
            bytes32 hash = keccak256(
                abi.encodePacked("\x19Ethereum Signed Message:\n32", signHash)
            );
            return recoverECDSASigner(hash, stripped) == signer;
        }

to

return recoverECDSASigner(signHash, stripped) == signer;

[dong77]

Why not just use SignatureType.EIP_712 as the type value?

[Brechtpd]

But the \x19Ethereum Signed Message:\n32 is added automatically when signing the data right? So if it's not added here where would you add the string?

and a user can not see the source text when they do the signing.

ETH_SIGN signs raw data without any context, so MetaMask probably does not know how to show this data to users. So it works for text I guess, but other data types I assume just show up mostly unreadable anyway (not sure)? Because we're doing hash(1234) and not hash("1234") for numbers for example, both values are different things. Users may not sign a hash, but otherwise I think they will sign some byte string they probably cannot read either. EIP_712 solves exactly this.

[kongliangzhong]

As mentioned in ERC191:

"\x19Ethereum Signed Message:\n" + len(message).

we use 32 instead of len(message), this indicates that the user must pass in a hash string. When this hash string is signed in metamask, there will be a warning, and the user can not know what he is signing, compared to a simple source text "1234".

[Brechtpd]

But the problem is that we don't hash the text data onchain, we sign actual data. So if you hash 1234 you hash the data 0x04D2. If you hash the string "1234" you hash the data 0x31323334 (if encoded with ascii). So if we want to hash something readable we'd have to convert everything to strings first onchain before hashing the data, no?

And even in the best case it would be just a long string of data without any real context of what this data means (unless that's also added in some way, but then you're basically recreating EIP712 in a worse way). The MetaMask warning even links to this which in turn links to the recommendation to switch to EIP712. I don't think there's any way to make ethsign work any better, which was the main motivation for EIP712.

[kongliangzhong]

@Brechtpd, I discussed with @dong77, and have reached a consensus, If the user doesn't want the contract to add the Prefix for them, then he can just append the EIP712 type to the signature.
In this way, the verifySignature function worked out nicely.

[kongliangzhong]

Luckily we don't need to change our contract code, this issue is closed.