LooksRare Aggregator contest - carlitox477's results

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#L54-L60 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L43 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelETH.sol#L54 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L109 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L51-L57

Vulnerability details

Impact

This means that the caller won't receive ETH but the transaction will continue, this could specially affects to function LookRareAggregator.execute, which use _returnETHIfAny(address) function, leading to originator lose of funds or mess up its state variables.

Proof of Concept

Originator is a smart cotnract which will revert if it current balance is greater than 10. Originator contract has a function which calls LookRareAggregator.execute (for instance execute) and after that update a counter for current trades, which afect its accountability, then:

1. Bob calls originator smart contract (current balance 11) function execute
2. Originator smart contract calls LooksRareAggregator.execute, it result that some ETH is remaining after the trades
3. _returnETHIfAny(originator) is called, when this function tries to send ETH this last call is reverted, but it continue the _returnETHIfAny execution
4. LooksRareAggregator.execute execution finished
5. Originator execute function continues it execution and edit diferent state variables which affect it accountability

Originator smart contract accountability was mess up for using LooksRareAgregator contract.

Tools Used

Manual review

Recommended Mitigation Steps

Add next line at the end of each function if (!status) revert ETHTransferFail();

LooksRareAggregator#setERC20EnabledLooksRareAggregator allows to emit nonsense event if zero is pass as new address

If _erc20EnabledLooksRareAggregator = address(0), setERC20EnabledLooksRareAggregator won't revert, and it will emit nonsense event ERC20EnabledLooksRareAggregatorSet. This function should check that _erc20EnabledLooksRareAggregator != address(0)

LooksRareAggregator#addFunction allows to emit nonsense event if already added proxy and selector is sent

If proxy function was already added, then addFunction will emit a nonsense event. The function should check that _proxyFunctionSelectors[proxy][selector] is false.

LooksRareAggregator#removeFunction allows to emit nonsense event if not added proxy and selector is sent

If proxy function was not already added, then removeFunction will emit a nonsense event. The function should check that _proxyFunctionSelectors[proxy][selector] is true.

LooksRareAggregator#setFee allows to emit nonsense event if fees are not really changed

If current values of _proxyFeeData[proxy] are sent, then the function will emit a nonsense event. This function should check that _proxyFeeData[proxy].bp != bp && _proxyFeeData[proxy].recipient != recipient;

ERC20EnabledLooksRareAggregator#constructor allows to set address zero to aggregator

It should be checked that _aggregator != address(0), if _aggregator == address(0) it will be needed to redeploy.

LooksRareProxy#constructor allows to set address zero to aggregator and marketplace

It should be checked that _aggregator != address(0) and _marketplace != address(0), if _aggregator == address(0) or _marketplace == address(0) it will be needed to redeploy.

SeaportProxy#constructor allows to set address zero to aggregator and marketplace

It should be checked that _aggregator != address(0) and _marketplace != address(0), if _aggregator == address(0) or _marketplace == address(0) it will be needed to redeploy.

_executeERC721TransferFrom, _executeERC1155SafeTransferFrom, _executeERC1155SafeBatchTransferFrom do not check contract length

The problem has similarities with one reported in OlympusDao contest. The problem is the ERC721 and ERC1155 contract length lack of check. These function in the whole interaction of the present contract does not present major issues, however if they are intended to be used by other contracts it may well present several high issues.

ReentrancyGuard can save gas during deployment if status manages only values 0 and 1

This implies changing the contract to

abstract contract ReentrancyGuard is IReentrancyGuard {
    uint256 private _status;

    constructor() {}

    /**
     * @notice Modifier to wrap functions to prevent reentrancy calls
     */
    modifier nonReentrant() {
        if (_status == 1) revert ReentrancyFail();
        _status = 1;
        _;
        _status = 0;
    }
}

OwnableTwoSteps#cancelOwnershipTransfer can save gas by caching ownershipStatus state variable

This state variable is accessed 4 times. This can be reduced to two by caching its value.

function cancelOwnershipTransfer() external onlyOwner {
    _ownershipStatus = ownershipStatus
    if (_ownershipStatus == Status.NoOngoingTransfer) revert NoOngoingTransferInProgress();

    if (_ownershipStatus == Status.TransferInProgress) {
        delete potentialOwner;
    } else if (_ownershipStatus == Status.RenouncementInProgress) {
        delete earliestOwnershipRenouncementTime;
    }

    delete ownershipStatus;

    emit CancelOwnershipTransfer();
}

OwnableTwoSteps#confirmOwnershipTransfer can omit one state variable access

Last owner access can be avoided by replacing this line for emit NewOwner(msg.sender);

OwnableTwoSteps#initiateOwnershipTransfer can omit one state variable access

Giving modifier onlyOwner, we can assure thatmsg.sender = owner. This means that we can avoid last owner access by replacing this line for emit InitiateOwnershipTransfer(msg.sender, newPotentialOwner);

OwnableTwoSteps#initiateOwnershipRenouncement can omit one state variable access

We can avoid one access to state variable earliestOwnershipRenouncementTime by replacing these lines for:

uint256 _earliestOwnershipRenouncementTime = block.timestamp + delay;
earliestOwnershipRenouncementTime = _earliestOwnershipRenouncementTime

emit InitiateOwnershipRenouncement(_earliestOwnershipRenouncementTime);

LooksRareAgregator#setFee can avoid double state variable access

These lines can be replaced to avoid double _proxyFeeData[proxy] access for:

    FeeData newFeeData = {bp: bp, recipient: recipient };
    _proxyFeeData[proxy] = newFeeData;