Blur Exchange contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

Exchange#bulkExecute() can reenter, and internal execution of delegatecall is allowed to fail, not revert, malicious users can reenter to steal funds

Proof of Concept

Assumptions: There is a malicious user alice,with NFT for sale, paid through eth If user bob executes a purchase steps:

1. bob executes Exchange#bulkExecute(), 2 Executions, pass msg.value=100 eth

Execution[0] = {seller:alice,paymentToken=address(0),price:50 eth} Execution[1] = {seller:jack,paymentToken=address(0),price:50 eth}

2. When Execution[0] is executed, #_executeFundsTransfer()->payable(alice).call{value: 50 eth}("") will be executed;

3. malicious user alice is a contract, alice#receive() be called, executes a new call, reentering Exchange#bulkExecute{value:1 wei}(Execution[]=[0]) At this point: remainingETH = 1 wei and #bulkExecute ends immediately, and Exchange#_returnDust() is executed, at which point the contract still has 50 eth left, will transfer to alice (which was meant to be used to trade with jack). and remainingETH is changed to 0,remainingETH=0

4. Execution[0] end of the transaction, back to the Execution[1], the execution of jack's transaction, due to insufficient eth, this transaction will fail, but will not revert

5. bob's entire Exchange#bulkExecute() ends normally, due to remainingETH=0, will not execute #_returnDust()

6. so that the user bob was stolen 50 eth in step 3

    function _transferTo(
        address paymentToken,
        address from,
        address to,
        uint256 amount
    ) internal {

            (bool success,) = payable(to).call{value: amount}(""); /****@audit step 2 call alice#receive() ***/

    function _returnDust() private {
        uint256 _remainingETH = remainingETH;
        assembly {
            if gt(_remainingETH, 0) {
                let callStatus := call(
                    gas(),
                    caller(),
                    selfbalance(), //*****@audit step 3 , steal 50 eth to alice****/
                    0,
                    0,
                    0,
                    0
                )
            }
        }
    }

    function bulkExecute(Execution[] calldata executions)
        external
        payable
        whenOpen
        setupExecution
    {
...
                calldatacopy(add(0x04, memPointer), order_pointer, size)
                // must be put in separate transaction to bypass failed executions
                // must be put in delegatecall to maintain the authorization from the caller
                let result := delegatecall(gas(), address(), memPointer, add(size, 0x04), 0, 0) /****@audit  step 4 , fail but don't revert****/
            }
        }

Tools Used

Recommended Mitigation Steps

#_execute use own reentrancyGuardForOneExecute

contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {

+    bool private reentrancyLockForOneExecute = false;

+    modifier reentrancyGuardForOneExecute {
+        require(!reentrancyLockForOneExecute, "Reentrancy detected");
+        reentrancyLockForOneExecute = true;
+        _;
+        reentrancyLockForOneExecute = false;
+    }

    function _execute(Input calldata sell, Input calldata buy)
        public
        payable
-       reentrancyGuard
+       reentrancyGuardForOneExecute
        internalCall
    {
..

    function execute(Input calldata sell, Input calldata buy)
        external
        payable
        whenOpen
+       reentrancyGuard         
        setupExecution
    {
...


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

    {

Lines of code

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

Vulnerability details

Impact

Pool.sol is Upgradeable but not method "#initialize()", can't init owner, owner still address(0), unable to Upgradeable

Proof of Concept

contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
...

/***@audit not method initialize() ****


    function _authorizeUpgrade(address) internal override onlyOwner {} /****@audit upgrade need onlyOwner,but still address(0) ***/

Tools Used

Recommended Mitigation Steps

add initialize() , deploy need call

contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {

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