search

code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

`DOMAIN_SEPARATOR` can allow for double spending #295

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L104-L117

Vulnerability details

Impact

The choice of DOMAIN_SEPARATOR may cause double spending in case a network is forked.

Proof of Concept

The DOMAIN_SEPARATOR is an immutable value for which block.chainid is determined at construction time. But if a fork occurs later and the chain id changes, this change won't impact the DOMAIN_SEPARATOR. Thus, after the fork, an attacker may execute trades another time, despite this is another chain.

Tools Used

Manual analysis

Recommended Mitigation Steps

Add an immutable variable chainId and set it to block.chainid during contract construction. Every time DOMAIN_SEPARATOR is needed, check if chainId == block.chainid and if yes, use the immutable DOMAIN_SEPARATOR, else derive the DOMAIN_SEPARATOR again.

nneverlander commented 2 years ago

Thank you

HardlyDifficult commented 2 years ago

True, the orders would be valid on both chains instead of just mainnet / 1. I do think there's an argument for Medium risk here, but the impact here doesn't sufficiently explore that to make the potential repercussions clear. Lowering risk and merging with the warden's QA report https://github.com/code-423n4/2022-06-infinity-findings/issues/285