Immutable `DOMAIN_SEPARATOR` will become incorrect if any chain protocol deploys on hard forks

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/EIP712.sol#L11-L23

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/EIP712.sol#L11-L23

    bytes32 public immutable DOMAIN_SEPARATOR;

    constructor(string memory name, string memory version) {
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(
                TYPE_HASH,
                keccak256(bytes(name)),
                keccak256(bytes(version)),
                block.chainid,
                address(this)
            )
        );
    }

We can see that this is the implementation of how the DOMAIN_SEPARATOR is being stored/cached, i.e it's set as an immutable variable and the chain.Id gets queried only once, issue here however is that the protocol doesn't take into account that the chain to which the protocol would be deployed could undergo a hardfork, which would then make the block.chainId attached to DOMAIN_SEPARATOR to now be stale.

Note that from the readMe the below has been stated https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/README.md#L101

| Chains the protocol will be deployed on | Ethereum,Arbitrum,Base,BSC,Optimism,Polygon |

Which heavily increases the likelihood of this occurring since we are not looking at a hard fork happening on the Ethereum chain only, but rather all supported chains.

Now going to the implemented EIP 712 definition of the DOMAIN_SEPARATOR , we can see that this has been hinted on the block.chainId https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator:

uint256 chainId the EIP-155 chain id. The user-agent should refuse signing if it does not match the currently active chain.

Note that EIP-155 has been out for a while and created around ~8 years ago, and it preaches that the chainIds used should allow replay attack protections.

Which then leads to the core issue on Krystal Defi's implementation being that if any of the chain which protocol is to be deployed on ever hard forks, signature replay becomes possible across the original and forked chain. This is because the immutable DOMAIN_SEPARATOR (which contains block.chainid) is pre-computed and will remain the same on the forked chain, even though its chain ID is different.

Impact

As hinted under Proof of Concept, if a chain hard forks, signature replay becomes possible across the original and forked chain.

Recommended Mitigation Steps

Consider checking if block.chainid matches the chainId cached in the DOMAIN_SEPARATOR before using it.

A pseudo fix, could be to introduce like a helper function to calculate the domain separator and this would then check if the DOMAIN_SEPERATOR has changed, i.e something like:

..
+    uint256 CURRENT_DEPLOYED_CHAIN_ID;

-    constructor(string memory name, string memory version) {
+    constructor(string memory name, string memory version, uint256 chainId) {

+        CURRENT_DEPLOYED_CHAIN_ID = chainId; // here you attach the current chainId of where the project is being deployed on
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(
                TYPE_HASH,
                keccak256(bytes(name)),
                keccak256(bytes(version)),
                block.chainid,
                address(this)
            )
        );
    }

+    function calculateDomainSeparator() internal view returns (bytes32) {
+       if (block.chainid == CURRENT_DEPLOYED_CHAIN_ID) {
+           return DOMAIN_SEPARATOR;
+       } else {
+           return keccak256(
+                abi.encode(
+                    TYPE_HASH,
+                    keccak256(bytes(name)),
+                    keccak256(bytes(version)),
+                    block.chainid,
+                    address(this)
+                )
+            );
+       }
+    }

And then query this function while hashing the typed Data.

Assessed type

Context

[3docSec]

The report is of good quality and clearly explains the issue

[c4-judge]

3docSec marked the issue as selected for report

[c4-judge]

3docSec marked the issue as satisfactory

[3docSec]

Note: this is L-4 from the bot report

[3docSec]

OOS; was reported by 4naly3er

[c4-judge]

3docSec marked the issue as unsatisfactory: Out of scope