Nouns DAO contest - DevABDee's results

1. Floating pragmas are considered a bad practice.

It is always recommended that pragma should be fixed (not floating) to the version that you are intending to deploy your contracts with. For more information Check this out

All the contracts have floating pagma, fix them to 0.8.6:

contracts/governance/NounsDAOLogicV1.sol:61: pragma solidity ^0.8.6;
contracts/governance/NounsDAOLogicV2.sol:53: pragma solidity ^0.8.6;
contracts/governance/NounsDAOInterfaces.sol:33: pragma solidity ^0.8.6;
contracts/governance/NounsDAOProxy.sol:36: pragma solidity ^0.8.6;
contracts/base/ERC721Checkpointable.sol:35: pragma solidity ^0.8.6;
contracts/base/ERC721Enumerable.sol:28: pragma solidity ^0.8.6;

1. Use Custom Errors instead of require statements.

Most nounsDao contracts use require statements for reverting errors, which is not a gas-efficient way to revert errors. The require statements store full-length Strings which costs a lot of Gas (deploying + function Calling & Reverting). For more info check this
Correct these:

contracts/governance/NounsDAOLogicV1.sol: L122, L123, L124, L125, L126, L130, L134, L138, L187, L191, L197, L198, L203, L207, L275, L301, L313, L336, L339 , L364, L365, L366, L422, L485 , L501 , L502, L505, L530, L531, L546, L547 , L563 , L564, L581, L582, L599, L617, L638, L651

contracts/governance/NounsDAOLogicV2.sol: L133, L134, L135, L136, L137, L141, L145, L145, L197, L201, L207, L208 , L213, L217, L286, L312, L324, L347 , L350, L375, L376, L377 , L433, L577, L593, L594, L597 , L622, L623, L638, L639, L655, L656, L674, L677 , L682, L702, L705, L709, L727, L801, L819 , L840, L853 , L1019

contracts/governance/NounsDAOProxy.sol: L79, L80

contracts/base/ERC721Checkpointable.sol: L140 , L141, L142, L164 , L254 , L259, L259, L269, L278

contracts/base/ERC721Enumerable.sol: L62 , L77

2. i++ costs more gas than ++i

i++ returns the non-incremented value, and ++i returns the incremented value. That's why ++i is more gas optimized. For more info check this

- for (uint256 i = 0; i < proposal.targets.length; i++) {
+ for (uint256 i = 0; i < proposal.targets.length; i++) {

Change these:

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

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

3. single Uint8 costs more gas than the uint256

uint256 gives better gas optimization than uint8. For more info check this

contracts/base/ERC721Checkpointable.sol:41: uint8 public constant decimals = 0;

4. Variable declared to its default value costs extra gas.

All uints by default are assigned to Zero. but if you assign them again to their default value (0), That will cost extra gas.

contracts/base/ERC721Checkpointable.sol:41: 
- uint8 public constant decimals = 0;
+ uint8 public constant decimals; //defaults to 0

5. Use Unchecked when looping through the elements of an array.

Each time i++ is called under/overflow checks are made. But we're already constraining i by length, i < length, making those under/overflow checks unnecessary. Consider for example:

uint256 length = array.length;
for(uint256 i = 0; i < length; i++) {
  doSomething(array[i]);
}

We can rewrite the loop like this with unchecked and potentially save significant gas:

uint256 length = array.length;
for(uint256 i = 0; i < length;) {
  doSomething(array[i]);
  unchecked{ i++; }
}

This can be easily implemented in the following lines:

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

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

6. Taking Error/Function Revert Message from the user.

In contracts/governance/NounsDAOLogicV2.sol, the errorMessage in the require statement is a param, which means the user can put any message string in there. This can be a really costly method. And why take an ERROR msg from the user? L1019

    function safe32(uint256 n, string memory errorMessage) internal pure returns (uint32) {
        require(n <= type(uint32).max, errorMessage);
        return uint32(n);
    }

Same thing done in contracts/base/ERC721Checkpointable.sol: L254 , L259, L259, L269, L278