search

code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

An owner can accidentally (or not?) delete the first auction #690

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L247-L254 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L206-L208 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L154

Vulnerability details

Impact

Token ids start from 0, however, this value is also checked against when unpausing the contract: 

  function unpause() external onlyOwner {
    _unpause();

    // If this is the first auction:
    if (auction.tokenId == 0) {
        // Transfer ownership of the contract to the DAO
        transferOwnership(settings.treasury);

        // Start the first auction
        _createAuction();
    }
    ...
  }

If the owner unpauses the auction twice, the second time the value of auction.tokenId is again 0 meaning the check will pass and re-create the auction again even though the current auction might have active bids. It is highly likely because I think the highest chances of unpausing are initially if something does not go according to the plan or an owner wants to adjust some parameters before continuing the sale. This could happen accidentally and the current highest bid and NFT #0 will be lost or this could be a griefing attack by the malicious owner which I suppose is very unlikely because the owner has too many privileges to exploit the system anyway. Yet I think this is a significant flaw in the codebase that should be prevented and deserves a severity of Medium.

Proof of Concept

PoC of a simplified version: https://gist.github.com/pauliax/07ba76afb89a431f78eadef488e296e0

Steps to reproduce are in the gist's comments section.

Recommended Mitigation Steps

Token ids should either not start from 0 or you should enhance this check, e.g. if (auction.tokenId == 0 && auction.startTime == 0).

GalloDaSballo commented 1 year ago

Dup of #376