LooksRare Aggregator contest - joestakey's results

Lines of code

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/lowLevelCallers/LowLevelETH.sol#L46

Vulnerability details

The execute function refunds the unused ETH back to the originator through _returnETHIfAny(). This internal function uses a low-level call to transfer the ETH.

The issue is that the return value of the call is not checked. As per the Solidity documentation

Exceptions to this rule are send and the low-level functions call, delegatecall and staticcall:
they return false as their first return value in case of an exception

If originator is not an EOA and has some logic in a receive() function that can revert upon certain conditions, it means the _returnETHIfAny() would silently fail without reverting, and the originator would not retrieve that ETH.

Note that the ETH is not frozen, as the next user calling execute() would retrieve it - as _returnETHIfAny() returns the entire balance of the contract - effectively "stealing" the ETH that was stuck after the previous execute() call.

Impact

Medium

Tools Used

Manual Analysis

Mitigation

Check the return value so that the function call reverts when status is false - this is already done in the functions of LowLevelERC20Transfer

43:     function _returnETHIfAny(address recipient) internal {
44:         assembly {
45:             if gt(selfbalance(), 0) {
46:                 let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
47:             }
48:         }
+           if (!status) revert();
49:     }