Blur Exchange contest - philogy's results

Lines of code

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

Vulnerability details

Impact

This vulnerability allows recipients to reenter the Exchange contract, stealing remaining ETH. When a batch of orders are matched using the bulkExecute method a malicious recipient of assets may reenter the bulkExecute method, stealing ETH that would've gone to filling further orders in the batch and / or would've been returned to the original caller. Specifically the following type of recipients may exploit this vulnerability:

• sellers who receive ETH for a sale
• buyers receiving ER721 / ERC1155 tokens

Proof of Concept

The bulkExecute method allows the caller to match multiple orders by passing a list of Execution structs which include both buy and sell side inputs of an order. The bulkExecute method is payable, accepting ETH from the sender which can subsequently be used to fill orders. In bulkExecute, reverts from failing order matches are intentionally contained and skipped, continuing with the matching of the next order.

Sellers who receive ETH as part of an order match (L631) get forwarded all gas, allowing them to reenter the Exchange contract if they're a contract that does so upon its receive method being invoked. Similarly an NFT recipient could reenter upon getting sent a token via the execution delegate (L663), which sends ERC721 tokens via the safeTransferFrom method (L95) triggering a receive check function.

When reentering, a malicious account must call the bulkExecute method with at least 1 wei. Due to the reentrancy guard around the _execute method the list of executions can be left empty as any call to _execute in the attacker's context would silently revert. The setupExecution modifier around the bulkExecute will store the msg.value in remainingETH, which is later checked by the _returnDust method, which is why at least 1 wei must be sent by the attacker. At the end of the bulkExecute method, _returnDust is triggered, sending the contract's entire balance to the attacker, irrespective of the ETH the attacker originally sent.

Returning to the original entry _execute and subsequently bulkExecute call contexts, order matching is resumed whereby every "internal" call to _execute will silently revert due to the isInternal flag having been set to false by the reentrant call.

Tools Used

Manual review.

Recommended Mitigation Steps

To avoid someone triggering the _returnDust method during a running context move up the reentrancy guard from the internal _execute method to the public facing execute and bulkExecute methods.

Lines of code

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

Vulnerability details

Impact

The Pool contract is not initializable and therefore not upgradeable as its owner is never set meaning it always defaults to address(0). Note that the core functionality of the Pool contract works as expected, it simply is not upgradeable.

Issue Description

Looking at the Pool contract it inherits from OpenZeppelin's OwnableUpgradeable& UUPSUpgradeable mixins implementing the _authorizeUpgrade method, implying it should be upgradeable. However it has no initialization method calling the __Ownable_init and __UUPSUpgradeable_init initializers, meaning it can't be upgraded as it doesn't and can't have an owner.

Tools Used

Manual review.

Recommended Mitigation Steps

If upgradeability is not necessary the _authorizeUpgrade method should be removed along with the imports and inheritance from the UUPS and Ownable upgradeable contracts. If upgradeability is desired an appropriate initialization method should be added.