EIP712 chainId is hardcoded which can cause replay attacks in case of hardfork

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xf58a0ff8068692d31c05b7abee5e73d34a1c04d266e5f39862fd5188e7b48d09 Severity: medium

Description: Description:

Pair.permit() function has implemented EIP 712-signed approvals through a permit() function. A domain separator and the chainID are included in the signature schema. However, this chainID is hardcoded as a part of DOMAIN_SEPARATOR and it is not dynamically fetched.

    function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external {
        require(deadline >= block.timestamp, "Pair: EXPIRED");
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
                keccak256(bytes(name)),
                keccak256(bytes("1")),
@>              block.chainid,
                address(this)
            )
        );
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
                DOMAIN_SEPARATOR,
                keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonces[owner]++, deadline))
            )
        );
        address recoveredAddress = ecrecover(digest, v, r, s);
        require(recoveredAddress != address(0) && recoveredAddress == owner, "Pair: INVALID_SIGNATURE");
        allowance[owner][spender] = value;

        emit Approval(owner, spender, value);
    }

Per EIP712, calculating the domain separator using a hardcoded chainId could pose problems. The reason is that if the chain undergoes a hard fork and changes its chain id, the domain separator will be inaccurately calculated. In addition, per current implementation DOMAIN_SEPARATOR is immutable which it should not be.

Impact

After the chain hardfork then DOMAIN_SEPARATOR value will become invalid. This is because the chainId parameter is computed in permit() function where chainId is hardcoded. This means even after hard fork chainId would remain same which is incorrect and could cause possible replay attacks. The chainId is hardcoded in DOMAIN_SEPARATOR which means even after hard fork, DOMAIN_SEPARATOR value will remain same and point to incorrect chainId.

Recommendation

It is recommended to adhere to the best practices of the EIP712 implementation. To avoid this issue, the domain separator should be dynamically calculated using the chainId opcode each time it is requested.

I highly recommend to check openzeppelin's EIP712.sol for correct implementation here

References

https://solodit.xyz/issues/m-05-replay-attack-in-case-of-hard-fork-code4rena-golom-golom-contest-git

https://solodit.xyz/issues/lack-of-chainid-validation-allows-reuse-of-signatures-across-forks-trailofbits-advanced-blockchain-pdf

[0xRizwan]

Reference from Fenix ERC721Permit.sol which is out of scope contract for this audit. Here, domain separator is implemented correctly and it is dynamically fetched. Please check here

[BohdanHrytsak]

Thank you for the submission.

In the view, you indicate that the DOMAIN_SEPARATOR with the chainid is hardcoded, but in fact, with each call, the DOMAIN_SEPARATOR will be updated to the current one, and in case of a hardfork, it will be calculated with the current chainid accordingly

In the recommendations, you indicate a mechanism for applying the cached DOMAIN_SEPARATOR in case the chainid has not changed and calculating a new one when changing it. In our case, we always calculate the current one without caching

Also, this submission is OOS, as it is inherited from Chronos & Thena