hats-finance / Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0

Proof of Humanity Protocol v2
2 stars 1 forks source link

DomainSeperator is missing version #23

Open hats-bug-reporter[bot] opened 2 weeks ago

hats-bug-reporter[bot] commented 2 weeks ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xd72df861654411b69ab064b7335ea688c5f3bc8aeee0be19d9b82a403a35cf63 Severity: low

Description: Description\ TypeHash of the domainSeprator is missing version which violating the EIP712 - https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator

// EIP-712.
bytes32 DOMAIN_TYPEHASH = 0x8cad95687ba82c2ce50e74f7b754645e5117c3a5bec8151c0726d5857980a866; // keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)").
DOMAIN_SEPARATOR = keccak256(
    abi.encode(DOMAIN_TYPEHASH, keccak256("Proof of Humanity"), block.chainid, address(this))
);

This is also the way it is implemented in OZ contracts - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1edc2ae004974ebf053f4eba26b45469937b9381/contracts/utils/cryptography/EIP712.sol#L89

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File
  2. Revised Code File (Optional)

    Add version to the TypeHash and the has itself.

    DOMAIN_SEPARATOR = keccak256(
        abi.encode(DOMAIN_TYPEHASH, keccak256("2"), keccak256("Proof of Humanity"), block.chainid, address(this))
    );
clesaege commented 2 weeks ago

The version serves at making signatures between different versions incompatible. Here the desired behaviour is the opposite (vouches of v1 should still be valid for v2).

As per EIP-712 Protocol designers only need to include the fields that make sense for their signing domain. Unused fields are left out of the struct type.

Here a version field doesn't make sense (vouches of v1 should still be valid) so the version field was left out of the struct type per EIP-712 recommendation.