LooksRare Aggregator contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L227-L270

Vulnerability details

Summary

When calling SeaportProxy#execute the user is allowed to supply a price field for each order. This price is used to determine the amount to pay fees on. For orders fulfilled by ERC20 tokens a user can set price = 0 and skip all fees.

Impact

User can avoid paying all fees on ERC20 orders

Proof of Concept

    for (uint256 i; i < ordersLength; ) {
        OrderExtraData memory orderExtraData = abi.decode(ordersExtraData[i], (OrderExtraData));
        advancedOrders[i].parameters = _populateParameters(orders[i], orderExtraData);
        advancedOrders[i].numerator = orderExtraData.numerator;
        advancedOrders[i].denominator = orderExtraData.denominator;
        advancedOrders[i].signature = orders[i].signature;

        if (orders[i].currency == address(0)) ethValue += orders[i].price;

        unchecked {
            ++i;
        }
    }

    (bool[] memory availableOrders, ) = marketplace.fulfillAvailableAdvancedOrders{value: ethValue}(
        advancedOrders,
        new CriteriaResolver[](0),
        extraDataStruct.offerFulfillments,
        extraDataStruct.considerationFulfillments,
        bytes32(0),
        recipient,
        ordersLength
    );

SeaportProxy#execute allows the user to supply an array of orders. order.price is a user supplied value that is used to determine the amout of fees. For orders filled with native ETH it determines the msg.value to send to the OpenSea marketplace, but for ERC20 orders their sole purpose it for fee calculation. The result is that a user can specify an order.price of 0 for all ERC20 orders and avoid paying any fees on those orders.

Tools Used

Manual Review

Recommended Mitigation Steps

In SeaportProxy#_populateParameters the first consideration amount should be set to order.price rather than orderExtraData.recipients[j].amount:

    for (uint256 j; j < recipientsLength; ) {
        // We don't need to assign value to identifierOrCriteria as it is always 0.

+       if (j > 0) {
+           uint256 recipientAmount = orderExtraData.recipients[j].amount;
+       } else {
+            uint256 recipientAmount = order.price;
+       }

-       uint256 recipientAmount = orderExtraData.recipients[j].amount;
        consideration[j].startAmount = recipientAmount;
        consideration[j].endAmount = recipientAmount;
        consideration[j].recipient = payable(orderExtraData.recipients[j].recipient);
        consideration[j].itemType = order.currency == address(0) ? ItemType.NATIVE : ItemType.ERC20;
        consideration[j].token = order.currency;

        unchecked {
            ++j;
        }
    }

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L32-L38 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L54-L61

Vulnerability details

Impact

ETH transfer my fail resulting in ETH not being returned to user

Proof of Concept

function _returnETHIfAny() internal {
    assembly {
        if gt(selfbalance(), 0) {
            let status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0)
        }
    }
}

In the low level functions that return ETH to the user after an aggregate trade fails to validate the return value of the ETH transfer. If the transfer fails, the user's ETH would become stuck in the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Confirm that the call returns true for LowLevelETH#_returnETHIfAny, and _returnETHIfAnyWithOneWeiLeft:

function _returnETHIfAny() internal {
    assembly {
        if gt(selfbalance(), 0) {
            let status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0)
        }
    }

+   if (!status) revert ETHTransferFail();
}

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L227-L270

Vulnerability details

Impact

SeaportProxy will be unable to fill orders in which there are multiple considerations with different tokens

Proof of Concept

for (uint256 j; j < recipientsLength; ) {
    // We don't need to assign value to identifierOrCriteria as it is always 0.
    uint256 recipientAmount = orderExtraData.recipients[j].amount;
    consideration[j].startAmount = recipientAmount;
    consideration[j].endAmount = recipientAmount;
    consideration[j].recipient = payable(orderExtraData.recipients[j].recipient);
    consideration[j].itemType = order.currency == address(0) ? ItemType.NATIVE : ItemType.ERC20;
    consideration[j].token = order.currency;

    unchecked {
        ++j;
    }
}

When setting consideration[j].token, it is always set to order.currency. In the case in which one of the considerations tokens is not order.currency the constructed consideration will not match the consideration of the desired OpenSea order, causing the order hash. Since the order hash is different the provided signature will not be valid to fill the order. The result is that the orders in which there are multiple considerations with different tokens, will be impossible to fill.

Tools Used

Manual Review

Recommended Mitigation Steps

The AdditionalRecipient struct should contain a field to specify the consideration token