Nouns DAO - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] initialize() can be called again if timelock is set to 0

The initialize() function in NounsDAOLogicV3 doesn't have an initializer modifier. Instead it checks address(ds.timelock) != address(0). This would be a good practice to only initialize once. However the admin can set the timelock address in _setTimelocksAndAdmin. Since there is no 0 address check, the admin can set the timelock to 0 by mistake. When that happens, the NounsDAOLogicV3 can be initialized again.

    function initialize(
        address timelock_,
        address nouns_,
        address forkEscrow_,
        address forkDAODeployer_,
        address vetoer_,
        NounsDAOParams calldata daoParams_,
        DynamicQuorumParams calldata dynamicQuorumParams_
    ) public virtual {
        if (address(ds.timelock) != address(0)) revert CanOnlyInitializeOnce();
        ...
    }

    function _setTimelocksAndAdmin(
        NounsDAOStorageV3.StorageV3 storage ds,
        address timelock,
        address timelockV1,
        address admin
    ) external onlyAdmin(ds) {
        ds.timelock = INounsDAOExecutorV2(timelock);
        ds.timelockV1 = INounsDAOExecutor(timelockV1);
        ds.admin = admin;

        emit TimelocksAndAdminSet(timelock, timelockV1, admin);
    }

[L-02] Admin can be set directly using _setTimelocksAndAdmin()

The NounsDAOExecutor has a 2 step admin transfer with the functions setPendingAdmin() and acceptAdmin(). A 2-step ownership pattern is a good practice. However this can be bypassed when the admin calls _setTimelocksAndAdmin(). This function allows to set the admin directly, which eliminates the purpose of the other 2 functions.

    function _setTimelocksAndAdmin(
        NounsDAOStorageV3.StorageV3 storage ds,
        address timelock,
        address timelockV1,
        address admin
    ) external onlyAdmin(ds) {
        ds.timelock = INounsDAOExecutorV2(timelock);
        ds.timelockV1 = INounsDAOExecutor(timelockV1);
        ds.admin = admin;

        emit TimelocksAndAdminSet(timelock, timelockV1, admin);
    }

Consider removing the option to set an admin directly.

Non critical

[N-01] Typos

• NounsDAOLogicV2.sol should be NounsDAOLogicV3.sol

[N-02] Missing max value for lastMinuteWindowInBlocks

When setting the lastMinuteWindowInBlocks, there is no min or max settable value. Missing a min value isn't a problem in this case. However missing a max value can lead to really long waiting times if it's set wrong. Consider adding a max value of a few days.

    function _setLastMinuteWindowInBlocks(NounsDAOStorageV3.StorageV3 storage ds, uint32 newLastMinuteWindowInBlocks)
        external
        onlyAdmin(ds)
    {
        uint32 oldLastMinuteWindowInBlocks = ds.lastMinuteWindowInBlocks;
        ds.lastMinuteWindowInBlocks = newLastMinuteWindowInBlocks;

        emit LastMinuteWindowSet(oldLastMinuteWindowInBlocks, newLastMinuteWindowInBlocks);
    }

[N-03] Be consistent with using if and require

The NounsDAOV3Admin is not consistent when setting the variables and checking the min and max values. Some functions use if and some require. It can be confusing when you switch them up so it's best to always use the same.

L387:
    require(
        newMaxQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS_UPPER_BOUND,
        'NounsDAO::_setMaxQuorumVotesBPS: invalid max quorum votes bps'
    );
L438:
    if (
        newMinQuorumVotesBPS < MIN_QUORUM_VOTES_BPS_LOWER_BOUND ||
        newMinQuorumVotesBPS > MIN_QUORUM_VOTES_BPS_UPPER_BOUND
    ) {
        revert InvalidMinQuorumVotesBPS();
    }

[N-04] Ethereum gas costs might change in the future

When a vote gets refunded, there is a REFUND_BASE_GAS of 36000 gas. This includes 7K for ETH transfer and 29K for general transaction overhead. However gas costs might change in the future. Leading to more or less refunding than necessary. A mitigation is to remove the so the admin can change the value.