Nouns Builder contest - zkhorse's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L131-L135 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L176-L190 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L192-L235

Vulnerability details

The owner of one or many ERC721Votes tokens can double their voting power once (and only once) by delegating to their own address as their first delegation.

Scenario

This exploit relies on the initial default value of the delegation mapping in ERC721Votes, which is why it will only work once per address.

First, the token owner must call delegate or delegateBySig, passing their own address as the delegate:

ERC721Votes#delegate

    /// @notice Delegates votes to an account
    /// @param _to The address delegating votes to
    function delegate(address _to) external {
        _delegate(msg.sender, _to);
    }

This calls into the internal _delegate function, with _from and _to both set to the token owner's address:

ERC721Votes#_delegate

    /// @dev Updates delegate addresses
    /// @param _from The address delegating votes from
    /// @param _to The address delegating votes to
    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
        address prevDelegate = delegation[_from];

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

Since this is the token owner's first delegation, the delegation mapping does not contain a value for the _from address, and prevDelegate on L#181 will be set to address(0):

ERC721Votes.sol#L180-L181

        // Get the previous delegate
        address prevDelegate = delegation[_from];

This function then calls into _moveDelegateVotes to transfer voting power. This time, _from is prevDelegate, equal to address(0); _to is the token owner's address; and _amount is balanceOf(_from), the token owner's current balance:

ERC721Votes#_moveDelegateVotes

 /// @dev Transfers voting weight
    /// @param _from The address delegating votes from
    /// @param _to The address delegating votes to
    /// @param _amount The number of votes delegating
    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
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

                // If this isn't a token burn:
                if (_to != address(0)) {
                    // Get the recipients's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_to]++;

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

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

                    // Update their voting weight
                    _writeCheckpoint(_to, nCheckpoints, prevTotalVotes, prevTotalVotes + _amount);
                }
            }
        }
    }

The if condition on L#203 is true, since _from is address(0), _to is the owner address, and _amount is nonzero:

ERC721Votes.sol#L202-L203

            // If voting weight is being transferred:
            if (_from != _to && _amount > 0) {

Execution skips the if block on L#205-217, since _from is address(0):

ERC721Votes.sol#L205-L217

                // 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
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

However, the if block on L#220-232 will execute and increase the voting power allocated to _to:

ERC721Votes.sol#L220-L232

                // If this isn't a token burn:
                if (_to != address(0)) {
                    // Get the recipients's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_to]++;

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

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

                    // Update their voting weight
                    _writeCheckpoint(_to, nCheckpoints, prevTotalVotes, prevTotalVotes + _amount);
                }

The token owner's voting power has now been increased by an amount equal to their total number of tokens, without an offsetting decrease.

This exploit only works once: if a token owner subsequently delegates to themselves after their initial self delegation, prevDelegate will be set to a non-default value in _delegate, and the delegation logic will work as intended.

Impact

Malicious ERC21Votes owners can accrue more voting power than they deserve. Especially malicious owners may quietly acquire multiple tokens before doubling their voting power. In an early DAO with a small supply of tokens, the impact of this exploit could be significant.

Recommendation

Make the delegates function public rather than external:

    /// @notice The delegate for an account
    /// @param _account The account address
    function delegates(address _account) public view returns (address) {
        address current = delegation[_account];
        return current == address(0) ? _account : current;
    }

Then, call this function rather than accessing the delegation mapping directly:

    /// @dev Updates delegate addresses
    /// @param _from The address delegating votes from
    /// @param _to The address delegating votes to
    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
        address prevDelegate = delegates(_from);

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

Note that the original NounsDAO contracts follow this pattern. (See here and here).

Test cases

(Put the following test cases in Gov.t.sol)

    function test_delegate_to_self_doubles_voting_power() public {
        mintVoter1();

        assertEq(token.getVotes(address(voter1)), 1);

        vm.startPrank(voter1);
        token.delegate(address(voter1));

        assertEq(token.getVotes(address(voter1)), 2);
    }

    function mintToken(uint256 tokenId) internal {
        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(tokenId);

        vm.warp(block.timestamp + auctionParams.duration + 1 seconds);
        auction.settleCurrentAndCreateNewAuction();
    }

    function test_delegate_to_self_multiple_tokens_doubles_voting_power() public {
        // An especially malicious user may acquire multiple tokens
        // before doubling their voting power through this exploit.
        mintVoter1();
        mintToken(3);
        mintToken(4);
        mintToken(5);
        mintToken(6);

        assertEq(token.getVotes(address(voter1)), 5);

        vm.prank(voter1);
        token.delegate(address(voter1));

        assertEq(token.getVotes(address(voter1)), 10);
    }

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

Lines of code

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

Vulnerability details

The "vetoer" role in NounsDAO governance is an especially powerful role intended as a check against governance attacks in the early stages of the DAO. In the original Nouns contracts, only the vetoer may renounce veto power or change the vetoer address:

NounsDAOLogicV1.sol

    /**
     * @notice Changes vetoer address
     * @dev Vetoer function for updating vetoer address
     */
    function _setVetoer(address newVetoer) public {
        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');

        emit NewVetoer(vetoer, newVetoer);

        vetoer = newVetoer;
    }

    /**
     * @notice Burns veto priviledges
     * @dev Vetoer function destroying veto power forever
     */
    function _burnVetoPower() public {
        // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');

        _setVetoer(address(0));
    }

However, in the Nouns Builder contracts, the vetoer may be burned or updated by governance:

Governor.sol#L593-L609

    /// @notice Updates the vetoer
    /// @param _newVetoer The new vetoer address
    function updateVetoer(address _newVetoer) external onlyOwner {
        if (_newVetoer == address(0)) revert ADDRESS_ZERO();

        emit VetoerUpdated(settings.vetoer, _newVetoer);

        settings.vetoer = _newVetoer;
    }

    /// @notice Burns the vetoer
    function burnVetoer() external onlyOwner {
        emit VetoerUpdated(settings.vetoer, address(0));

        delete settings.vetoer;
    }

This renders the vetoer role ineffective as protection against governance attacks, since governance itself may kick or change the vetoer.

Impact

A malicious majority may collude to kick or change the vetoer via governance as a prelude to a larger governance attack. Impact is mitigated since these changes must still be queued and processed through the governance timelock, but an attack is still possible in an early or low activity DAO without an active, vigilant vetoer.

Recommendation

Restrict updateVetoer and burnVetoer to the current vetoer rather than governance.

Low

DAO can be deployed with 100% allocation to founders

It's possible to deploy a DAO with an exact 100% allocation to the founders. If a DAO is deployed with these parameters, all auction bids will revert with out of gas errors, when they search for the next available unallocated token.

See the following test case:

// in Token.t.sol
function testHundredPercentFunderOwnership() public {
    // setup
    address[] memory wallets = new address[](1);
    uint256[] memory percents = new uint256[](1);
    uint256[] memory vestingEnds = new uint256[](1);

    wallets[0] = founder;
    percents[0] = 100;
    vestingEnds[0] = 4 weeks;

    setFounderParams(wallets, percents, vestingEnds);
    setMockTokenParams();
    setMockAuctionParams();
    setMockGovParams();
    deploy(foundersArr, tokenParams, auctionParams, govParams);

    vm.prank(founder);
    auction.unpause();

    address bidder = makeAddr("bidder");
    vm.prank(bidder);
    auction.createBid{ value: 1 ether }(2);

    // causes an infinite loop, hence eventually an out of gas error
    // [FAIL. Reason: EvmError: Revert] testHundredPercentFunderOwnership() (gas: 8660281895701201590)
}

Recommendation:

Change the following condition in Token#addFounders from:

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

to:

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

QA

Use string.concat

Solidity v0.8.12 introduced a string.concat function that is a little nicer than using string(abi.encodePacked(data)).

For example, this function in MetadataRenderer:

MetadataRenderer#_getItemImage

    /// @dev Encodes the reference URI of an item
    function _getItemImage(Item memory _item, string memory _propertyName) private view returns (string memory) {
        return
            UriEncode.uriEncode(
                string(
                    abi.encodePacked(ipfsData[_item.referenceSlot].baseUri, _propertyName, "/", _item.name, ipfsData[_item.referenceSlot].extension)
                )
            );
    }

May be rewritten using string.concat as:

    /// @dev Encodes the reference URI of an item
    function _getItemImage(Item memory _item, string memory _propertyName) private view returns (string memory) {
        return
            UriEncode.uriEncode(
                string.concat(
                    ipfsData[_item.referenceSlot].baseUri, _propertyName, "/", _item.name, ipfsData[_item.referenceSlot].extension
                )
            );
    }

There are a number of locations in MetadataRenderer where you may wish to replace string(abi.encodePacked()) with string.concat.

Shadowed variable

The _owner state variable inherited from Ownable is shadowed inside Manager#initialize:

Manager#initialize

    /// @notice Initializes ownership of the manager contract
    /// @param _owner The owner address to set (will be transferred to the Builder DAO once its deployed)
    function initialize(address _owner) external initializer {
        // Ensure an owner is specified
        if (_owner == address(0)) revert ADDRESS_ZERO();

        // Set the contract owner
        __Ownable_init(_owner);
    }

Consider renaming this parameter owner_ to avoid shadowing the inherited variable.

Omit unused virtual functions

Since the Nouns Builder contracts are meant to be deployed via factory contracts rather than extended as a library, you may wish to omit the unused virtual transfer hook functions in ERC721.

ERC721.sol

   /// @dev Hook called before a token transfer
    /// @param _from The sender address
    /// @param _to The recipient address
    /// @param _tokenId The ERC-721 token id
    function _beforeTokenTransfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal virtual {}

    /// @dev Hook called after a token transfer
    /// @param _from The sender address
    /// @param _to The recipient address
    /// @param _tokenId The ERC-721 token id
    function _afterTokenTransfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal virtual {}
}

Unnecessary low-level assembly

There are two usages of low-level assembly that may be replaced with Solidity equivalents.

Auction#_handleOutgoingTransfer

        // Used to store if the transfer succeeded
        bool success;

        assembly {
            // Transfer ETH to the recipient
            // Limit the call to 50,000 gas
            success := call(50000, _to, _amount, 0, 0, 0, 0)
        }

Recommended:

        (bool succes,) = payable(_to).call{ value: _amount, gas: 50000 }("");

Address#isContract

    /// @dev If an address is a contract
    function isContract(address _account) internal view returns (bool rv) {
        assembly {
            rv := gt(extcodesize(_account), 0)
        }
    }

Recommended:

    /// @dev If an address is a contract
    function isContract(address _account) internal view returns (bool rv) {
        return _account.code.length > 0;
    }

Omit unused modifier

The reinitializer modifier and internal _disableInitializers functions in Initializable appear to be unused. Consider removing these functions if they are not used in the codebase.

Initializable.sol

   /// @dev Enables initializer versioning
    /// @param _version The version to set
    modifier reinitializer(uint8 _version) {
        if (_initializing || _initialized >= _version) revert ALREADY_INITIALIZED();

        _initialized = _version;

        _initializing = true;

        _;

        _initializing = false;

        emit Initialized(_version);
    }

    ///                                                          ///
    ///                          FUNCTIONS                       ///
    ///                                                          ///

    /// @dev Prevents future initialization
    function _disableInitializers() internal virtual {
        if (_initializing) revert INITIALIZING();

        if (_initialized < type(uint8).max) {
            _initialized = type(uint8).max;

            emit Initialized(type(uint8).max);
        }
    }