LooksRare Aggregator contest - SinceJuly's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L35 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L46 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L57 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/prototype/V0Aggregator.sol#L61 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L109

Vulnerability details

Impact

When users uses ETH to batch buy NFTs, if the transaction fails, the aggregator contract will call the function _returnETHIfAny to return the ETH to the user. This function transfers all the balance in the contract to the recipient through call, but there is no check of return value to the call. Therefore, there may be two problems: one is that there is a possibility that the user fails to withdraw ETH when transactions fail, and the other is that if the first user fails to withdraw ETH, then the next user who needs to withdraw ETH can get more ETH.

Proof of Concept

/**
 * @notice Return ETH back to the original sender if any ETH is left in the payable call.
 */
function _returnETHIfAny() internal {
    assembly {
        if gt(selfbalance(), 0) {
            let status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0)
        }
    }
}

/**
 * @notice Return ETH back to the designated sender if any ETH is left in the payable call.
 */
function _returnETHIfAny(address recipient) internal {
    assembly {
        if gt(selfbalance(), 0) {
            let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
        }
    }
}

/**
 * @notice Return ETH to the original sender if any is left in the payable call but leave 1 wei of ETH in the contract.
 */
function _returnETHIfAnyWithOneWeiLeft() internal {
    assembly {
        if gt(selfbalance(), 1) {
            let status := call(gas(), caller(), sub(selfbalance(), 1), 0, 0, 0, 0)
        }
    }
}

The return value of ETH transfer is not checked, therefore, if this call fails, user may fail to withdraw ETH, or if the first user fails to withdraw ETH, then the next user who needs to withdraw ETH can get more ETH.

Tools Used

Recommended Mitigation Steps

It is recommended to check the return value, and when the return value is false, just revert.

Lines of code

https://github.com/code-423n4/2022-11-looksrare/tree/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L199-L217

Vulnerability details

Impact

The fulfillAdvancedOrder may return false on an order not successfully fulfilled. The try/catch only skip current order when it reverts on fail, while still charge fee on user when it returns false.

Proof of Concept

https://github.com/code-423n4/2022-11-looksrare/tree/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L199-L217

try
    marketplace.fulfillAdvancedOrder{value: currency == address(0) ? price : 0}(
        advancedOrder,
        new CriteriaResolver[](0),
        bytes32(0),
        recipient
    )
  {
    if (feeRecipient != address(0)) {
        uint256 orderFee = (price * feeBp) / 10000;
        if (currency == lastOrderCurrency) {
            fee += orderFee;
        } else {
            if (fee > 0) _transferFee(fee, lastOrderCurrency, feeRecipient);
  
            lastOrderCurrency = currency;
            fee = orderFee;
        }
    }
  } catch {}

when marketplace.fulfillAdvancedOrder return false (but not reverted), the code in the try clause will execute, and charge the fee on the user. However, it is inappropriate to charge a fee on a not fulfilled order.

Tools Used

Recommendation

also check return value on marketplace.fulfillAdvancedOrder

try
    marketplace.fulfillAdvancedOrder{value: currency == address(0) ? price : 0}(
        advancedOrder,
        new CriteriaResolver[](0),
        bytes32(0),
        recipient
    ) returns (bool fulfilled)
  {
    if (fulfilled && feeRecipient != address(0)) {
        uint256 orderFee = (price * feeBp) / 10000;
        if (currency == lastOrderCurrency) {
            fee += orderFee;
        } else {
            if (fee > 0) _transferFee(fee, lastOrderCurrency, feeRecipient);
  
            lastOrderCurrency = currency;
            fee = orderFee;
        }
    }
  } catch {}