Nouns Builder contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L101

Vulnerability details

Impact

getPastVotes() uses the binary search algorithm, which returns the result if it encounters the same timestamp, and since timestamps are not unique in the array The number of votes can be controlled by adding checkpoints to locate any one of the same timestamps.

Proof of Concept

example:

1. When block.timestamp = t1, several accounts in the same transaction transfer token to each other (or set proxy) to generate multiple checkpoints of t1, and submit a proposal at the same time proposal.timeCreated = t1

user1: timestamp | votes

.. . .. . t1 0 t1 1 t1 0

user2:

timestamp | votes

.. . .. . t1 1 t1 0 t1 1

user3: .. user4: ..

2.when voting , before vote , increase the user's checkpoints to control "high", so that you can control "middle", you can freely select the votes = 1 record in checkpoints (user actually has no votes)

function getPastVotes(address _account, uint256 _timestamp) public view returns (uint256) {
       ...
            while (high > low) {

                middle = high - (high - low) / 2;  //****control hight,then control middle****//
                cp = accountCheckpoints[middle];
                if (cp.timestamp == _timestamp) {
                    // Return the voting weight
                    return cp.votes;

3. freely select the votes = 1 record in checkpoints of timestamp = t1

    function _castVote(
        bytes32 _proposalId,
        address _voter,
        uint256 _support,
        string memory _reason
    ) internal returns (uint256) {
    ..
    ..
    weight = getVotes(_voter, proposal.timeCreated); //**control weight**//
    ...

Recommended Mitigation Steps

Ensure that the timestamp of checkpoints is unique, _writeCheckpoint() overwrites the last one if it is the same timestamp

Reference：

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/Comp.sol#L265

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/Comp.sol#L247

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L144

Vulnerability details

Impact

when call delegateBySig() , delegatee set to address(0), the delegatee is still itself, but user Checkpoints.votes will be reduced. it will cause the NFT of the user being delegate to be locked, can no set other delegatee or NFT transfer

Proof of Concept

Example:

1. user_A has 1 NFT, user_B has 1 NFT, and user_B's delegate is user_A, then user_A's Checkpoints.votes=2
2. user_A call delegateBySig(address(0)), after user_A 's Checkpoints.votes=1
3. user_A transfers its own NFT to user_C, then Checkpoints.votes=0
4. then user_B can not modify the delegatee, NFT can not be transferred,be lock Checkpoints.votes - 1 will underflows

    function _moveDelegateVotes(
        address _from,
        address _to,
        uint256 _amount
    ) internal {
        unchecked {
            // If voting weight is being transferred:
            if (_from != _to && _amount > 0) {
                // If this isn't a token mint:
                if (_from != address(0)) {
                    // Get the sender's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_from]++;

                    // Used to store the sender's previous voting weight
                    uint256 prevTotalVotes;

                    // If this isn't the sender's first checkpoint: Get their previous voting weight
                    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

                    // Update their voting weight
                    //**** (prevTotalVotes - _amount) will underflows ****//
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

Tools Used

Recommended Mitigation Steps

delegateBySig() add check to !=address(0)

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L194

Vulnerability details

Impact

MetadataRenderer.sol addProperties() can add any number of properties, in token mint() will generate tokenAttributes based on properties, but tokenAttributes = uint16[16] If there are more than 15 properties it will cause an overflow and token will never to mint()

Proof of Concept

    function addProperties(
        string[] calldata _names,
        ItemParam[] calldata _items,
        IPFSGroup calldata _ipfsGroup
    ) external onlyOwner {
        ...
        unchecked {
            // For each new property:
            for (uint256 i = 0; i < numNewProperties; ++i) {
                // Append storage space
                properties.push();   //***** any number ****///

    function onMinted(uint256 _tokenId) external returns (bool) {


    	uint256 numProperties = properties.length;

        uint16[16] storage tokenAttributes = attributes[_tokenId];  //*** max 16 ***//


        unchecked {
            // For each property:
            for (uint256 i = 0; i < numProperties; ++i) {
                // Get the number of items to choose from
                uint256 numItems = properties[i].items.length;

                // Use the token's seed to select an item
                tokenAttributes[i + 1] = uint16(seed % numItems); //**** when properties.lenght > 15 will raise "Index out of bounds" ****//

                // Adjust the randomness
                seed >>= 16;
            }
        }        
 

    }

    function _mint(address _to, uint256 _tokenId) internal override {
        ...

        //*** mint will always fail ***//
        if (!settings.metadataRenderer.onMinted(_tokenId)) revert NO_METADATA_GENERATED();

    }

Tools Used

Recommended Mitigation Steps

addProperties() check length <= 15

    function addProperties(
        string[] calldata _names,
        ItemParam[] calldata _items,
        IPFSGroup calldata _ipfsGroup
    ) external onlyOwner {

    ...

+++   require(properties.length<=15, "too much properties")

   }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L207

Vulnerability details

Impact

token.sol totalSupply does not reduce the number when burn, resulting in inaccurate totalSupply and incorrect vote approval rate.

Proof of Concept

when mint() totalSupply++, but when burn not totalSupply-- , resulting in incorrect percentage of votes

    function mint() external nonReentrant returns (uint256 tokenId) {
    	...
    	...
        unchecked {
            do {
                // Get the next token to mint
                tokenId = settings.totalSupply++;  //****** 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);
    }    

    function burn(uint256 _tokenId) external {
        // Ensure the caller is the auction house
        if (msg.sender != settings.auction) revert ONLY_AUCTION();


        //***** without settings.totalSupply-- *****//
        // Burn the token
        _burn(_tokenId); 
    }

causes proposalThreshold() and quorum() to be inaccurate

    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

Tools Used

Recommended Mitigation Steps

function burn(uint256 _tokenId) external {
    // Ensure the caller is the auction house
    if (msg.sender != settings.auction) revert ONLY_AUCTION();

    // Burn the token
+++ settings.totalSupply--;    
    _burn(_tokenId);
}

    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++;  
+++             tokenId = settings.currentTokenId++;              
+++             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);
    }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L128

Vulnerability details

Impact

When the initial deployment, totalSupply = 0, if the proposal is submitted at this time maliciously, it can be submitted directly, while proposal.quorumVotes = 0 Since quorumVotes = 0 may be able to enter the execution queue without voting

Proof of Concept

when totalSupply = 0

    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {

        unchecked {
        	//**** totalSupply = 0  pass without votes ****///
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }


        proposal.quorumVotes = uint32(quorum());  //**** quorumVotes = 0 ****//

Tools Used

Recommended Mitigation Steps

propose() add

if (totalSupply < MIN_SUPPLY) revert()

Judge has assessed an item in Issue #409 as Medium risk. The relevant finding follows:

1.Auction.sol unpause() Judgment whether the first auction by tokenId = 0, but there tokenId can be equal to 0, it is recommended to add judgment startTime == 0

    function unpause() external onlyOwner {
        _unpause();

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

            // Start the first auction
            _createAuction();
        }

2. Token.sol _addFounders() Need to check ownershipPct <100,don't check uint8(founderPct) after conversion to compare

    function _addFounders(IManager.FounderParams[] calldata _founders) internal {
        // Cache the number of founders
        uint256 numFounders = _founders.length;

        // Used to store the total percent ownership among the founders
        uint256 totalOwnership;

        unchecked {
            // For each founder:
            for (uint256 i; i < numFounders; ++i) {
                // Cache the percent ownership
                uint256 founderPct = _founders[i].ownershipPct;

                // Continue if no ownership is specified
                if (founderPct == 0) continue;

                // Update the total ownership and ensure it's valid
---             if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();
+++             if ((totalOwnership += founderPct) > 100) revert INVALID_FOUNDER_OWNERSHIP();