Nouns DAO - shark's results

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L752-L759 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L793-L801

Vulnerability details

Impact

The implementation of a 'checkForDuplicates' function has not fully addressed the issue of sending a greater than prorata token share leading to loss of DAO funds. This is because this function does not adequately account for Double Entry Tokens – tokens which have multiple contract addresses representing different versions of the same token, as was the case with TUSD which famously caused a potential attack on Compound.

In the event of a DAO fork or members quitting, the presence of these Double Entry Tokens in the array could potentially send a greater than prorata share of those tokens to the fork DAO treasury or to the quitting members, which would lead to a loss of funds for the original DAO.

Proof of Concept

A single token, such as TUSD, could technically have multiple contract addresses if different versions of the token contract were deployed at different times. In such cases, the existing checkForDuplicates function may not accurately identify these as duplicate entries, since the contract addresses are different, even though they represent the same underlying asset.

Under this context, an Admin function for setting the list of ERC20 tokens to transfer on quit could still have duplicate ERC20 tokens entailed and undetected:

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L749-L759

    /**
     * @notice Admin function for setting the list of ERC20 tokens to transfer on `quit`.
     */
    function _setErc20TokensToIncludeInQuit(address[] calldata erc20tokens) external {
        if (msg.sender != admin) revert AdminOnly();
        checkForDuplicates(erc20tokens);

        emit ERC20TokensToIncludeInQuitSet(erc20TokensToIncludeInQuit, erc20tokens);

        erc20TokensToIncludeInQuit = erc20tokens;
    }

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L793-L801

    function checkForDuplicates(address[] calldata erc20tokens) internal pure {
        if (erc20tokens.length == 0) return;

        for (uint256 i = 0; i < erc20tokens.length - 1; i++) {
            for (uint256 j = i + 1; j < erc20tokens.length; j++) {
                if (erc20tokens[i] == erc20tokens[j]) revert DuplicateTokenAddress();
            }
        }
    }

Despite the low likelihood of this scenario, the impact could be high, resulting in a significant loss of funds. It is therefore categorized as a medium severity issue.

Recommended Mitigation Steps

Consider maintaining a registry of known token contract migrations and checking this registry when a token contract address is added. A safeguard could be introduced to treat two different addresses representing the same token as duplicates, thereby preventing the loss of funds.

While this may introduce a level of complexity to the contract, the potential to prevent a high-impact scenario like loss of DAO funds justifies this undertaking.

Assessed type

ERC20

Reserve price not fully taken care of

It is possible for an active auction to close at a price lower than the newly increased reserve price. This is undesirable especially when preventing a Noun auctioned off at the lower than expected price could be out of control in a bear market. Consider adding a check alleging that the contract balance needing to exceed the reserve price. Else, the last bidder will be refunded prior to having the Noun burned. Here's a refactored code logic that will take care of the suggestion.

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/NounsAuctionHouseFork.sol#L236-L256

/**
 * @notice Settle an auction, finalizing the bid and paying out to the owner.
 * @dev If there are no bids, the Noun is burned.
 */
function _settleAuction() internal {
    INounsAuctionHouse.Auction memory _auction = auction;

    require(_auction.startTime != 0, "Auction hasn't begun");
    require(!_auction.settled, 'Auction has already been settled');
    require(block.timestamp >= _auction.endTime, "Auction hasn't completed");

    auction.settled = true;

    // Check if contract balance is greater than reserve price
    if (address(this).balance < reservePrice) {
        // If contract balance is less than reserve price, refund to the last bidder
        if (_auction.bidder != address(0)) {
            _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
        }

        // And then burn the Noun
        nouns.burn(_auction.nounId);
    } else {
        if (_auction.bidder == address(0)) {
            nouns.burn(_auction.nounId);
        } else {
            nouns.transferFrom(address(this), _auction.bidder, _auction.nounId);
        }

        if (_auction.amount > 0) {
            _safeTransferETHWithFallback(owner(), _auction.amount);
        }
    }

    emit AuctionSettled(_auction.nounId, _auction.bidder, _auction.amount);
}

Spec document contradicts function logic

The spec is noted as saying,

https://hackmd.io/@el4d/nouns-dao-v3-spec

In this new version a proposal can also be cancelled if:

1. One of the accounts that signed on the proposal chooses to cancel, or
2. The sum of votes of all proposers no longer exceeds threshold.

However, it is implemented otherwise in the require statement of NounsDAOV3Proposals.cancel() below where signer can only cancel the proposal when the sum of votes of all proposers no longer exceeds threshold. Consider updating the document to correctly reflect what has been implemented.

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Proposals.sol#L595-L598

        require(
            msgSenderIsProposer || votes <= proposal.proposalThreshold,
            'NounsDAO::cancel: proposer above threshold'
        );

Code and comment mismatch

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L86

    @ audit 4,000 should be changed to 6,000
    uint256 public constant MAX_QUORUM_VOTES_BPS_UPPER_BOUND = 6_000; // 4,000 basis points or 60%

Spelling errors

There are numerous instances throughout the codebase in different contracts. Here's just one of the specific instances:

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L61

    @ audit setable should be changed to settable
    /// @notice The minimum setable proposal threshold

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L218

            @ audit arity should be changed to parity
            'NounsDAO::propose: proposal function information arity mismatch'

There are numerous instances throughout the codebase in different contracts. Here's just one of the specific instances:

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L901

     @ audit priviledges should be changed to privileges
     * @notice Burns veto priviledges

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV3.sol#L19

@ audit NounsDAOLogicV2.sol should be changed to NounsDAOLogicV3.sol
// NounsDAOLogicV2.sol is a modified version of Compound Lab's GovernorBravoDelegate.sol:

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOInterfaces.sol#L217

    @ audit the during of should be omitted
    /// @notice Emitted when the during of the forking period is set

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/token/base/ERC721CheckpointableUpgradeable.sol#L80-L84

    @ audit thats should be changed to that's
    /// @notice An event thats emitted when an account changes its delegate
    event DelegateChanged(address indexed delegator, address indexed fromDelegate, address indexed toDelegate);

    @ audit thats should be changed to that's
    /// @notice An event thats emitted when a delegate account's vote balance changes
    event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

Wrong adoption of block time

The following voting period constants are assuming 9.6 instead of 12 seconds per block. Depending on the sensitivity of lower and upper ranges desired, these may limit or shift the intended settable voting periods. For instance, using the supposed 12 second per block convention, the minimum and maximum settable voting periods should respectively be 7_200 and 100_800.

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L67-L71

    /// @notice The minimum setable voting period
    uint256 public constant MIN_VOTING_PERIOD = 5_760; // About 24 hours

    /// @notice The max setable voting period
    uint256 public constant MAX_VOTING_PERIOD = 80_640; // About 2 weeks

MIN_PROPOSAL_THRESHOLD_BPS is too low a value

In NounsDAOLogicV2.sol, NounsDAOV3Admin.sol, and NounsDAOLogicV1Fork.sol, the minimum proposal threshold can be set as low as 1 basis point.

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L61-L62 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Admin.sol#L111-L112 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol#L118-L119

    /// @notice The minimum setable proposal threshold
    uint256 public constant MIN_PROPOSAL_THRESHOLD_BPS = 1; // 1 basis point or 0.01%

This could pose a precision issue even if the total supply of Nouns tokens is already in its three digits. Apparently, a proposal threshold determined via the following two functions could return zero, e.g. (1 * 720) / 10000 yields zero due to truncation, i.e. the numerator smaller than the denominator.

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L921-L923

    function proposalThreshold() public view returns (uint256) {
        return bps2Uint(proposalThresholdBPS, nouns.totalSupply());
    }

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L1066-L1068

    function bps2Uint(uint256 bps, uint256 number) internal pure returns (uint256) {
        return (number * bps) / 10000;
    }

V3 upgrade could depreciate token value due to oversupply

The forking feature, though rarely happened as documented in the FAQs, could potentially affect the Nouns token value. This is because:

• a new fork will have its own DAO tokens daily generated and auctioned off through a new Auction House.
• new Nouns tokens claimed through escrow and during forking period will not have the original Nouns tokens burned. They are transferred to the original treasury or elsewhere as the DAO deems fit.

With a new fork competing with the original DAO for the daily auction, it will likely diverge the amount of ETH intended to go into bidding for the daily new NFTs at opposing ends. The situation could be worse in the far future if more forks were to transpire.

Nouns fork may not efficiently remedy a bad situation

The 20% threshold, as documented in the FAQs for instance, 800 * 0.2 = 160 of Nouns tokens, is not a small number comparatively in terms of the market cap. This translates to approximately 160 * 30 ETH * $2,000 almost equivalent to 10 million worth of USDC. For members wishing to dodge bad/undesirable proposals that aren't going to be vetoed, it's likely this will not materialize where the proposals get executed long before the threshold could be met to initiate a fork.

Consider conditionally reducing the threshold given that ragequit (or quitting) is going to happen regardless of the size of the fork. It is the forking group that could share the same goal and direction in a new DAO that matters.

Prolonged process due to updatable state

The introduction of updatePeriodEndBlock in V3 compared to the V1/V2 could unnecessarily prolong the entire proposal voting process.

Consider reducing the pending period to make room for the updatable period which should nonetheless be entailing a longer period still, albeit in a more reasonable sense.

Comments for the judge to contextualize your findings

I submitted 1 high, 4 mediums along with additional inputs in the Gas and QA reports. It's worth noting that loopholes still abound depending on how you would visualize the codebase both short and long terms. Here are the breakdown of the HM findings in their condensed forms:

1. Any signer can still have a pending/active/queue proposal cancelled to grief the proposal process A recently discovered vulnerability in a DAO system could allow a small malicious group to unilaterally cancel proposals, potentially leading to major disruptions and a lack of trust in the system. This flaw originates from the ability to transfer voting power to other addresses prior to invoking the cancel function. To exploit it, a group could first have their signatures included in a proposal, then delegate their votes to other addresses, decreasing the total vote count below the proposal threshold and enabling any signer to cancel the proposal. The issue could be mitigated by restricting proposal cancellation to the proposer or implementing a cooldown period or vote-locking mechanism to prevent signers from manipulating their voting power.
2. lastMinuteWindowInBlocks can be dodged to have undesirable proposal executed The report describes a vulnerability in the DAO system where a savvy minority could manipulate the voting process to push through proposals not in the majority's interests. They can achieve this by timing their votes to switch the proposal from defeated to successful just before the last-minute window period (ds.lastMinuteWindowInBlocks) can be triggered, preventing the initialization of the objection period (proposal.objectionPeriodEndBlock). Consequently, this could result in proposals being executed that do not benefit the majority of DAO members. The proposed mitigation step is to implement a monitoring system that alerts DAO members when the vote count nears the last-minute window threshold, allowing the majority to respond if an unexpected swing in votes occurs.
3. Inequitable distribution of funds when quitting DAO due to token opt-out The report highlights a vulnerability in the NounsDAOLogicV1Fork.sol contract, where participants who opt out of receiving certain ERC20 tokens when they quit the DAO do not receive any compensation for the value of these tokens. As a result, they receive less than their fair share of the DAO's funds, leading to an imbalance in fund distribution. This situation may deter potential participants, especially from jurisdictions where certain tokens are illegal. The code responsible for this is within the quitInternal function, where the balance to send is calculated but doesn't account for redistribution of the opted-out tokens. A suggested solution is to introduce a token swap function, which would let quitting users receive equivalent value in an accepted form, like USDC. This, however, could introduce new complexities and risks such as price oracle manipulation and sandwich attack risks.
4. Potential risks associated with Nouns Fork The NounsDAOLogicV1Fork.sol contract presents a vulnerability in the DAO's governance model where the departure of token holders decreases the adjustedTotalSupply, thereby lowering the proposal threshold and making it easier for remaining members to pass potentially harmful proposals. This could incite a vicious cycle of token holders departing in response to these proposals, progressively lowering the total supply and proposal threshold, eventually leading to a "ghost DAO" with few or no active participants. This can disrupt the purpose of the DAO, possibly leading to a decrease in token value and a decline in community involvement. The situation also undermines the new Auction House where new Nouns are minted and auctioned daily, which could see lower bids or no bidders at all. To mitigate this, it's suggested to implement a function logic that increases the proposal threshold as the total supply decreases, helping to maintain a healthy community.
5. Double entry tokens The checkForDuplicates function in the DAO contract doesn't adequately address the problem of Double Entry Tokens, where tokens have multiple contract addresses representing different versions of the same token. If a DAO fork or a member exit occurs, the presence of these tokens could lead to a greater than prorata share being sent to the fork DAO treasury or the quitting member, causing a loss of funds for the original DAO. Despite the low likelihood, the high impact of this issue, such as significant loss of funds, categorizes it as a medium severity issue. To mitigate this, it's suggested to maintain a registry of known token contract migrations and treat different addresses representing the same token as duplicates, thus preventing the potential loss of DAO funds.

Approach taken in evaluating the codebase

While the codebase is comparatively easy to understand, comprehending the true business logic and intention is not. I needed to go through the specs numerous times and eventually approached Spearbit's draft audit reports. That's where I started seeing some vulnerabilities that might not have been fully mitigated. And as I worked on better fixes, other vulnerabilities, both mediums and lows, began to surface.

Architecture recommendations

There are quite a number of redundant codes that I have reported in the Gas report. There should be more of similar issues to detect and suggest. One of them that I have not included in the report, for instance, is the long list of identical events in NounsDAOV3Admin.sol that could have been skimmed considering these events have been included in the import ./NounsDAOInterfaces.sol.

Codebase quality analysis

The codebase is overall well structured with ample NatSpec comments attached, facilitating the need to link up all needed flows and function calls. It would have been better had some form of flow charts been provided. This would be crucially helpful to someone who has limited background of the protocol and the business intentions entailed.

Centralization risks

There shouldn't be much concern here considering the protocol is so much DAO based where almost all changes would need to go through the governance except for a simple bypass where the treasury gets to migrate funds to the new executor without the need going through a proposal. Nonetheless, much power still lies in the hands of vetoers in the V3 logic, a feature that has sensibly been removed in the forked DAO.

Mechanism review

Upon reviewing the DAO's mechanisms, it is clear that several aspects could pose vulnerabilities, especially in light of the planned DAO fork. The DAO’s governance model and mechanisms can be exposed to different forms of manipulation such as 'Griefing the proposal process' and 'Manipulation of last-minute voting'. This could lead to an imbalance in power and decision-making, potentially resulting in proposals being passed that are not in the best interest of the community at large.

Additionally, the management of ERC20 tokens, specifically the handling of Double Entry Tokens and equitable distribution of funds when a member quits, reveal the need for improvement. Current mechanisms do not fully cater to the unique challenges of these scenarios, potentially leading to the loss of DAO funds or an inequitable distribution of funds.

Lastly, the potential systemic risks that arise with the DAO fork should not be overlooked. While it represents an innovative and bold step in DAO governance, it introduces complexities and risks such as the decrease in proposal thresholds and a potential devaluation of tokens.

It is thus highly recommended to review and revise these mechanisms, keeping in mind the lessons learned from the reported vulnerabilities and recommendations provided to ensure a more robust and resilient DAO system.

Systemic risks

Nouns DAO is laudable for the first ever DAO fork not seen in the industry. Nonetheless, the rush to deliver V3 and its fork could potentially bring in issues and threats other than the known ones, as I have reported in my findings. Ultimately, it all boils down to sustainability of both the original and forked DAO amidst the tradeoffs in possibly diluting the token values due to foreseeable increased supply collectively.

Time spent:

50 hours