Nouns DAO - MohammedRizwan's results

Summary

Low Risk Issues

[L‑01] Misleading comment on timelock_ contract address

In NounsDAOLogicV1Fork.sol, timelock_ address comment is misleading with respect to function used in contract. It says,

156     * @param timelock_ The address of the NounsDAOExecutor

However, NounsDAOExecutor do not have sendERC20() and sendETH() functions which are used in NounsDAOLogicV1Fork.sol. Therefore the comment needs to be corrected with below recommendation.

Recommended Mitigation steps

-     * @param timelock_ The address of the NounsDAOExecutor
+     * @param timelock_ The address of the NounsDAOExecutorV2

[L‑02] Missing events in initialize() function

In NounsDAOExecutorV2.sol, initialize() function can only be called once. The state variables are set inside the initialize() must emit event for everyones information and verifications. It should log events. Add event indicating the delay and admin state variables.

There is 1 instance of this issue: In NounsDAOExecutorV2.sol at L95,

File: nouns-contracts/contracts/governance/NounsDAOExecutorV2.sol

    function initialize(address admin_, uint256 delay_) public virtual initializer {
        require(delay_ >= MINIMUM_DELAY, 'NounsDAOExecutor::constructor: Delay must exceed minimum delay.');
        require(delay_ <= MAXIMUM_DELAY, 'NounsDAOExecutor::setDelay: Delay must not exceed maximum delay.');

        admin = admin_;
        delay = delay_;
    }

[L‑03] Prevent constructor from setting admin address as address(0)

admin address can not be address(0). Add zero address check inside constructor.

File: nouns-contracts/contracts/governance/NounsDAOExecutor.sol

    constructor(address admin_, uint256 delay_) {
        require(delay_ >= MINIMUM_DELAY, 'NounsDAOExecutor::constructor: Delay must exceed minimum delay.');
        require(delay_ <= MAXIMUM_DELAY, 'NounsDAOExecutor::setDelay: Delay must not exceed maximum delay.');

        admin = admin_;
        delay = delay_;
    }

Recommended Mitigation steps

File: nouns-contracts/contracts/governance/NounsDAOExecutor.sol

    constructor(address admin_, uint256 delay_) {
+        require(admin != address(0), "invalid address");
        require(delay_ >= MINIMUM_DELAY, 'NounsDAOExecutor::constructor: Delay must exceed minimum delay.');
        require(delay_ <= MAXIMUM_DELAY, 'NounsDAOExecutor::setDelay: Delay must not exceed maximum delay.');

        admin = admin_;
        delay = delay_;
    }

[L‑04] Add event for NounsDAOLogicV1Fork.initialize()

In NounsDAOLogicV1Fork.initialize(), it doesn not emit the event which must be emitted. Events are the cheapest to store data on blockchain. Recommend to add same in initialize function.

There is 1 instance of this issue: In NounsDAOLogicV1Fork.initialize() at L-165

Gas Optimizations

[G‑01] Instead of repeated code for access validation, use modifier to save gas

For functions access the access code is getting repeated which means more bytes ultimatly costly more gas. Recommend to use modifier.

There are 15 instances of this issue: 3 Instances in NounsDAOExecutor.sol

5 Instances in NounsDAOExecutorV2.sol

7 Instances in NounsDAOLogicV1Fork.sol

Recommended Mitigation steps

Add modifier for admin accessed functions.

For example:

File: nouns-contracts/contracts/governance/NounsDAOExecutorV2.sol

+    modifier onlyAdmin(){
+       require(msg.sender == admin, 'NounsDAOExecutor::sendERC20: Call must come from admin.');
+       _;
+   }


    function sendERC20(
        address recipient,
        address erc20Token,
        uint256 tokensToSend
-    ) external {
+    ) external onlyAdmin {
-       require(msg.sender == admin, 'NounsDAOExecutor: Call must come from admin.');
        IERC20(erc20Token).safeTransfer(recipient, tokensToSend);

        emit ERC20Sent(recipient, erc20Token, tokensToSend);
    }

[G‑02] In NounsDAOInterfaces.sol, Structs can be packed into fewer storage slots

In NounsDAOInterfaces.sol, Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

Total Number of Slots saved = 8 slots

There are 6 instances of this issue:

1. Instance 1 at L-311,

Instance 1 Struct can be packed to save lots of gas as below,

File: nouns-contracts/contracts/governance/NounsDAOInterfaces.sol

    struct Proposal {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice the ordered list of target addresses for calls to be made
        address[] targets;
        /// @notice The ordered list of values (i.e. msg.value) to be passed to the calls to be made
        uint256[] values;
        /// @notice The ordered list of function signatures to be called
        string[] signatures;
        /// @notice The ordered list of calldata to be passed to each call
        bytes[] calldatas;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been vetoed
        bool vetoed;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
        /// @notice Receipts of ballots for the entire set of voters
        mapping(address => Receipt) receipts;
    }

2. Instance 2 at L-411,

Instance 2 Struct can be packed to save lots of gas as below,

File: nouns-contracts/contracts/governance/NounsDAOInterfaces.sol

411    struct Proposal {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice the ordered list of target addresses for calls to be made
        address[] targets;
        /// @notice The ordered list of values (i.e. msg.value) to be passed to the calls to be made
        uint256[] values;
        /// @notice The ordered list of function signatures to be called
        string[] signatures;
        /// @notice The ordered list of calldata to be passed to each call
        bytes[] calldatas;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been vetoed
        bool vetoed;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
        /// @notice Receipts of ballots for the entire set of voters
        mapping(address => Receipt) receipts;
        /// @notice The total supply at the time of proposal creation
        uint256 totalSupply;
        /// @notice The block at which this proposal was created
        uint256 creationBlock;
452    }

3. Instance 3 at L-508,

Instance 3 Struct can be packed to save lots of gas as below,

File: nouns-contracts/contracts/governance/NounsDAOInterfaces.sol

508    struct ProposalCondensed {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The minimum number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been vetoed
        bool vetoed;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
        /// @notice The total supply at the time of proposal creation
        uint256 totalSupply;
        /// @notice The block at which this proposal was created
        uint256 creationBlock;
539    }

4. Instance 4 at L-653,

Instance 4 Struct can be packed to save lots of gas as below,

File: nouns-contracts/contracts/governance/NounsDAOInterfaces.sol

653    struct StorageV3 {
        // ================ PROXY ================ //
        /// @notice Administrator for this contract
        address admin;
        /// @notice Pending administrator for this contract
        address pendingAdmin;
        /// @notice Active brains of Governor
        address implementation;
        // ================ V1 ================ //
        /// @notice Vetoer who has the ability to veto any proposal
        address vetoer;
        /// @notice The delay before voting on a proposal may take place, once proposed, in blocks
        uint256 votingDelay;
        /// @notice The duration of voting on a proposal, in blocks
        uint256 votingPeriod;
        /// @notice The basis point number of votes required in order for a voter to become a proposer. *DIFFERS from GovernerBravo
        uint256 proposalThresholdBPS;
        /// @notice The basis point number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed. *DIFFERS from GovernerBravo
        uint256 quorumVotesBPS;
        /// @notice The total number of proposals
        uint256 proposalCount;
        /// @notice The address of the Nouns DAO Executor NounsDAOExecutor
        INounsDAOExecutorV2 timelock;
        /// @notice The address of the Nouns tokens
        NounsTokenLike nouns;
        /// @notice The official record of all proposals ever proposed
        mapping(uint256 => Proposal) _proposals;
        /// @notice The latest proposal for each proposer
        mapping(address => uint256) latestProposalIds;
        // ================ V2 ================ //
        DynamicQuorumParamsCheckpoint[] quorumParamsCheckpoints;
-       /// @notice Pending new vetoer
-       address pendingVetoer;
        // ================ V3 ================ //
        /// @notice user => sig => isCancelled: signatures that have been cancelled by the signer and are no longer valid
        mapping(address => mapping(bytes32 => bool)) cancelledSigs;
+       /// @notice Pending new vetoer
+       address pendingVetoer;
        /// @notice The number of blocks before voting ends during which the objection period can be initiated
        uint32 lastMinuteWindowInBlocks;
        /// @notice Length of the objection period in blocks
        uint32 objectionPeriodDurationInBlocks;
        /// @notice Length of proposal updatable period in block
        uint32 proposalUpdatablePeriodInBlocks;
        /// @notice address of the DAO's fork escrow contract
        INounsDAOForkEscrow forkEscrow;
        /// @notice address of the DAO's fork deployer contract
        IForkDAODeployer forkDAODeployer;
        /// @notice ERC20 tokens to include when sending funds to a deployed fork
        address[] erc20TokensToIncludeInFork;
        /// @notice The treasury contract of the last deployed fork
        address forkDAOTreasury;
        /// @notice The token contract of the last deployed fork
        address forkDAOToken;
        /// @notice Timestamp at which the last fork period ends
        uint256 forkEndTimestamp;
        /// @notice Fork period in seconds
        uint256 forkPeriod;
        /// @notice Threshold defined in basis points (10,000 = 100%) required for forking
        uint256 forkThresholdBPS;
        /// @notice Address of the original timelock
        INounsDAOExecutor timelockV1;
        /// @notice The proposal at which to start using `startBlock` instead of `creationBlock` for vote snapshots
        /// @dev Make sure this stays the last variable in this struct, so we can delete it in the next version
        /// @dev To be zeroed-out and removed in a V3.1 fix version once the switch takes place
        uint256 voteSnapshotBlockSwitchProposalId;
717    }

5. Instance 5 at L-719,

Instance 5 Struct can be packed to save lots of gas as below,

719    struct Proposal {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice the ordered list of target addresses for calls to be made
        address[] targets;
        /// @notice The ordered list of values (i.e. msg.value) to be passed to the calls to be made
        uint256[] values;
        /// @notice The ordered list of function signatures to be called
        string[] signatures;
        /// @notice The ordered list of calldata to be passed to each call
        bytes[] calldatas;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been vetoed
        bool vetoed;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
+        /// @notice When true, a proposal would be executed on timelockV1 instead of the current timelock
+        bool executeOnTimelockV1;
        /// @notice Receipts of ballots for the entire set of voters
        mapping(address => Receipt) receipts;
        /// @notice The total supply at the time of proposal creation
        uint256 totalSupply;
        /// @notice The block at which this proposal was created
        uint64 creationBlock;
        /// @notice The last block which allows updating a proposal's description and transactions
        uint64 updatePeriodEndBlock;
        /// @notice Starts at 0 and is set to the block at which the objection period ends when the objection period is initiated
        uint64 objectionPeriodEndBlock;
        /// @dev unused for now
        uint64 placeholder;
        /// @notice The signers of a proposal, when using proposeBySigs
        address[] signers;
-        /// @notice When true, a proposal would be executed on timelockV1 instead of the current timelock
-        bool executeOnTimelockV1;
770    }

6. Instance 6 at L-719,

Instance 6 Struct can be packed to save lots of gas as below,

791    struct ProposalCondensed {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The minimum number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been vetoed
        bool vetoed;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
        /// @notice The total supply at the time of proposal creation
+       /// @notice When true, a proposal would be executed on timelockV1 instead of the current timelock
+       bool executeOnTimelockV1;
        uint256 totalSupply;
        /// @notice The block at which this proposal was created
        uint256 creationBlock;
        /// @notice The signers of a proposal, when using proposeBySigs
        address[] signers;
        /// @notice The last block which allows updating a proposal's description and transactions
        uint256 updatePeriodEndBlock;
        /// @notice Starts at 0 and is set to the block at which the objection period ends when the objection period is initiated
        uint256 objectionPeriodEndBlock;
-       /// @notice When true, a proposal would be executed on timelockV1 instead of the current timelock
-       bool executeOnTimelockV1;
830    }

[G‑03] In NounsDAOStorageV1Fork.sol, save gas by packing structs

In NounsDAOStorageV1Fork.sol, Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

Total Number of Slots saved = 2 slots

There are 2 instances of this issue:

1. Instance 1 at L-58,

Instance 1 Struct can be packed to save lots of gas as below,

    struct Proposal {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice the ordered list of target addresses for calls to be made
        address[] targets;
        /// @notice The ordered list of values (i.e. msg.value) to be passed to the calls to be made
        uint256[] values;
        /// @notice The ordered list of function signatures to be called
        string[] signatures;
        /// @notice The ordered list of calldata to be passed to each call
        bytes[] calldatas;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
        /// @notice Receipts of ballots for the entire set of voters
        mapping(address => Receipt) receipts;
        /// @notice The block at which this proposal was created
        uint256 creationBlock;
    }

1. Instance 2 at L-58,

Instance 2 Struct can be packed to save lots of gas as below,

    struct ProposalCondensed {
        /// @notice Unique id for looking up a proposal
        uint256 id;
-       /// @notice Creator of the proposal
-       address proposer;
        /// @notice The number of votes needed to create a proposal at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 proposalThreshold;
        /// @notice The minimum number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed at the time of proposal creation. *DIFFERS from GovernerBravo
        uint256 quorumVotes;
        /// @notice The timestamp that the proposal will be available for execution, set once the vote succeeds
        uint256 eta;
        /// @notice The block at which voting begins: holders must delegate their votes prior to this block
        uint256 startBlock;
        /// @notice The block at which voting ends: votes must be cast prior to this block
        uint256 endBlock;
        /// @notice Current number of votes in favor of this proposal
        uint256 forVotes;
        /// @notice Current number of votes in opposition to this proposal
        uint256 againstVotes;
        /// @notice Current number of votes for abstaining for this proposal
        uint256 abstainVotes;
+       /// @notice Creator of the proposal
+       address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;
        /// @notice The block at which this proposal was created
        uint256 creationBlock;
    }