Blur Exchange contest - rotcivegaf's results

Lines of code

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

Vulnerability details

Author: rotcivegaf

Impact

The _execute use a modifier to only can called internally, also specified in the documentation of the function: Must be called internally.

But this modifier can be pass if a contract call the execute or bulkExecute and in the _returnDust callback when return the dust, the contract sender call the _execute because the isInternal should be true

Tools Used

Review

Recommended Mitigation Steps

Option 1: Change the isInternal variable, before and after use _execute

@@ -39,10 +39,8 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U

     modifier setupExecution() {
         remainingETH = msg.value;
-        isInternal = true;
         _;
         remainingETH = 0;
-        isInternal = false;
     }

     modifier internalCall() {

@@ -157,7 +155,9 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         whenOpen
         setupExecution
     {
+        isInternal = true;
         _execute(sell, buy);
+        isInternal = false;
         _returnDust();
     }

@@ -181,6 +181,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         _returnDust(remainingETH);
         */
         uint256 executionsLength = executions.length;
+        isInternal = true;
         for (uint8 i = 0; i < executionsLength; i++) {
             assembly {
                 let memPointer := mload(0x40)

@@ -206,6 +207,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
                 let result := delegatecall(gas(), address(), memPointer, add(size, 0x04), 0, 0)
             }
         }
+        isInternal = false;
         _returnDust();
     }

Option 2: Move the reentrancyGuard modifier to the functions execute and bulkExecute

@@ -156,6 +156,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         payable
         whenOpen
         setupExecution
+        reentrancyGuard
     {
         _execute(sell, buy);
         _returnDust();

@@ -170,6 +171,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         payable
         whenOpen
         setupExecution
+        reentrancyGuard
     {
         /*
         REFERENCE

@@ -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/Exchange.sol#L212-L227

Vulnerability details

Author: rotcivegaf

Impact

The Yul call return value on function _returnDust is not checked, which could leads to the sender lose funds

Proof of Concept

The caller of the functions bulkExecute and execute could be a contract who may not implement the fallback or receive functions or reject the call, when a call to it with value sent in the function _returnDust, it will revert, thus it would fail to receive the dust ether

Proof:

• A contract use bulkExecute
• One of the executions fails
• The Exchange contract send the dust(Exchange balance) back to the contract
• This one for any reason reject the call
• The dust stay in the Exchange contract
• In the next call of bulkExecute or execute the balance of the Exchange contract(including the old dust) will send to the new caller
• The second sender will get the funds of the first contract

Tools Used

Review

Recommended Mitigation Steps

+    error ReturnDustFail();
+
     function _returnDust() private {
         uint256 _remainingETH = remainingETH;
+        bool success;
         assembly {
             if gt(_remainingETH, 0) {
-                let callStatus := call(
+                success := call(
                     gas(),
                     caller(),
                     selfbalance(),
                     0,
                     0,
                     0,
                     0
                 )
             }
         }
+        if (!success) revert ReturnDustFail();
     }

QA report

Author: rotcivegaf

Low Risk

Non-critical

Low Risk

[L-01] Open TODO

Open TODO can point to architecture or programming issues that still need to be resolved.

File: contracts/Pool.sol

18    // TODO: set proper address before deployment

[L-02] Remove testing function

File: contracts/Exchange.sol

134    // temporary function for testing
135    function updateDomainSeparator() external {
136        DOMAIN_SEPARATOR = _hashDomain(EIP712Domain({
137            name              : NAME,
138            version           : VERSION,
139            chainId           : block.chainid,
140            verifyingContract : address(this)
141        }));
142    }

Non-critical

[N‑01] require without custom errors

Use custom errors to give the idea what was the cause of failure, source

File: contracts/Exchange.sol

240        require(sell.order.side == Side.Sell);

291        require(msg.sender == order.trader);

573            require(remainingETH >= price);

File: contracts/Pool.sol

45        require(_balances[msg.sender] >= amount);

48        require(success);

71        require(_balances[from] >= amount);

72        require(to != address(0));

[N‑02] Unused imports

File: contracts/Exchange.sol

4 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

[N‑03] Lint

Wrong identation: Use always 4 spaces

File: contracts/Exchange.sol

108      _disableInitializers();

394          return true;

526          return false;

528          return signer == recoveredSigner;

Remove space:

File: contracts/Exchange.sol

 31 \n

392 \n

Don't use extra parenthesis:

File: contracts/Exchange.sol

608        return (receiveAmount);

[N-04] Unused commented code lines

Remove the commented code blocks, to clarify the code

File: contracts/Exchange.sol

282        // return (price);

174        /*
175        REFERENCE
176        uint256 executionsLength = executions.length;
177        for (uint8 i=0; i < executionsLength; i++) {
178            bytes memory data = abi.encodeWithSelector(this._execute.selector, executions[i].sell, executions[i].buy);
179            (bool success,) = address(this).delegatecall(data);
180        }
181        _returnDust(remainingETH);
182        */

486            /*
487            REFERENCE
488            (v, r, s) = abi.decode(extraSignature, (uint8, bytes32, bytes32));
489            */

497            /*
498            REFERENCE
499            uint8 _v, bytes32 _r, bytes32 _s;
500            (bytes32[] memory merklePath, uint8 _v, bytes32 _r, bytes32 _s) = abi.decode(extraSignature, (bytes32[], uint8, bytes32, bytes32));
501            v = _v; r = _r; s = _s;
502            */

Gas report

Author: rotcivegaf

[G-01] Use uint256(1) and uint256(2) for 0(false)/1(true) to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from 0(false) to 1(true), after having been 1(true) in the past, see source

Recommended Mitigation: Change 0 to 1 and 1 to 2

File: contracts/Exchange.sol

 33    uint256 public isOpen;

 36        require(isOpen == 1, "Closed");

 57        isOpen = 1;

 61        isOpen = 0;

119        isOpen = 1;

[G-02] public functions to external functions

The following functions could be set external to save gas and improve code quality external call cost is less expensive than of public functions

File: contracts/Pool.sol

44    function withdraw(uint256 amount) public {

59        public

79    function balanceOf(address user) public view returns (uint256) {

83    function totalSupply() public view returns (uint256) {

[G-03] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

File: contracts/Exchange.sol

146    bool public isInternal = false;

147    uint256 public remainingETH = 0;

[G-04] Unnecessary require

With a - b if a is lower than b this revert with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block) Can remove require or use unchecked in the line

File: contracts/Exchange.sol

/// @audit: checked in L573
573            require(remainingETH >= price);
574            remainingETH -= price;

/// @audit: checked in L604
604        require(totalFee <= price, "Total amount of fees are more than the price");
605
606        /* Amount that will be received by seller. */
607        uint256 receiveAmount = price - totalFee;

File: contracts/Pool.sol

/// @audit: can't overflow
36        _balances[msg.sender] += msg.value;

/// @audit: checked in L45
45        require(_balances[msg.sender] >= amount);
46        _balances[msg.sender] -= amount;

/// @audit: checked in L71
71        require(_balances[from] >= amount);
72        require(to != address(0));
73        _balances[from] -= amount;

/// @audit: can't overflow
74        _balances[to] += amount;

[G-05] <x> += <y>/<x> -= <y> costs more gas than <x> = <x> + <y>/<x> = <x> - <y> for state variables

File: contracts/Exchange.sol

316        nonces[msg.sender] += 1;

574            remainingETH -= price;

File: contracts/Pool.sol

37        _balances[msg.sender] += msg.value;

48        _balances[msg.sender] -= amount;

76        _balances[from] -= amount;

77        _balances[to] += amount;