Theft of ETH that was not used for successful execution of orders in non-atomic execution

[code423n4]

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();
}

[c4-judge]

Picodes marked the issue as duplicate of #241

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory