Native funds on the aggregator contract balance is a free grab

[code423n4]

Lines of code

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

Vulnerability details

Native funds on the aggregator contract balance is a free grabLooksRareAggregator's execute() returns the native balance of the contract to the caller even when nothing was provided with the call.

This happens when LooksRareAggregator's execute() is called directly by a user with no ERC20 transfers, i.e. when tokenTransfersLength == 0.

Bob the attacker can setup a bot that tracks aggregator's native balance and calls execute() with isAtomic == false and some bogus trade to be reverted on the marketplace level, just to invoke _returnETHIfAny(Bob), which will transfer LooksRareAggregator's native balance to Bob.

This way whenever any user mistakenly sends directly any native funds to the contract, an operational mistake that happens a lot with any router-like contracts and inexperienced users, these funds will be immediately stolen by Bob's bot.

Net impact here is fund loss, which is conditional on user's mistake, so setting the severity to medium.

Proof of Concept

execute() can be called directly and set originator = msg.sender, then proceeds with _returnETHIfAny(originator):

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

    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; ) {
                ...
        }

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

        emit Sweep(originator);
    }

_returnETHIfAny() sends the whole native balance to the recipient:

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L40-L49

    /**
     * @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)
            }
        }
    }

Funds can be send directly to the contract by any user:

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L219-L220

    receive() external payable {}

Recommended Mitigation Steps

As _returnETHIfAny() is to refund the native funds provided by the caller, consider recording msg.value of the call and send back min(msg.value, balance).

[c4-judge]

Picodes marked the issue as duplicate of #277

[c4-judge]

Picodes marked the issue as satisfactory