calling execute() may lead to stealing funds if some ETH is stuck on the contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

if some Ether is sent (erroneously or not) to the contract, anyone that calls correctly LooksRareAggregator.execute will be able to steal those coins.

to execute the function using ethers as payment, these conditions must be true:

• there is at least a trade (tradeData), tradeDataLength > 0
• recipient != address 0

if tokenTransfersLength == 0, originator will be set as msg.sender() aka the contract that is calling the function.

assuming the user can successfully execute an order through any proxy, at the end of the function there is the call:

_returnETHIfAny(originator);

this function is responsible to check if self balance > 0 and send the balance to recipient, which is, in this case, msg.sender

Proof of Concept

Prerequisite is that the LooksRareAggregator contract has received somehow some Ethers.

call execute() function without specify any tokenTransfer; assuming the call through proxy does not revert, the function will transfer the whole available balance to the address set as msg.sender.

Tools Used

VS Code

Recommended Mitigation Steps

get contract's balance before and after the delegatecall, then finally reimburse up to the balance before the call.

[c4-judge]

Picodes marked the issue as duplicate of #277

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory