code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/TokenRescuer.sol#L14-L39 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/OwnableTwoSteps.sol#L125-L127

Vulnerability details

Impact

Contract TokenRescuer inherits the contract OwnableTwoSteps but does not set any delay. There should be a constructor in TokenRescuer responsible for setting the delay as described in the comments of the OwnableTwoSteps contract. Without any delay, ownership can be changed/revoked within a single block.

Proof of Concept

contract TokenRescuer is OwnableTwoSteps, LowLevelETH, LowLevelERC20Transfer {
    error InsufficientAmount();

   // @audit: Missing constructor/ call to _setupDelayForRenouncingOwnership

    function rescueETH(address to) external onlyOwner
    function rescueERC20(address currency, address to) external onlyOwner
}

Tools Used

forge

Recommended Mitigation Steps

Add a constructor to TokenRescuer setting a delay of 1 day for ownership changes/revoking.

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-sponsor commented 1 year ago

0xhiroshi marked the issue as disagree with severity

0xhiroshi commented 1 year ago

valid but it sounds like a low risk to me (also we are going to time delay completely)

Picodes commented 1 year ago

To me this is a case of "break of functionality" where the intent of the code is clearly to have a two step ownership transfer but it is not. As this safeguard may be important for decision-making (e.g. giving an unlimited approval to ERC20EnabledLooksRareAggregator), I think Medium severity is appropriate.

0xhiroshi commented 1 year ago

To me this is a case of "break of functionality" where the intent of the code is clearly to have a two step ownership transfer but it is not. As this safeguard may be important for decision-making (e.g. giving an unlimited approval to ERC20EnabledLooksRareAggregator), I think Medium severity is appropriate.

Ok

C4-Staff commented 1 year ago

JeeberC4 marked the issue as duplicate of #127

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