Nouns DAO - ihtishamsudo's results

[L-01] returnTokensToOwner should emit and event for numTokensInEscrow -= tokenIds.length

Description

Missing events for critical arithmetic parameters.

Impact

Without emitting an event for numTokensInEscrow it would be difficult to track numTokensInEscrow reduced by the number of tokenIds especially in off-chain procedures, eventually results in wrong calculation in numTokensInEscrow

Tools Used

Encephalon aka Brain

Vulnerable Code Link

NounsDAOForkEscrow.sol#L116-L125

Recommended Mitigation Steps

Event should be defined in the contract i.e

event TokensReturnedToOwner(address indexed owner, uint256 numTokens);

And should be emit in the function in a way i.e

function returnTokensToOwner(address owner, uint256[] calldata tokenIds) external onlyDAO {
        for (uint256 i = 0; i < tokenIds.length; i++) {
            if (currentOwnerOf(tokenIds[i]) != owner) revert NotOwner();

            nounsToken.transferFrom(address(this), owner, tokenIds[i]);
            escrowedTokensByForkId[forkId][tokenIds[i]] = address(0);
        }

        numTokensInEscrow -= tokenIds.length;
        // Emit that event for tracking changes
        emit TokensReturnedToOwner(owner, tokenIds.length);
    }

[L-02] Using block.timestamp for making decisions regarding governance could be problematic

Description

usage of block.timestamp. block.timestamp can be manipulated by miners.

Impact

Miners can manipulate the block timestamp to some extent. Which could Effect checkGovernanceActive() function

Tools Used

Solidity Visual Developer

Vulnerable Code Link

NounsDAOLogicV1Fork.sol#L377-L378 NounsDAOLogicV1Fork.sol#L377-L378

Recommended Mitigation Step

Avoid relying on block.timestamp.

[L-03] Nouns will not be able to be transferred once the block.number passes type(uint32).max

Description

Considering This Issue From Past Report Of Nouns DAO And Not Sure The Mitigation Is Done So Submitting Again, Feedback if mitigation is implemented because I have seen no changes.

Impact

While this currently equates to ~1260 years, if there’s a hard fork which makes block times much more frequent (e.g. to compete with Solana), then this limit may be reached much faster than expected, and transfers and delegations will remain stuck at their existing settings

Tools Used

Past Report

Vulnerable Code Link

NounsDAOLogicV2.sol#L984

Based On My Experience Auditing This Codebase I Will Answer The Analysis Questions.

[Q-01] Analysis of the codebase (What's unique? What's using existing patterns?)

The first thing I would like to mention is the complexity to clone this repo. Because of the symlinks to libraries and tests, everything in the scope couldn't be clones using git clone. For Instance, to understand this, Whenever you want to clone the repo that was available for audit, the script folder is not fetched in the local environment i.e. in IDE like VS Code. I have to manually add the script folder by downloading it from Repo and, for forge tests, Lib was missing obviously Lib wasn't fetched either while git cloning so had to manually import in my local environment, so it was kind of mess Setting up Repo in the local environment. Now, Here's the point what is unique in this codebase. The Nouns DAO is unique in its approach to NFT generation and governance. Unlike, An NFT Project, which has literally no use case just a random shit people wasting money on. Noun DAO is using attractive model For NFTs and a proper use case that is beneficial for creators and the people behind Noun DAO. One more thing I like about this codebase and project is that the new version introduces several unique features like proposal editing and Nouns Fork.

[Q-02] Architecture feedback

The arcihtecture, to be honest, seems to be well-structured with clear separation of concerns. Transparent proxy being used that allows Governance contract upgradability. But, transition from the current treasury to a new treasury could be a potential point of concern and needs to be handled carefully.And transition from the current treasury to a new treasury could be a potential point of concern and needs to be handled carefully. And I want to put some words about the Nouns DAO Fork it seems like the Nouns Fork mechanism is designed with a lot of thought towards protecting minority token holders and ensuring fair governance. The two-phase process Escrow and Forking periods allows for flexibility and gives token holders the ability to change their minds before the fork is finalized. However, the potential for an attacker to follow into the new forked DAO is a concern that might need further mitigation strategies. Hope so Many wardens, including me, have submitted many mitigations to improve this forking process.

[Q-03] Centralization risks

The DAO model inherently reduces centralization risks as it is governed by token holders. However, the introduction of the Nouns Fork feature could potentially lead to fragmentation of the community if a significant minority decides to exit the DAO.

[Q-04] Systemic risks

One systemic risk could be the potential for a malicious majority to force a minority to fork and then join the fork with many of their tokens, continuing to attack the minority in their new fork DAO.

[Q-05] Other recommendations

The transition from the current treasury to the new treasury should be handled with care to avoid any potential issues. Also, the potential issues with the Nouns Fork feature should be carefully considered and addressed.

[Q-06] How much time did you spend?

I roughly spent 20-22 hours on this codebase, wanted to spend some more time on this interesting codebase but we all have priorities. Will be glad to secure this protocol again in near future.

Time spent:

20 hours