mstpr-brainbot - EIP712 chainId is hardcoded

[sherlock-admin2]

mstpr-brainbot

medium

EIP712 chainId is hardcoded

Summary

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. To avoid this issue, the domain separator should be dynamically calculated using the chainId opcode each time it is requested.

Vulnerability Detail

As stated in the summary, hardcoding the chain ID is not a good practice when implementing EIP712. The best practice is to use a dynamically calculated DOMAIN_SEPARATOR, as suggested by OpenZeppelin.

Here some previous findings in the space regards to this specific issue: https://solodit.xyz/issues/eip712-domain_separator-stored-as-immutable-mixbytes-none-bebop-markdown https://solodit.xyz/issues/m-05-replay-attack-in-case-of-hard-fork-code4rena-golom-golom-contest-git

Impact

Medium, since this issue was previously counted as medium in various protocols and it can be an issue where permit can be used for infinite approvals if the hard fork chain id suits. Also, DODO deployed in various of chains so the thread of this happening is more likely

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSP.sol#L86-L94

Tool used

Manual Review

Recommendation

build the domain separator dynamically with dynamic block.chainId in case of forks of the chain. Smith like this:

function _buildDomainSeparator() private view returns (bytes32) {
        return keccak256(abi.encode(TYPE_HASH, _EIP712NameHash(), _EIP712VersionHash(), block.chainid, address(this)));
    }

[Skyewwww]

We think hardfork has a very low possibility. If a hardfork does happen, we will inform our user to not interact with the hardforked chain. Since calculate the hash dynamically will cost more gas, it It is not worth fixing for V2 but we will fix it in GSP.

[nevillehuang]

Since watsons highlighted a possible scenario where replay attacks can possibly happen which will have significant impact, leaving as medium severity seems appropriate.

[Skyewwww]

We fix this bug in this PR: https://github.com/DODOEX/dodo-gassaving-pool/pull/10

[Czar102]

Planning to make low since it's a valid design choice that the codebase works only on the single chainid it is deployed on. On a fork, it may lose funds. The protocol team is free not to want to implement the fix.

[CergyK]

We fix this bug in this PR: DODOEX/dodo-gassaving-pool#10

I think to fix one should simply not store the DOMAIN_SEPARATOR value in storage but recompute it each time it is used, this has the same problem as the initial version since init() would be called before hardfork as well

[Skyewwww]

We fix this bug in this PR: DODOEX/dodo-gassaving-pool#10

I think to fix one should simply not store the DOMAIN_SEPARATOR value in storage but recompute it each time it is used, this has the same problem as the initial version since init() would be called before hardfork as well

It is a design choice. We hope that it is better not to call buildDomainSeparator() each time calling permit(), because permit is a high-frequency operation called by the user and it will increase the gas. We consider initize DOMAIN_SEPARATOR in init() to prevent it to be an empty value, and we will call buildDomainSeparator() again once the hardfork happens.

[CergyK]

Got it, indeed I missed that buildDomainSeparator() can be called separately and permissionlessly.

Then LGTM