Nouns Builder contest - ChristianKuri's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L244-L260 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L204-L237 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L143-L162 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L177-L199

Vulnerability details

Impact

When the founder creates the DAO, he can assign himself any percentage of the generated tokens from 0 - 100%. If the founder selects to have the 100% of the tokens it will break the application.

Proof of Concept

A user creates a DAO and assigns himself the 100% of the tokens. To start receiving the tokens he unpauses the auction, which transfers the ownership of the auction to the treasury and creates the first auction.

When the first auction is created the Auction contract mints a token using token.mint(), in that function at line 152 it stats a do while, that do while checks if that token id is for founder (this will be always true). If thats true the auction mints the token, and repeats the process until it finds a token that is not for the founder. Since all the tokens are for the founder, it never stops minting. This will create an infinite loop and the transaction will fail.

At the end the DAO wont work for him because the auction will never be able to create a new token. This will make him lose trust in the DAO builder and he will probably not use it again.

auction/Auction.sol#L244-L260 & #L204-L237

File: auction/Auction.sol#L244-L260

      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();
          }
          // Else if the contract was paused and the previous auction was settled:
          else if (auction.settled) {
              // Start the next auction
              _createAuction();
          }
      }

File: auction/Auction.sol#L204-L237

      function _createAuction() private {
        // Get the next token available for bidding
        try token.mint() returns (uint256 tokenId) {
            // Store the token id
            auction.tokenId = tokenId;

            // Cache the current timestamp
            uint256 startTime = block.timestamp;

            // Used to store the auction end time
            uint256 endTime;

            // Cannot realistically overflow
            unchecked {
                // Compute the auction end time
                endTime = startTime + settings.duration;
            }

            // Store the auction start and end time
            auction.startTime = uint40(startTime);
            auction.endTime = uint40(endTime);

            // Reset data from the previous auction
            auction.highestBid = 0;
            auction.highestBidder = address(0);
            auction.settled = false;

            emit AuctionCreated(tokenId, startTime, endTime);

            // Pause the contract if token minting failed
        } catch Error(string memory) {
            _pause();
        }
    }

token/Token.sol#L143-L162 & #L177-L199

File: token/Token.sol#L143-L162

    function mint() external nonReentrant returns (uint256 tokenId) {
        // Cache the auction address
        address minter = settings.auction;

        // Ensure the caller is the auction
        if (msg.sender != minter) revert ONLY_AUCTION();

        // Cannot realistically overflow
        unchecked {
            do {
                // Get the next token to mint
                tokenId = settings.totalSupply++;

                // Lookup whether the token is for a founder, and mint accordingly if so
            } while (_isForFounder(tokenId));
        }

        // Mint the next available token to the auction house for bidding
        _mint(minter, tokenId);
    }


File: token/Token.sol#L177-L199

    function _isForFounder(uint256 _tokenId) private returns (bool) {
        // Get the base token id
        uint256 baseTokenId = _tokenId % 100;

        // If there is no scheduled recipient:
        if (tokenRecipient[baseTokenId].wallet == address(0)) {
            return false;

            // Else if the founder is still vesting:
        } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
            // Mint the token to the founder
            _mint(tokenRecipient[baseTokenId].wallet, _tokenId);

            return true;

            // Else the founder has finished vesting:
        } else {
            // Remove them from future lookups
            delete tokenRecipient[baseTokenId];

            return false;
        }
    }

Recommended Mitigation Steps

If the founder wants to have the 100% of the tokens, it should ask him how many tokens he wants, and then it should mint him that amount of tokens. If later he wants to mint more tokens he can start the auction but now he wont maintain the 100% of the tokens. In this case if the auction is started, the auction wont mint him any more tokens. Basically adding the functionality to mint a specific amount of tokens instead of a percent.

Tools Used

VSCode and Manual Analysis

[L-01] Front-runnable initializer

If the initializer is not executed in the same transaction as the constructor, a malicious user can front-run the initialize() call. In the best case, the owner is forced to waste gas and re-deploy. In the worst case, the owner does not notice that his/her call reverts, and everyone starts using a contact that the attacker would have control of (the manager) since he will be the owner.

If the manager contract is initialized before upgrading to v2, the manager proxy would also have incorrect values, and all the contract for the logic of the token, auction, governance, treasury and metadata would have an incorrect manager address. Making the deployer deploy everything again.

manager/Manager.sol#L80-L86

File: manager/Manager.sol   #1

80    function initialize(address _owner) external initializer {
81        // Ensure an owner is specified
82        if (_owner == address(0)) revert ADDRESS_ZERO();
83
84        // Set the contract owner
85        __Ownable_init(_owner);
86    }

[L-02] Add address(0) check to all the addresses that are passed in all the initializers

The Treasury initialize() function have a check for address(0) in the governor param. treasury/Treasury.sol#L43-L60

File: treasury/Treasury.sol#L47-L48

47    // Ensure a governor address was provided
48    if (_governor == address(0)) revert ADDRESS_ZERO();

The Governor initialize() function have a check for address(0) in the treasury and token params. governor/Governor.sol#L57-L87

File: governor/Governor.sol#L69-L71

69    // Ensure non-zero addresses are provided
70    if (_treasury == address(0)) revert ADDRESS_ZERO();
71    if (_token == address(0)) revert ADDRESS_ZERO();

To keep consistency, all the initializers should have address(0) check as well.

token/Token.sol#L43 metadata/MetadataRenderer.sol#L45 auction/Auction.sol#L54

[L-03] A Paused Auction that already started can still be bid on and settled

The auction can be paused and then started again. If the auction is paused it can be bid on and settled. I consider that if an auction is paused it should not allow anyone to bid on it or settle it. When the auction is unpaused it should increment the auction duration by the duration of the pause, this way the auction will be correctly paused.

auction/Auction.sol#L90-L127 auction/Auction.sol#L268-L270

[L-04] Auction duration can be a really low number

The auction duration can be set to a really low number, this can cause the auction to end before anyone can interact with it. The auction should have a minimum duration. When the auction finishes and settles it burns the token if no one bid, but keeps increasing the totalSupply of the token.

[N-01] Return value could confuse developers

When calling a uint++ the value that is returned is the previous one.

token/Token.sol#L154

File: token/Token.sol   #1

/// Some developers could thing that founderId is the new value from settings.numFounders (1)
/// But instead in the first iteration founderId will be 0
uint256 founderId = settings.numFounders++;

My recommendation is do it this way.

uint256 founderId = settings.numFounders;
settings.numFounders++;

[N-02] Modulus after the set is not needed

The % after the assignment is not necessary, it does not do anything. Also the Schedule cannot be greater than 100.

token/Token.sol#L118

(baseTokenId += schedule) % 100;