LooksRare Aggregator contest - corerouter's results

Lines of code

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

Vulnerability details

Impact

It is possible that a user's ETH will be trapped inside the contracts.

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L109 _returnETHIfAny(originator);

But in the implementation of the _returnETHIfAny() function, it will just silently return even the returning ETH transaction fails. As a result, even if the user fails to claims back his ETH, the transaction will succeed which will cause the user's ETH is trapped inside the contract and may be claimed by others later.

Proof of Concept

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49

Recommended Mitigation Steps

It is suggested to modify the implementation of the _returnETHIfAny() function to make it revert when the transaction fails. The modified version of the function is shown as follows:

function _returnETHIfAny(address recipient) internal { bool status = true;

    assembly {
        if gt(selfbalance(), 0) {
            status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
        }
    }

    if (!status) revert ETHTransferFail();

}