LooksRare Aggregator contest - V_B's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/f4c90ca149f4aeeac125605a56166297b717201a/contracts/LooksRareAggregator.sol#L51 https://github.com/code-423n4/2022-11-looksrare/blob/f4c90ca149f4aeeac125605a56166297b717201a/contracts/LooksRareAggregator.sol#L109 https://github.com/code-423n4/2022-11-looksrare/blob/f4c90ca149f4aeeac125605a56166297b717201a/contracts/lowLevelCallers/LowLevelETH.sol#L43 https://github.com/code-423n4/2022-11-looksrare/blob/f4c90ca149f4aeeac125605a56166297b717201a/contracts/lowLevelCallers/LowLevelETH.sol#L46

Vulnerability details

Description

There is an execute function in LooksRareAggregator contract. It refunds any ETH that was unused (for example that left due to the unsuccessful execution of an order) at the end of its execution flow:

_returnETHIfAny(originator);

_returnETHIfAny function is the following:

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

Let's remember the EIP-150 63/64 rule. The 63/64 rule says that call opcode can consume at most 63/64 gas of parent call, in other words, the child call can consume "all but one 64th" of the gas, and parent calls will have at least 1/64 of gas to continue the execution.

So, _returnETHIfAny can make a call with gas that is not enough to successfully end the execution of the transfer, but the parent call will still have some gas and can successfully end the execution. As result, the remaining ETH will not be refunded but the execution will succeed.

After that, the attacker can withdraw the remaining ETH by calling execute function with any valid input and receiving the remaining ETH in the same way that the fair user (actually he is the attacker's target) expected to receive it.

The same is applicable for the _returnETHIfAny() and _returnETHIfAnyWithOneWeiLeft() functions, but they are not used in places of the code where it can lead to some malicious actions.

Attack scenario

A fair user calls execute function with a nonzero ETH value and isAtomic parameter equals false.

Some of the orders fail and a situation described in Description section of this document happens ("according to EIP-150 rule ETH that should be refunded to the user will remain on the aggregator contract but the execution of the overall call will succeed"). That can happen if the attacker (for example using MEV logic) front-runs such a transaction and changes the state in an appropriate way (changes some storage slots/executes some orders by himself/makes some of the orders available/etc.). Also, it is possible if an originator is a smart contract and the call of a fallback function fails due to its internal logic.

In the next transaction, the attacker calls execute function with any valid input (for example buying a cheap, or even fake one, nft) and receives funds that correspond to the fair user, according to the logic of the _returnETHIfAny function.

Impact

Theft of ETH that was not used for successful execution of orders in non-atomic execution, with the possibility of forcing a fair user to this scenario.

Please note that eth_estimateGas from web3 json-rpc API, returns gas that isn't enough to finish the returning funds to the recipient. Because of that, many users will use this standard method will suffer from loss of funds.

Recommended Mitigation Steps

Add an additional check that the refund ETH call was succeeded:

/**
 * @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)
        }
    }
    if (!status) revert ETHTransferFail();
}

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L22 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L34 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L27 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L108 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L109 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L245 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43

Vulnerability details

Description

The LooksRareAggregator smart contract implements a bunch of functions to escape funds by the contract owner (see rescueETH, rescueERC20, rescueERC721, and rescueERC1155). In this way, any funds that were accidentally sent to the contract or were locked due to incorrect contract implementation can be returned to the owner. However, locked funds can be rescued by anyone without the owner's permission. This is completely contrary to the idea of having rescue functions.

In order to withdraw funds from the contract, a user may just call the execute function in the ERC20EnabledLooksRareAggregator with tokenTransfers that contain the addresses of tokens to be withdrawn. Thus, after the order execution _returnERC20TokensIfAny and _returnETHIfAny will be called, and the whole balance of provided ERC20 tokens and Ether will be returned to msg.sender.

Please note, that means that the owner can be front-ran with rescue functions and an attacker will receive funds instead.

Impact

Useless of rescue functionality and vulnerability to jamming funds.

Recommended Mitigation Steps

_returnETHIfAny and _returnERC20TokensIfAny should return the amount of the token that was deposited.

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/SeaportProxy.sol#L211 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/SeaportProxy.sol#L172 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L19

Vulnerability details

Description

The LooksRareAggregator owner can set up any address as a fee receiver. Later, the user that calls SeaportProxy will send the fee directly to that address. Moreover, the fee receiver can be a smart contract that executes some logic, so with transferring fees malicious fee receiver can reenter the aggregator contract.

Please note, that owner has the opportunity to rescue funds via rescueEther/rescueERC20, so the malicious fee receiver can reenter rescueEther and/or rescueERC20 to steal funds that weren't processed yet.

Attack scenario

1. Owner set a malicious smart contract as a fee receiver
2. Owner transfer ownership to the malicious smart contract (can be the same "owner" contract)
3. User calls the execute function on the LooksRareAggregator/ERC20EnabledLooksRareAggregator with isAtomic == false
4. The first order is processed, the contract calls the fee receiver to transfer fees
5. Malicious fee receiver contract calls rescueEther and rescueERC20 and steals all funds
6. All other orders fail due to a lack of funds

All in all, all the user's money will be stolen instead of going to fulfill orders.

Impact

A malicious owner can steal user's funds.

Recommended Mitigation Steps

Require the fee receiver is an EOA account, to prevent a reentrancy attack.