Blur Exchange contest - hihen'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#L154-L158 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168-L172 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L212-L227

Vulnerability details

Impact

The funds (ETH) that users sent along with the bulkExecute may be stolen.

Proof of Concept

When a buyer send a bulkExecute() tx with msg.value > 0 (with order of buying token with eth), the sent ETH may be stolen if the tx contains a malicious selling order which can trigger a reentry attack.

Here are the details of the reentry attack:

1. Buyer B calls bulkExecute() with a list of executions(order pairs) and some ETH. Assume v1 = remainingETH = msg.value, we have balanceOf(Exchange) >= v1.
2. When _execute() one of the executing which is malicious, triggle a call to a deployed malicious contract X. We can know that v2 = remainingETH >= v1.
3. The malicious contract X will call bulkExecute() of the Exchange with at least 1 wei and an empty list of executions. And this call with send all (balanceOf(Exchange) >= v2 + 1wei) ETH to contract X by _returnDust().
4. When X returns, we will have remainingETH == 0 and balanceOf(Exchange) ==0 now.

The invocation of a malicious contract in step 2 above may occur under the following circumstances and may be considered constructed maliciously

1. Contains a sell order whose collection contract is malicious: the logic in step 3 can be executed in _executeTokenTransfer().
2. Contains a sell order with malicious fee recipient: the reentry attack happens when fee is sent to the malicious recipient who is the malicious contract X.

In addition, when the Buyer calls execute() with a msg.value exceeds the actual price, the buyer may also lose the remaining funds by a reentry attack.

Tools Used

VS Code

Recommended Mitigation Steps

This problem is mainly due to the fact that _execute() is reentrancy protected and setupExecution() is not.

Recommended: remove reentrancyGuard from _execute() and add it to external functions (execute() and bulkExecute()) to make setupExecution reentrancy protected.

Here is the diff:

diff --git a/contracts/Exchange.sol b/contracts/Exchange.sol
index ec27b1d..f3fa09b 100644
--- a/contracts/Exchange.sol
+++ b/contracts/Exchange.sol
@@ -155,6 +155,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         external
         payable
         whenOpen
+        reentrancyGuard
         setupExecution
     {
         _execute(sell, buy);
@@ -169,6 +170,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         external
         payable
         whenOpen
+        reentrancyGuard
         setupExecution
     {
         /*
@@ -234,7 +236,6 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
     function _execute(Input calldata sell, Input calldata buy)
         public
         payable
-        reentrancyGuard
         internalCall
     {
         require(sell.order.side == Side.Sell);

Lines of code

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

Vulnerability details

Impact

The Pool (proxy) contract will never be able to upgrade successfully

Proof of Concept

The Pool contract is defined as OwnableUpgradeable and UUPSUpgradeable, and it should be upgradeable just like the Exchange contract.

UUPSUpgradeable will invoke _authorizeUpgrade() in upgradeTo() and upgradeToAndCall(), and the Pool contract overrides _authorizeUpgrade:

function _authorizeUpgrade(address) internal override onlyOwner {}

This means that only the owner can perform the upgrade.

But, according to the existing implementation, the owner of the Pool proxy will always be the ZERO address, because it is not initialized. Therefore, only the ZERO address can call upgradeTo() or upgradeToAndCall() successfully, and this is impossible.

Tools Used

VS Code

Recommended Mitigation Steps

Add an initialize function to call the __Ownable_init().

The diff:

diff --git a/contracts/Pool.sol b/contracts/Pool.sol
index 3722082..ff664b5 100644
--- a/contracts/Pool.sol
+++ b/contracts/Pool.sol
@@ -22,6 +22,14 @@ contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {

     event Transfer(address indexed from, address indexed to, uint256 amount);

+       constructor() {
+               _disableInitializers();
+       }
+
+       function initialize() external initializer {
+               __Ownable_init();
+       }
+