Blur Exchange contest - wait's results

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L40-L46 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168-L210

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;
}

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L216-L224

Vulnerability details

Impact

User may lose remaining ETH of the transaction

Proof of Concept

As the returned status of the call in _returnDust is ignored, the tx will success even if the call failed, which means the caller does not get his ETH back.

Tools Used

n/a

Recommended Mitigation Steps

Revert the whole transaction when the call failed (returns 0).

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L13

Vulnerability details

Impact

Pool has no owner and will be un-upgradeable.

Proof of Concept

Pool does not provide an initialize interface to initialize the owner, so the owner will never be set.

Pool as a UUPSUpgradeable can not be upgraded without a valid owner.

Tools Used

n/a

Recommended Mitigation Steps

Implement the initialize function to set the owner just like Exchange.sol#L112-L118.