Nouns DAO contest - __141345__'s results

AVOID FLOATING PRAGMAS: THE VERSION SHOULD BE LOCKED

The pragma declared across the solution is ^0.8.6. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.6) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields.

contracts/base/ERC721Checkpointable.sol

73: 
    event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

contracts/governance/NounsDAOInterfaces.sol
37-47:
    event ProposalCreated(
        uint256 id,
        address proposer,
        address[] targets,
        uint256[] values,
        string[] signatures,
        bytes[] calldatas,
        uint256 startBlock,
        uint256 endBlock,
        string description
    );
50-62:
    event ProposalCreatedWithRequirements(
        uint256 id,
        address proposer,
        address[] targets,
        uint256[] values,
        string[] signatures,
        bytes[] calldatas,
        uint256 startBlock,
        uint256 endBlock,
        uint256 proposalThreshold,
        uint256 quorumVotes,
        string description
    );
70:
    event VoteCast(address indexed voter, uint256 proposalId, uint8 support, uint256 votes, string reason);

NO STORAGE GAP FOR UPGRADEABLE CONTRACT MIGHT LEAD TO STORAGE SLOT COLLISION

For upgradeable contracts, there must be storage gap to “allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments” (quote OpenZeppelin). Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts, potentially causing loss of user fund or cause the contract to malfunction completely.

While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future, or making current state variable changes.

The storage gap is essential for upgradeable contract because “It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments”.

contracts/governance/NounsDAOInterfaces.sol

• NounsDAOStorageV1
• NounsDAOStorageV1Adjusted
• NounsDAOStorageV2

None of the above contain storage gap, in case new state variables added in the future and change the layout of the contract, the gap will become essential to keep the upgradable contract's upgradeability.

Refer to the bottom part of Openzepplin article, which talks about the slot overwrite.

Here is the reference to the usage of gap variable:

• the bottom part of this OZ article
• this OZ issue

Suggestion: Add appropriate storage gap at the end of upgradeable contracts such as below. Can also reference OpenZeppelin upgradeable contract templates.

    uint256[50] private __gap;

Redundant require() check

contracts/governance/NounsDAOLogicV1.sol
364-365:
        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
        require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');

contracts/governance/NounsDAOLogicV2.sol
375-376:
        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
        require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');

The first vetoer != address(0) check is redundant, because the subsequent line checks for msg.sender == vetoer, which ensures vetoer cannot be 0 address.

NATSPEC not complete

contracts/base/ERC721Checkpointable.sol

Suggestion: Follow NATSPEC.

_moveDelegates() will never underflow

contracts/base/ERC721Checkpointable.sol

    function _moveDelegates(
        address srcRep,
        address dstRep,
        uint96 amount
    ) internal {
        // ...
            if (srcRep != address(0)) {
                uint32 srcRepNum = numCheckpoints[srcRep];
                uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
                uint96 srcRepNew = sub96(srcRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount underflows');
                _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
            }
        // ...
    }

_moveDelegates() function is only called in _delegate() and _beforeTokenTransfer(), hence in delegate() and transfer()/transferFrom().

• If in delegate(), amount is the NFF balance of delegator:

205:        uint96 amount = votesToDelegate(delegator);

    function votesToDelegate(address delegator) public view returns (uint96) {
        return safe96(balanceOf(delegator), 'ERC721Checkpointable::votesToDelegate: amount exceeds 96 bits');
    }

And from _delegate(), the votes balance of currentDelegate is at least the same as the NFT balance of the delegator (if currentDelegate only get votes balance from this delegator and not no NFT balance). As a result, it won't underflow.

• If in transfer()/transferFrom(), the amount would be 1, and srcRepOld would be > 0, so won't underflow.

Suggestion: No need to check for underflow:

        uint96 srcRepNew = srcRepOld - amount；

0 address check for delegateBySig()

While address(0) is checked in delegate(), delegateBySig() does not have this check.

As a consequence, through delegateBySig() users might accidentally delegate to 0 address, and need to transfer the NFT and back to restore from the mistake.

Suggestion: Add 0 address check in delegateBySig(), like in delegate(). By doing this, in _moveDelegates(), the 0 address check for srcRep and dstRep can be skipped, since it can never happen. And some gas cost can be saved.

Two-step process of delegate()/delegateBySig()

Users might accidentally delegate to wrong address, although the mistake can be recovered by transferring the NFT and back, the procedure can be lots of work.

But a two-step process to delegate large amount of votes balance can be an option to make the delegate process more robust. It is similar to the approve-transferFrom functionality: The contract approves the new address for a new delegate, and the new address acquires the delegate by calling the contract.

Suggestion: Add two-step delegation as another option in case of large amount votes balance delegate. Just like transfer() and safeTransfer().

Variable re-arrangement by storage packing

in contracts/governance/NounsDAOInterfaces.sol

• in NounsDAOStorageV1, in struct Proposal
• in NounsDAOStorageV1Adjusted, in struct Proposal
• in NounsDAOStorageV2, in struct ProposalCondensed

address proposer can be placed together with

    bool canceled;
    bool vetoed;
    bool executed;

as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

After re-arrangement, it will look like the following:

    address proposer;
    bool canceled;
    bool vetoed;
    bool executed;

Reference: Layout of State Variables in Storage.

FOR-LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.

contracts/governance/NounsDAOLogicV1.sol
281:        for (uint256 i = 0; i < proposal.targets.length; i++) {
319:        for (uint256 i = 0; i < proposal.targets.length; i++) {
346:        for (uint256 i = 0; i < proposal.targets.length; i++) {
371:        for (uint256 i = 0; i < proposal.targets.length; i++) {

contracts/governance/NounsDAOLogicV2.sol
292:        for (uint256 i = 0; i < proposal.targets.length; i++) {
330:        for (uint256 i = 0; i < proposal.targets.length; i++) {
357:        for (uint256 i = 0; i < proposal.targets.length; i++) {
382:        for (uint256 i = 0; i < proposal.targets.length; i++) {

Suggestion: Cache the length before the loop.

    uint length = proposal.targets.length;

unchecked{++i} COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the gas comparison can be seen here.

contracts/governance/NounsDAOLogicV1.sol
216:        proposalCount++;
281:        for (uint256 i = 0; i < proposal.targets.length; i++) {
319:        for (uint256 i = 0; i < proposal.targets.length; i++) {
346:        for (uint256 i = 0; i < proposal.targets.length; i++) {
371:        for (uint256 i = 0; i < proposal.targets.length; i++) {

contracts/governance/NounsDAOLogicV2.sol
226:        proposalCount++;
292:        for (uint256 i = 0; i < proposal.targets.length; i++) {
330:        for (uint256 i = 0; i < proposal.targets.length; i++) {
357:        for (uint256 i = 0; i < proposal.targets.length; i++) {
382:        for (uint256 i = 0; i < proposal.targets.length; i++) {

Suggestion: For readability, uncheck the whole for loop is the same.

        unchecked{
                for (uint256 i; i < length; ++i) {
                }
        }

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

contracts/governance/NounsDAOLogicV1.sol
281:        for (uint256 i = 0; i < proposal.targets.length; i++) {
319:        for (uint256 i = 0; i < proposal.targets.length; i++) {
346:        for (uint256 i = 0; i < proposal.targets.length; i++) {
371:        for (uint256 i = 0; i < proposal.targets.length; i++) {
223:        newProposal.eta = 0;
230:        newProposal.forVotes = 0;
231:        newProposal.againstVotes = 0;
232:        newProposal.abstainVotes = 0;

contracts/governance/NounsDAOLogicV2.sol
292:        for (uint256 i = 0; i < proposal.targets.length; i++) {
330:        for (uint256 i = 0; i < proposal.targets.length; i++) {
357:        for (uint256 i = 0; i < proposal.targets.length; i++) {
382:        for (uint256 i = 0; i < proposal.targets.length; i++) {
231:        newProposal.eta = 0;
238:        newProposal.forVotes = 0;
239:        newProposal.againstVotes = 0;
240:        newProposal.abstainVotes = 0;

contracts/base/ERC721Checkpointable.sol
181:        uint32 lower = 0;

The demo of the gas comparison can be seen here.

SPLITTING REQUIRE() STATEMENTS THAT USE &&

REQUIRE() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

contracts/governance/NounsDAOLogicV1.sol

126-141:
        require(
            votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
            'NounsDAO::initialize: invalid voting period'
        );
        require(
            votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
            'NounsDAO::initialize: invalid voting delay'
        );
        require(
            proposalThresholdBPS_ >= MIN_PROPOSAL_THRESHOLD_BPS && proposalThresholdBPS_ <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::initialize: invalid proposal threshold'
        );
        require(
            quorumVotesBPS_ >= MIN_QUORUM_VOTES_BPS && quorumVotesBPS_ <= MAX_QUORUM_VOTES_BPS,
            'NounsDAO::initialize: invalid proposal threshold'
        );
191-196:
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
531-534:
        require(
            newVotingDelay >= MIN_VOTING_DELAY && newVotingDelay <= MAX_VOTING_DELAY,
            'NounsDAO::_setVotingDelay: invalid voting delay'
        );
547-550:
        require(
            newVotingPeriod >= MIN_VOTING_PERIOD && newVotingPeriod <= MAX_VOTING_PERIOD,
            'NounsDAO::_setVotingPeriod: invalid voting period'
        );
565-568:
        require(
            newProposalThresholdBPS >= MIN_PROPOSAL_THRESHOLD_BPS &&
                newProposalThresholdBPS <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::_setProposalThreshold: invalid proposal threshold'
        );
582-585:
        require(
            newQuorumVotesBPS >= MIN_QUORUM_VOTES_BPS && newQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS,
            'NounsDAO::_setProposalThreshold: invalid proposal threshold'
        );
617:
        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');

contracts/governance/NounsDAOLogicV2.sol
137-148:
        require(
            votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
            'NounsDAO::initialize: invalid voting period'
        );
        require(
            votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
            'NounsDAO::initialize: invalid voting delay'
        );
        require(
            proposalThresholdBPS_ >= MIN_PROPOSAL_THRESHOLD_BPS && proposalThresholdBPS_ <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::initialize: invalid proposal threshold bps'
        );

201-206:
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
623-626:
        require(
            newVotingDelay >= MIN_VOTING_DELAY && newVotingDelay <= MAX_VOTING_DELAY,
            'NounsDAO::_setVotingDelay: invalid voting delay'
        );
639-642:
        require(
            newVotingPeriod >= MIN_VOTING_PERIOD && newVotingPeriod <= MAX_VOTING_PERIOD,
            'NounsDAO::_setVotingPeriod: invalid voting period'
        );
656-660:
        require(
            newProposalThresholdBPS >= MIN_PROPOSAL_THRESHOLD_BPS &&
                newProposalThresholdBPS <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::_setProposalThreshold: invalid proposal threshold bps'
        );
677-681:
        require(
            newMinQuorumVotesBPS >= MIN_QUORUM_VOTES_BPS_LOWER_BOUND &&
                newMinQuorumVotesBPS <= MIN_QUORUM_VOTES_BPS_UPPER_BOUND,
            'NounsDAO::_setMinQuorumVotesBPS: invalid min quorum votes bps'
        );
819:
        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');

REDUCE THE SIZE OF ERROR MESSAGES

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

contracts/base/ERC721Checkpointable.sol
140-142:
        require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
        require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
        require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
164:
        require(blockNumber < block.number, 'ERC721Checkpointable::getPriorVotes: not yet determined');

contracts/base/ERC721Enumerable.sol
62:
        require(index < ERC721.balanceOf(owner), 'ERC721Enumerable: owner index out of bounds');
77:
        require(index < ERC721Enumerable.totalSupply(), 'ERC721Enumerable: global index out of bounds');

contracts/governance/NounsDAOLogicV1.sol
122-141:
        require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once');
        require(msg.sender == admin, 'NounsDAO::initialize: admin only');
        require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address');
        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
        require(
            votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
            'NounsDAO::initialize: invalid voting period'
        );
        require(
            votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
            'NounsDAO::initialize: invalid voting delay'
        );
        require(
            proposalThresholdBPS_ >= MIN_PROPOSAL_THRESHOLD_BPS && proposalThresholdBPS_ <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::initialize: invalid proposal threshold'
        );
        require(
            quorumVotesBPS_ >= MIN_QUORUM_VOTES_BPS && quorumVotesBPS_ <= MAX_QUORUM_VOTES_BPS,
            'NounsDAO::initialize: invalid proposal threshold'
        );
187-198:
        require(
            nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold,
            'NounsDAO::propose: proposer votes below proposal threshold'
        );
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
203-210:
        require(
            proposersLatestProposalState != ProposalState.Active,
            'NounsDAO::propose: one live proposal per proposer, found an already active proposal'
        );
        require(
            proposersLatestProposalState != ProposalState.Pending,
            'NounsDAO::propose: one live proposal per proposer, found an already pending proposal'
        );
275-278:
        require(
            state(proposalId) == ProposalState.Succeeded,
            'NounsDAO::queue: proposal can only be queued if it is succeeded'
        );
301-304:
        require(
            !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
            'NounsDAO::queueOrRevertInternal: identical proposal action already queued at eta'
        );
313-316:
        require(
            state(proposalId) == ProposalState.Queued,
            'NounsDAO::execute: proposal can only be executed if it is queued'
        );
336:
        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
339-343:
        require(
            msg.sender == proposal.proposer ||
                nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold,
            'NounsDAO::cancel: proposer above threshold'
        );
364-366:
        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');
422:
        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
485:
        require(signatory != address(0), 'NounsDAO::castVoteBySig: invalid signature');
501-505:
        require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed');
        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');
        Proposal storage proposal = proposals[proposalId];
        Receipt storage receipt = proposal.receipts[voter];
        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
530-534:
        require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');
        require(
            newVotingDelay >= MIN_VOTING_DELAY && newVotingDelay <= MAX_VOTING_DELAY,
            'NounsDAO::_setVotingDelay: invalid voting delay'
        );
546-549:
        require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only');
        require(
            newVotingPeriod >= MIN_VOTING_PERIOD && newVotingPeriod <= MAX_VOTING_PERIOD,
            'NounsDAO::_setVotingPeriod: invalid voting period'
        );
563-568:
        require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only');
        require(
            newProposalThresholdBPS >= MIN_PROPOSAL_THRESHOLD_BPS &&
                newProposalThresholdBPS <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::_setProposalThreshold: invalid proposal threshold'
        );
581-585:
        require(msg.sender == admin, 'NounsDAO::_setQuorumVotesBPS: admin only');
        require(
            newQuorumVotesBPS >= MIN_QUORUM_VOTES_BPS && newQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS,
            'NounsDAO::_setProposalThreshold: invalid proposal threshold'
        );
599:
        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');
617:
        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
638:
        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
651:
        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');

contracts/governance/NounsDAOLogicV2.sol
133-148:
        require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once');
        require(msg.sender == admin, 'NounsDAO::initialize: admin only');
        require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address');
        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
        require(
            votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
            'NounsDAO::initialize: invalid voting period'
        );
        require(
            votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
            'NounsDAO::initialize: invalid voting delay'
        );
        require(
            proposalThresholdBPS_ >= MIN_PROPOSAL_THRESHOLD_BPS && proposalThresholdBPS_ <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::initialize: invalid proposal threshold bps'
        );
197-208:
        require(
            nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold,
            'NounsDAO::propose: proposer votes below proposal threshold'
        );
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
213-220:
        require(
            proposersLatestProposalState != ProposalState.Active,
            'NounsDAO::propose: one live proposal per proposer, found an already active proposal'
        );
        require(
            proposersLatestProposalState != ProposalState.Pending,
            'NounsDAO::propose: one live proposal per proposer, found an already pending proposal'
        );
286-289:
        require(
            state(proposalId) == ProposalState.Succeeded,
            'NounsDAO::queue: proposal can only be queued if it is succeeded'
        );
312-315:
        require(
            !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
            'NounsDAO::queueOrRevertInternal: identical proposal action already queued at eta'
        );
324-327:
        require(
            state(proposalId) == ProposalState.Queued,
            'NounsDAO::execute: proposal can only be executed if it is queued'
        );
347:
        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
350-354:
        require(
            msg.sender == proposal.proposer ||
                nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold,
            'NounsDAO::cancel: proposer above threshold'
        );
375-377:
        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');
433:
        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
577:
        require(signatory != address(0), 'NounsDAO::castVoteBySig: invalid signature');
593-594:
        require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed');
        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');
597:
        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
622-626:
        require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');
        require(
            newVotingDelay >= MIN_VOTING_DELAY && newVotingDelay <= MAX_VOTING_DELAY,
            'NounsDAO::_setVotingDelay: invalid voting delay'
        );
638-642:
        require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only');
        require(
            newVotingPeriod >= MIN_VOTING_PERIOD && newVotingPeriod <= MAX_VOTING_PERIOD,
            'NounsDAO::_setVotingPeriod: invalid voting period'
        );
655-660:
        require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only');
        require(
            newProposalThresholdBPS >= MIN_PROPOSAL_THRESHOLD_BPS &&
                newProposalThresholdBPS <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::_setProposalThreshold: invalid proposal threshold bps'
        );
674:
        require(msg.sender == admin, 'NounsDAO::_setMinQuorumVotesBPS: admin only');
677-685:
        require(
            newMinQuorumVotesBPS >= MIN_QUORUM_VOTES_BPS_LOWER_BOUND &&
                newMinQuorumVotesBPS <= MIN_QUORUM_VOTES_BPS_UPPER_BOUND,
            'NounsDAO::_setMinQuorumVotesBPS: invalid min quorum votes bps'
        );
        require(
            newMinQuorumVotesBPS <= params.maxQuorumVotesBPS,
            'NounsDAO::_setMinQuorumVotesBPS: min quorum votes bps greater than max'
        );
702:
        require(msg.sender == admin, 'NounsDAO::_setMaxQuorumVotesBPS: admin only');
705-712:
        require(
            newMaxQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS_UPPER_BOUND,
            'NounsDAO::_setMaxQuorumVotesBPS: invalid max quorum votes bps'
        );
        require(
            params.minQuorumVotesBPS <= newMaxQuorumVotesBPS,
            'NounsDAO::_setMaxQuorumVotesBPS: min quorum votes bps greater than max'
        );
727:
        require(msg.sender == admin, 'NounsDAO::_setQuorumCoefficient: admin only');
801:
        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');
819:
        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
840:
        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
853:
        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');

contracts/governance/NounsDAOProxy.sol
79-80:
        require(msg.sender == admin, 'NounsDAOProxy::_setImplementation: admin only');
        require(implementation_ != address(0), 'NounsDAOProxy::_setImplementation: invalid implementation address');

Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) (reference)

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

The demo of the gas comparison can be seen here.

The following files have multiple require() statements can use custom errors:

BOOLEAN COMPARISON

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.

The demo of the gas comparison can be seen here.

Suggestion: Use if(value) / if(!value) instead of if(value == true) / if(value == false)

in the following:

contracts/governance/NounsDAOLogicV1.sol
505:
        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');

contracts/governance/NounsDAOLogicV2.sol
597:
        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');

Adjust the position of require() statement

Here the require() statement can be moved before the assignment of params. Since if the require() reverts, the subsequent code wouldn't be executed and save some operation and gas cost.

contracts/governance/NounsDAOLogicV2.sol

675-681:
        DynamicQuorumParams memory params = getDynamicQuorumParamsAt(block.number);

        require(
            newMinQuorumVotesBPS >= MIN_QUORUM_VOTES_BPS_LOWER_BOUND &&
                newMinQuorumVotesBPS <= MIN_QUORUM_VOTES_BPS_UPPER_BOUND,
            'NounsDAO::_setMinQuorumVotesBPS: invalid min quorum votes bps'
        );

703-708:
        DynamicQuorumParams memory params = getDynamicQuorumParamsAt(block.number);

        require(
            newMaxQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS_UPPER_BOUND,
            'NounsDAO::_setMaxQuorumVotesBPS: invalid max quorum votes bps'
        );

FUNCTIONS ONLY CALLED OUTSIDE OF THE CONTRACT CAN BE DECLARED AS EXTERNAL

External call cost is less expensive than of public functions. And when arguments are read-only on external functions, the data location can be calldata.

The difference is because in public functions, Solidity immediately copies array arguments to memory, while external functions can read directly from calldata. Memory allocation is expensive, whereas reading from calldata is cheap.

contracts/governance/NounsDAOLogicV1.sol
174-180:
    function propose(
        address[] memory targets,
        uint256[] memory values,
        string[] memory signatures,
        bytes[] memory calldatas,
        string memory description
    ) public returns (uint256) {
649:
    function _burnVetoPower() public {
660:
    function proposalThreshold() public view returns (uint256) {

contracts/governance/NounsDAOLogicV2.sol
184-190:
    function propose(
        address[] memory targets,
        uint256[] memory values,
        string[] memory signatures,
        bytes[] memory calldatas,
        string memory description
    ) public returns (uint256) {
851:
    function _burnVetoPower() public {
862:
    function proposalThreshold() public view returns (uint256) {


contracts/base/ERC721Checkpointable.sol
112:
    function delegate(address delegatee) public {

EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO keccak256(),` SHOULD USE IMMUTABLE RATHER THAN CONSTANT

Constant expressions are left as expressions, not constants.

When a constant declared as:

contracts/base/ERC721Checkpointable.sol
59-60:
    bytes32 public constant DOMAIN_TYPEHASH =
        keccak256('EIP712Domain(string name,uint256 chainId,address verifyingContract)');
63-64:
    bytes32 public constant DELEGATION_TYPEHASH =
        keccak256('Delegation(address delegatee,uint256 nonce,uint256 expiry)');

contracts/governance/NounsDAOLogicV1.sol
97-98:
    bytes32 public constant DOMAIN_TYPEHASH =
        keccak256('EIP712Domain(string name,uint256 chainId,address verifyingContract)');
101:
    bytes32 public constant BALLOT_TYPEHASH = keccak256('Ballot(uint256 proposalId,uint8 support)');

contracts/governance/NounsDAOLogicV2.sol
101-102:
    bytes32 public constant DOMAIN_TYPEHASH =
        keccak256('EIP712Domain(string name,uint256 chainId,address verifyingContract)');
105:
    bytes32 public constant BALLOT_TYPEHASH = keccak256('Ballot(uint256 proposalId,uint8 support)');

It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.

consequences:

• each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)
• since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

reference: https://github.com/ethereum/solidity/issues/9232

Use bytes32 instead of string

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32.

Bytes32 can be used instead of string in the following:

contracts/governance/NounsDAOLogicV1.sol
67:     string public constant name = 'Nouns DAO';

contracts/governance/NounsDAOLogicV2.sol
59:     string public constant name = 'Nouns DAO';

Redundant require() check

contracts/governance/NounsDAOLogicV1.sol
364-365:
        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
        require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');

contracts/governance/NounsDAOLogicV2.sol
375-376:
        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
        require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');

The first vetoer != address(0) check is redundant, because the subsequent line checks for msg.sender == vetoer, which ensures vetoer cannot be 0 address.

Update value order can be adjusted to simplify the code and save gas

For example, to update the num variable with newVal, the current way is as following:

    uint oldVal = num;
    num = newVal;
    emit update(oldVal, newVal);

If the execution order is adjusted, some operations can be saved (memory space allocation, variable assignment), reducing both the deployment and run time gas cost.

    emit update(num, newVal);
    num = newVal;

It is already used here:

842-844:
        emit NewVetoer(vetoer, newVetoer);

        vetoer = newVetoer;

The demo of the gas comparison can be seen here.

There are multiple places can use this trick for optimization, since the updates of parameters are widely and frequently used, the optimization can be beneficial.

contracts/governance/NounsDAOLogicV2.sol

627-630:
        uint256 oldVotingDelay = votingDelay;
        votingDelay = newVotingDelay;

        emit VotingDelaySet(oldVotingDelay, votingDelay);

643-646:
        uint256 oldVotingPeriod = votingPeriod;
        votingPeriod = newVotingPeriod;

        emit VotingPeriodSet(oldVotingPeriod, votingPeriod);

661-664:
        uint256 oldProposalThresholdBPS = proposalThresholdBPS;
        proposalThresholdBPS = newProposalThresholdBPS;

        emit ProposalThresholdBPSSet(oldProposalThresholdBPS, proposalThresholdBPS);

687-692:
        uint16 oldMinQuorumVotesBPS = params.minQuorumVotesBPS;
        params.minQuorumVotesBPS = newMinQuorumVotesBPS;

        _writeQuorumParamsCheckpoint(params);

        emit MinQuorumVotesBPSSet(oldMinQuorumVotesBPS, newMinQuorumVotesBPS);

714-719:
        uint16 oldMaxQuorumVotesBPS = params.maxQuorumVotesBPS;
        params.maxQuorumVotesBPS = newMaxQuorumVotesBPS;

        _writeQuorumParamsCheckpoint(params);

        emit MaxQuorumVotesBPSSet(oldMaxQuorumVotesBPS, newMaxQuorumVotesBPS);

730-735:
        uint32 oldQuorumCoefficient = params.quorumCoefficient;
        params.quorumCoefficient = newQuorumCoefficient;

        _writeQuorumParamsCheckpoint(params);

        emit QuorumCoefficientSet(oldQuorumCoefficient, newQuorumCoefficient);

804-810:
        address oldPendingAdmin = pendingAdmin;

        pendingAdmin = newPendingAdmin;

        emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);

822-832:
        address oldAdmin = admin;
        address oldPendingAdmin = pendingAdmin;

        // Store admin with value pendingAdmin
        admin = pendingAdmin;

        // Clear the pending value
        pendingAdmin = address(0);

        emit NewAdmin(oldAdmin, admin);
        emit NewPendingAdmin(oldPendingAdmin, pendingAdmin);

Variables referred multiple times can be cached in local memory

Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference) Even for a memory struct variable, the member variable access still has overhead. It can be saved further by caching the final result.

targets.length can be cached.

contracts/governance/NounsDAOLogicV1.sol
191-198:
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');

contracts/governance/NounsDAOLogicV2.sol
201-208:
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');

Variable amount in _delegate() can be skipped

Since amount is only used in _moveDelegates(), there is no need for the extra variable.

205-207:
        uint96 amount = votesToDelegate(delegator);

        _moveDelegates(currentDelegate, delegatee, amount);

Suggestion:

        _moveDelegates(currentDelegate, delegatee, votesToDelegate(delegator));

_moveDelegates() will never underflow

contracts/base/ERC721Checkpointable.sol

    function _moveDelegates(
        address srcRep,
        address dstRep,
        uint96 amount
    ) internal {
        // ...
            if (srcRep != address(0)) {
                uint32 srcRepNum = numCheckpoints[srcRep];
                uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
                uint96 srcRepNew = sub96(srcRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount underflows');
                _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
            }
        // ...
    }

_moveDelegates() function is only called in _delegate() and _beforeTokenTransfer(), hence in delegate() and transfer()/transferFrom().

• If in delegate(), amount is the NFF balance of delegator:

205:        uint96 amount = votesToDelegate(delegator);

    function votesToDelegate(address delegator) public view returns (uint96) {
        return safe96(balanceOf(delegator), 'ERC721Checkpointable::votesToDelegate: amount exceeds 96 bits');
    }

And from _delegate(), the votes balance of currentDelegate is at least the same as the NFT balance of the delegator (if currentDelegate only get votes balance from this delegator and not no NFT balance). As a result, it won't underflow.

• If in transfer()/transferFrom(), the amount would be 1, and srcRepOld would be > 0, so won't underflow.

Suggestion: No need to check for underflow:

        uint96 srcRepNew = srcRepOld - amount；