LowLevelETH: _returnETHIfAny; _returnETHIfAny; _returnETHIfAnyWithOneWeiLeft do not check if call was successful

[code423n4]

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

[c4-judge]

Picodes marked the issue as duplicate of #241

[c4-judge]

Picodes marked the issue as satisfactory