-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mstpr-brainbot - EIP712 chainId is hardcoded #113
Comments
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. |
Since watsons highlighted a possible scenario where replay attacks can possibly happen which will have significant impact, leaving as medium severity seems appropriate. |
We fix this bug in this PR: https://github.com/DODOEX/dodo-gassaving-pool/pull/10 |
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. |
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. |
Got it, indeed I missed that Then LGTM |
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:
The text was updated successfully, but these errors were encountered: