Nouns DAO contest - catchup's results

Low Risk Issues

1. state() function does not check the proposalId to be non-zero

The first created proposal takes index 1 in the _proposals mapping:

226:         proposalCount++;
227:         Proposal storage newProposal = _proposals[proposalCount];
228:         newProposal.id = proposalCount;
229:         newProposal.proposer = msg.sender;

Therefore, there is no proposal with proposalId = 0. state(uint256 proposalId) function checks proposalId to be smaller than or equal to proposalCount, but it does not check if proposalId is non-zero.

432:     function state(uint256 proposalId) public view returns (ProposalState) {
433:         require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
434:         Proposal storage proposal = _proposals[proposalId];
435:         if (proposal.vetoed) {
436:             return ProposalState.Vetoed;

Hence, when the state() function is called with argument 0, it would not revert.

The original GovernorBravoDelegate.sol has such a control:

    function state(uint proposalId) public view returns (ProposalState) {
        require(proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id");
        Proposal storage proposal = proposals[proposalId];

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L432-L434 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L226-L228 https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/Governance/GovernorBravoDelegate.sol#L194-L195

2. Inconsistent limit checks when setting parameters

minQuorumVotesBPS and maxQuorumVotesBPS can be updated independently through _setMinQuorumVotesBPS() and _setMaxQuorumVotesBPS() functions, or they can be updated through _setDynamicQuorumParams() all at once. The individual functions accept the new value to be within limits and limits inclusive, whereas _setDynamicQuorumParams() function excludes the limit values.

677:         require(
678:             newMinQuorumVotesBPS >= MIN_QUORUM_VOTES_BPS_LOWER_BOUND &&
679:                 newMinQuorumVotesBPS <= MIN_QUORUM_VOTES_BPS_UPPER_BOUND,
680:             'NounsDAO::_setMinQuorumVotesBPS: invalid min quorum votes bps'
681:         );
682:         require(
683:             newMinQuorumVotesBPS <= params.maxQuorumVotesBPS,
684:             'NounsDAO::_setMinQuorumVotesBPS: min quorum votes bps greater than max'
685:         );
686: 
687:         uint16 oldMinQuorumVotesBPS = params.minQuorumVotesBPS;

705:         require(
706:             newMaxQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS_UPPER_BOUND,
707:             'NounsDAO::_setMaxQuorumVotesBPS: invalid max quorum votes bps'
708:         );
709:         require(
710:             params.minQuorumVotesBPS <= newMaxQuorumVotesBPS,
711:             'NounsDAO::_setMaxQuorumVotesBPS: min quorum votes bps greater than max'
712:         );
713: 

756:         if (
757:             newMinQuorumVotesBPS < MIN_QUORUM_VOTES_BPS_LOWER_BOUND ||
758:             newMinQuorumVotesBPS > MIN_QUORUM_VOTES_BPS_UPPER_BOUND
759:         ) {
760:             revert InvalidMinQuorumVotesBPS();
761:         }
762:         if (newMaxQuorumVotesBPS > MAX_QUORUM_VOTES_BPS_UPPER_BOUND) {
763:             revert InvalidMaxQuorumVotesBPS();
764:         }
765:         if (newMinQuorumVotesBPS > newMaxQuorumVotesBPS) {
766:             revert MinQuorumBPSGreaterThanMaxQuorumBPS();
767:         }
768: 

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L673-L720 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L748-L781

3. Missing non-zero address checks when setting important addresses

It is a good practice to include 0 address check when setting an important address. I suggest to include a non-zero address check when setting the vetoer in the initialize().

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L156

Non-Critical Issues

1. Faulty comment

The comment within _burnVetoPower() is faulty.

851:     function _burnVetoPower() public {
852:         // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
853:         require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');
854: 
855:         _setVetoer(address(0));

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L852

2. Missing indexed fields

Each event can have up to 3 indexed fields. I believe it would be good to have the address proposer indexed in the below events:

36:     /// @notice An event emitted when a new proposal is created
37:     event ProposalCreated(
38:         uint256 id,
39:         address proposer,
40:         address[] targets,
41:         uint256[] values,
42:         string[] signatures,
43:         bytes[] calldatas,
44:         uint256 startBlock,
45:         uint256 endBlock,
46:         string description
47:     );
48: 
49:     /// @notice An event emitted when a new proposal is created, which includes additional information
50:     event ProposalCreatedWithRequirements(
51:         uint256 id,
52:         address proposer,
53:         address[] targets,
54:         uint256[] values,
55:         string[] signatures,
56:         bytes[] calldatas,
57:         uint256 startBlock,
58:         uint256 endBlock,
59:         uint256 proposalThreshold,
60:         uint256 quorumVotes,
61:         string description
62:     );
63: 

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOInterfaces.sol#L39 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOInterfaces.sol#L52

3. Why burn a privileged role forever

Burning the vetoer role forever through _burnVetoPower() function doesn't make sense, since this is irreversible.

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L851-L856

G01 Saving SLOAD's

Below changes would save 18 SLOADs (each ~100 gas):

NounsDAOLogicV2.sol

• proposalCount is read twice. Cache to save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L227-L228
• replace votingDelay with newVotingDelay. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L630
• replace votingPeriod with newVotingPeriod. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L646
• replace proposalThresholdBPS with newProposalThresholdBPS. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L664
• pendingAdmin is read 3 times. Cache to save 2 SLOADs. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L819 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L823 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L826
• replace pendingAdmin with address(0). Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L832
• vetoer is read twice. Cache to save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L840 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L842

NounsDAOLogicV1.sol

• proposalCount is read twice. Cache to save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L217-L219
• replace votingDelay with newVotingDelay. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L538
• replace votingPeriod with newVotingPeriod. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L554
• replace proposalThresholdBPS with newProposalThresholdBPS. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L572
• replace quorumVotesBPS with newQuorumVotesBPS. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L589
• pendingAdmin is read 3 times. Cache to save 2 SLOADs. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L617 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L621 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L624
• replace pendingAdmin with address(0). Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L630
• vetoer is read twice. Cache to save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L638 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L640

NounsDAOProxy.sol

• replace implementation with implementation_. Save 1 SLOAD. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOProxy.sol#L85

G02 for loops can be optimized

1- For loop index increments can be made unchecked. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases.

2- Postfix increments can be changed as prefix as it is cheaper.

Lines of code

There are 8 instances https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L281 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L319 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L346 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L371 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L292 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L330 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L357 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L382

G03 Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L223 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L230-L235 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L231 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#LL238-L243

G04 Error strings longer than 32 characters

Error strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

Most of the error strings of the require statements are longer than 32 characters. There are around 90 instances in the files within scope. Each instance would save 6 gas.