Blur Exchange contest - 0x4non's results

Lines of code

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

Vulnerability details

Impact

The call opcode's return value not checked, which could leads to the originator lose funds.

Proof of Concept

The caller of Exchange.sol::execute or Exchange.sol::bulkExecute could be a contract who may not implement the fallback or receive function, when a call to it with value sent, it will revert, thus failed to receive the ETH.

Lets imagine that we call execute or bulkExecute and there is some remaining ETH, the contract orginator doesn't implement the fallback/receive function and the call opcode's return value not checked, thus the ETH value will be trapped in the Exchange contract. The originator lose funds.

Tools Used

manual revision

Recommended Mitigation Steps

Check return value

diff --git a/contracts/Exchange.sol b/contracts/Exchange.sol
index ec27b1d..7e56de0 100644
--- a/contracts/Exchange.sol
+++ b/contracts/Exchange.sol
@@ -210,10 +210,11 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
     }
 
     function _returnDust() private {
+        bool callStatus;
         uint256 _remainingETH = remainingETH;
         assembly {
             if gt(_remainingETH, 0) {
-                let callStatus := call(
+                callStatus := call(
                     gas(),
                     caller(),
                     selfbalance(),
@@ -224,6 +225,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
                 )
             }
         }
+        require(callStatus, "Fail ETH transfer");
     }
 
     /**

QA

Low

If blockRange is set to 0 or a small value executions will always revert

The require on line Exchange.sol#L414 will always fail if you set to 0 (or will probably always fail with a small value)

Please add a minimum value check on setBlockRange function in Exchange.sol#L354

Unexpected ether in Pool can break invariants

Consider the next invariant; SUM(balances) == totalSupply

So the sum of all users balances is equal to the totalSupply. But since totalSupply is the balance of the contract a malicius user could force feeding chainging contract balances and breaking this invariant.

Consider keep track of totalSupply in a variable instead of using address(this).balance as you do on Pool.sol#L83-L85

bulkExecute function on Exchange.sol#L168 should have nonReentrant modifier

The bulkExecute cant follow the check effects itteration pattern it would be best to add a nonReentrant modifier to avoid a reentrancy issue causing unexpected outputs.

Exchange.sol#L204-L206 :: Mising event emmision to log/notify if execution is success or fail

So you arent checking the result of the delegatecall because you dont want to revert if the execution is not successful, but you should emit events to track whats going on.

Recommendation, add event emmision to track delegate call output on Exchange.sol#L204-L206

You also may reconsider this pattern... if a delegatecall fails, why you wouldnt want to revert the whole transaction?

Open access on function Exchange::_execute

It seems that _execute function its meant to be executed only internally, if so, why would you have a public modifier? lines Exchange.sol#L235

Mark this function as internal or private.

Non critical

Empty revert messages

On: Exchange.sol#L291 Exchange.sol#L573 Pool.sol#L45 Pool.sol#L48 Pool.sol#L71 Pool.sol#L72

Consider adding clear revert messages.

Use named imports

Use named imports on Exchange.sol#L4-L15 and Pool.sol#L4-L7 to make clear the contracts you use like you do on lines Exchange.sol#L16-L24

Exchange.sol :: Remove temporary testing functions

On Exchange.sol#L134-L142 the function updateDomainSeparator is marked as // temporary function for testing. This function should be removed for the production version.

Exchange.sol :: Commented return should be deleted

There is a commented retun on Exchange.sol#L282 that should be removed;

        // return (price);

Exchange.sol :: TODO commented

Resolve and remove the follow comment on Pool.sol#L18 // TODO: set proper address before deployment

Exchange.sol :: Address(0) check should be made outside the if

Consider moving out the if statement the address(0) check on Exchange.sol#L630

From;

diff --git a/contracts/Exchange.sol b/contracts/Exchange.sol
index ec27b1d..3ab30c1 100644
--- a/contracts/Exchange.sol
+++ b/contracts/Exchange.sol
@@ -625,9 +626,9 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
             return;
         }
 
+        require(to != address(0), "Transfer to zero address");
         if (paymentToken == address(0)) {
             /* Transfer funds in ETH. */
-            require(to != address(0), "Transfer to zero address");
             (bool success,) = payable(to).call{value: amount}("");
             require(success, "ETH transfer failed");
         } else if (paymentToken == POOL) {

Use openzeppelin/solmate patterns instead of rewrite them

There are some features that are alreade made, my recommendation its to leverage on this battle test contracts instead of rewrite them.

Exchange.sol is importing a ReentrancyGuard, EIP712 and a MerkleVerifier, OZ got you cover so you could leverage on this contracts, please take a look to;

• ReentrancyGuard
• EIP712
• MerkleProof
• And for low level transfers check solmate implementation of SafeTransferLib

gas

Upgrade OZ libs to latest version

Upgrade OZ to latest version that has tons of gas optimizations

diff --git a/package.json b/package.json
index a045a74..e64fc3b 100644
--- a/package.json
+++ b/package.json
@@ -58,8 +58,8 @@
     "@makerdao/hardhat-utils": "^0.1.3",
     "@nomiclabs/ethereumjs-vm": "^4.2.2",
     "@nomiclabs/hardhat-web3": "^2.0.0",
-    "@openzeppelin/contracts": "4.4.1",
-    "@openzeppelin/contracts-upgradeable": "^4.6.0",
+    "@openzeppelin/contracts": "4.8.0",
+    "@openzeppelin/contracts-upgradeable": "^4.8.0",
     "@types/chai-almost": "^1.0.1",
     "@types/yargs": "^17.0.7",
     "chai-almost": "^1.0.1",

Some functions on Poolshould be marked as external

These functions are not being used by the contract and can be marked as external to save gas.

• withdraw
• transferFrom
• balanceOf
• totalSupply

Optimization of Pool:_transfer method

Consider this optimization;

diff --git a/contracts/Pool.sol b/contracts/Pool.sol
index 3722082..6746263 100644
--- a/contracts/Pool.sol
+++ b/contracts/Pool.sol
@@ -68,19 +68,18 @@ contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
     }
 
     function _transfer(address from, address to, uint256 amount) private {
-        require(_balances[from] >= amount);
         require(to != address(0));
         _balances[from] -= amount;
-        _balances[to] += amount;
+        unchecked { _balances[to] += amount; }
 
         emit Transfer(from, to, amount);
     }

We can safely remove require(_balances[from] >= amount); because will revert with underflow on _balances[from] -= amount; if this condition cant be keeped. Also we can safely add an unchecked wrap to _balances[to] += amount; also because of the previous check. Please take a look to solmate erc20 implementation; https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC20.sol#L76-L88

Optimize Exchange::incrementNonce method

On Exchange::incrementNonce

Instead of

    function incrementNonce() external {
        nonces[msg.sender] += 1;
        emit NonceIncremented(msg.sender, nonces[msg.sender]);
    }

You could just;

    function incrementNonce() external {
      unchecked {
        emit NonceIncremented(msg.sender, nonces[msg.sender]++);
      }
    }

Its safe to use unchecked, its virtually impossible to overflow, also you could save the extra SLOAD inlining everything (or using an internal variable to save the new nonce)

The modifier internalCall is used only once, just add the require to save gas

The modifier internalCall on Exchange.sol#L48-L50 is used only once in _execute Exchange.sol#L238 so you could just add the inline require to save gas;

diff --git a/contracts/Exchange.sol b/contracts/Exchange.sol
index ec27b1d..6d438e8 100644
--- a/contracts/Exchange.sol
+++ b/contracts/Exchange.sol
@@ -235,8 +235,8 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         public
         payable
         reentrancyGuard
-        internalCall
     {
+        require(isInternal, "This function should not be called directly");
         require(sell.order.side == Side.Sell);
 
         bytes32 sellHash = _hashOrder(sell.order, nonces[sell.order.trader]);

Add unchecke on safe math operations

On Exchange.sol#L574 its safe to add unchecked due previous check require(remainingETH >= price);

diff --git a/contracts/Exchange.sol b/contracts/Exchange.sol
index ec27b1d..96b468f 100644
--- a/contracts/Exchange.sol
+++ b/contracts/Exchange.sol
@@ -571,7 +573,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
     ) internal {
         if (msg.sender == buyer && paymentToken == address(0)) {
             require(remainingETH >= price);
-            remainingETH -= price;
+            unchecked { remainingETH -= price; }
         }
 
         /* Take fee. */

On Pool.sol#L36::deposit its safe to add unchecked

diff --git a/contracts/Pool.sol b/contracts/Pool.sol
index 3722082..f1a0dec 100644
--- a/contracts/Pool.sol
+++ b/contracts/Pool.sol
@@ -33,7 +33,7 @@ contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
      * @dev deposit ETH into pool
      */
     function deposit() public payable {
-        _balances[msg.sender] += msg.value;
+        unchecked { _balances[msg.sender] += msg.value; }
         emit Transfer(msg.sender, address(0), msg.value);
     }

On [Pool.sol#L44::withdraw]https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L44 you could do unchecked or remove the require

Option 1:

diff --git a/contracts/Pool.sol b/contracts/Pool.sol
index 3722082..578f7e7 100644
--- a/contracts/Pool.sol
+++ b/contracts/Pool.sol
@@ -42,7 +42,6 @@ contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
      * @param amount Amount to withdraw
      */
     function withdraw(uint256 amount) public {
-        require(_balances[msg.sender] >= amount);
         _balances[msg.sender] -= amount;
         (bool success,) = payable(msg.sender).call{value: amount}("");
         require(success);

The require is not necessary because it will trigger an underflow revert, but if you still want to use the require to add a clear revert message you could do an unchecked operation after;

diff --git a/contracts/Pool.sol b/contracts/Pool.sol
index 3722082..2f15b01 100644
--- a/contracts/Pool.sol
+++ b/contracts/Pool.sol
@@ -43,7 +43,7 @@ contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
      */
     function withdraw(uint256 amount) public {
         require(_balances[msg.sender] >= amount);
-        _balances[msg.sender] -= amount;
+        unchecked { _balances[msg.sender] -= amount; }
         (bool success,) = payable(msg.sender).call{value: amount}("");
         require(success);
         emit Transfer(address(0), msg.sender, amount);

Send ether on using a yul implementation on Pool.sol#L47

Since you are already doing that on _returnDust you could do it also here;

diff --git a/contracts/Pool.sol b/contracts/Pool.sol
index 3722082..2b2102a 100644
--- a/contracts/Pool.sol
+++ b/contracts/Pool.sol
@@ -43,9 +43,13 @@ contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
      */
     function withdraw(uint256 amount) public {
         require(_balances[msg.sender] >= amount);
-        _balances[msg.sender] -= amount;
-        (bool success,) = payable(msg.sender).call{value: amount}("");
-        require(success);
+        unchecked { _balances[msg.sender] -= amount; }
+        bool success;
+        /// @solidity memory-safe-assembly
+        assembly {
+            // Transfer the ETH and store if it succeeded or not.
+            success := call(gas(), caller(), amount, 0, 0, 0, 0)
+        }
         emit Transfer(address(0), msg.sender, amount);
     }

Mark functions as payable (with discretion)

You can mark public or external functions as payable to save gas. Functions that are not payable have additional logic to check if there was a value sent with a call, however, making a function payable eliminates this check. This optimization should be carefully considered due to potentially unwanted behavior when a function does not need to accept ether.

My recommendation is to mark all onlyOwner functions as payable on lines;

• Exchange.sol#L56
• Exchange.sol#L60
• Exchange.sol#L66
• Exchange.sol#L325
• Exchange.sol#L334
• Exchange.sol#L343
• Exchange.sol#L352