LooksRare Aggregator contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

Any remaining balance can be used by anyone. This can impact on users who transfers directly to the protocol by mistake.

Proof of Concept

If any user by mistake transfers ERC20/ETH directly (not through the ERC20EnabledLooksRareAggregator or execute function in LooksRareAggregator) to LooksRareAggregator, any body can call execute, and at the end of this function any nonzero balance of LooksRareAggregator will be transferred to the caller. The vulnerability is that when transferring the remaining ERC/ETH, it does not check that how much was the balance of protocol before, and only transfers the difference to the caller not all the remaining.

For example, userA transfers 10 token X directly to LooksRareAggregator by mistake. Later userB calls execute in ERC20EnabledLooksRareAggregator and transfers 20 token X. If userB's transaction is bypassed (like reaching to maxFeeBpViolated for a non-atomic tx), at the end the remaining token X will be transferred to userB which is 20 + 10 token X. This is not correct, because the protocol should check the balance of protocol before and after this transfer, then the difference should be returned to userB. https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L85

Tools Used

Recommended Mitigation Steps

In the ERC20EnabledLooksRareAggregator, the execute function should be modified to:

function execute(
    TokenTransfer[] calldata tokenTransfers,
    ILooksRareAggregator.TradeData[] calldata tradeData,
    address recipient,
    bool isAtomic
) external payable {
    if (tokenTransfers.length == 0) revert UseLooksRareAggregatorDirectly();

    // this loop is added
    uint256[] balanceBefore = new unit256[](tokenTransfersLength);
    for (uint256 i; i < tokenTransfersLength; ++i) {
        balanceBefore[i] = IERC20(tokenTransfers[i].currency).balanceOf(
            address(this)
        );
    }
    // the modified part is finished

    //Rest of code
}

In the LooksRareAggregator, the execute function should be modified to:

function execute(
    TokenTransfer[] calldata tokenTransfers,
    TradeData[] calldata tradeData,
    address originator,
    address recipient,
    bool isAtomic,
    uint256[] calldata balanceBefore // this is added
) external payable nonReentrant {
    //Rest of code

    uint256 balanceDiff;
    for (uint256 i; i < tokenTransfersLength; ++i) {
        balanceDiff =
            IERC20(tokenTransfers[i].currency).balanceOf(address(this)) -
            balanceBefore[i];
        if (balanceDiff > 0)
            _executeERC20DirectTransfer(
                tokenTransfers[i].currency,
                recipient,
                balanceDiff
            );
    }
}