Lines of code
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168-L210&gt;

Vulnerability details

Impact

The setupExecution can be re-entered by calling bulkExecute inside an _execution.

Because the global state remainingETH and isInternal are modified (reset to 0 and flase) after the latter reentrant call, the previous call is affected:

1. all subsequent executions will fail, because isInternal is reset to false
2. the origin tx sender will lose ETH, because remainingETH is reset to 0

At the same time, the ETH lost is taken away by _returnDust() in the re-entered calling if it carries msg.value greater than 0.

Proof of Concept

Use a custom contract which will re-enter Exchange by calling bulkExecute.

The logic of the custom contract can be invoked by the execution of a malicious constructed order in
Exchange.sol#L631
or Exchange.sol#L663
or Exchange.sol#L665.

Tools Used

n/a

Recommended Mitigation Steps

Adding require(!isInternal) to setupExecution will avoid this problem:

modifier setupExecution() {
    require(!isInternal);    // added
    remainingETH = msg.value;
    isInternal = true;
    _;
    remainingETH = 0;
    isInternal = false;
}

The text was updated successfully, but these errors were encountered:

All reactions