Blur Exchange contest - Koolex's results

Lines of code

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

Vulnerability details

Impact

The contract Exchange.sol has execute() function which can be called by anyone to execute a single buy and sell order. The function calls _execute() then _returnDust(). The latter sends the unrequired ether back to the caller. However, a malicious actor could steal this ether amount from the caller via a cross-functional re-entercy to bulkExecute().

Proof of Concept

Actors:

• Caller: anyone who calls _execute().
• Seller: someone selling the NFT.
• Buyer: someone buying the NFT.
• Fee Recipients: one or more who are receving the fee (provided by the seller).

A scenario as an example: In the following scenario, the buyer could possibly be the bad actor (in each step I added the value of the state variable remainingETH which is updated by setupExecution modifier):

1. The caller calls execute() with two matched order that has a POOL payment or WETH, and a msg.value = 1 ether.
   • remainingETH = 1 ether
2. execute() calls _execute() .
3. _execute() is being executed till it reaches _executeTokenTransfer() which transfers the token to the buyer.
   • remainingETH = 1 ether
4. The buyer is a contract which has onERC721Received() that calls bulkExecute() with empty executions array and a msg.value > 0. Let's assume it is 0.1 ether.
   • remainingETH = 0.1 ether
5. The bulkExecute() will pass the for-loop and calls _returnDust() .
6. _returnDust() checks the remainingETH variable is greater than zero (which is 0.1 ether) and sends all the contract's balance to the msg.sender.
7. At this moment, the msg.sender is the buyer and the contract's balance is 1.1 ether.
8. The buyer receives 1.1 ether and remainingETH variable is set to zero via the setupExecution modifier.
   • remainingETH = 0
9. Now back to the execute() which calls _returnDust() after _execute() is done.
10. _returnDust() checks the remainingETH variable is actually zero so it doesn't send anything to the caller.

Note: a similiar scenario could be done by any Fee Recipient by re-entering bulkExecute() from the receive callback when the payment is ether (paymentToken == address(0))

Tools Used

Manual analysis

Recommended Mitigation Steps

Add the reentrancyGuard modifier to bulkExecute() function.

function bulkExecute(Execution[] calldata executions)
        external
        payable
        whenOpen
        setupExecution
	reentrancyGuard
    {
				
	  }

Hint: It is not recommended to perform a state change in modifiers (which is done for example in setupExecution) as it violates the Checks-Effects-Interactions pattern. Please consider moving the logic in setupExecution to a regular function.