search

code-423n4 / 2022-11-looksrare-findings

0 stars 0 forks source link

Public to all funds escape #277

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L22 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L34 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L27 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L108 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L109 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L245 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43

Vulnerability details

Description

The LooksRareAggregator smart contract implements a bunch of functions to escape funds by the contract owner (see rescueETH, rescueERC20, rescueERC721, and rescueERC1155). In this way, any funds that were accidentally sent to the contract or were locked due to incorrect contract implementation can be returned to the owner. However, locked funds can be rescued by anyone without the owner's permission. This is completely contrary to the idea of having rescue functions.

In order to withdraw funds from the contract, a user may just call the execute function in the ERC20EnabledLooksRareAggregator with tokenTransfers that contain the addresses of tokens to be withdrawn. Thus, after the order execution _returnERC20TokensIfAny and _returnETHIfAny will be called, and the whole balance of provided ERC20 tokens and Ether will be returned to msg.sender.

Please note, that means that the owner can be front-ran with rescue functions and an attacker will receive funds instead.

Impact

Useless of rescue functionality and vulnerability to jamming funds.

Recommended Mitigation Steps

_returnETHIfAny and _returnERC20TokensIfAny should return the amount of the token that was deposited.

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

Picodes commented 1 year ago

As only stuck funds are at risk, and as the aggregator contract itself is not supposed to handle funds, I don't think this qualify for High Severity

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as selected for report

0xhiroshi commented 1 year ago

We have decided that any ERC20 tokens sent there accidentally are free for all

c4-sponsor commented 1 year ago

0xhiroshi marked the issue as sponsor disputed

Picodes commented 1 year ago

Keeping the medium severity because the contract implements TokenRescuer, so the intent "that any ERC20 tokens sent there accidentally are free for all" totally make sense but wasn't clear prior the audit. So I consider this a case where tokens that should belong to the protocol could be withdrawn by anyone.

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory