Nouns DAO contest - 0x1f8b's results

• Low
  • 1. Mixing and Outdated compiler
  • 2. Outdated packages
  • 3. Denial of serivce by wrong initialization
  • 4. Unsecure state order
  • 5. Lack of returns check
• Non critical
  • 6. Wrong desing
  • 7. Bad nomenclature
  • 8. Use abstract for base contracts
  • 9. Optimize hierarchy in order to improve the readibility
  • 10. Lack of event emmit
  • 11. Lack of modifier
  • 12. Unify codding style
  • 13. Unify error handling

Low

1. Mixing and Outdated compiler

pragma solidity ^0.8.0;
pragma solidity ^0.8.6;

Note that mixing pragma are not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

2. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

    "@openzeppelin/contracts": "^4.1.0",
    "@openzeppelin/contracts-upgradeable": "^4.1.0",

Affected source code:

• package.json:32-33

3. Denial of serivce by wrong initialization

In NounsDAOLogicV1 contract, the method initialize doesn't check that the provided value of vetoer_ is valid and not empty, and this could produce a denial of service because the method _setVetoer checks that the sender is vetoer and if this property is empty, it can never be set.

Recommended change:

    function initialize(
        address timelock_,
        address nouns_,
        address vetoer_,
        uint256 votingPeriod_,
        uint256 votingDelay_,
        uint256 proposalThresholdBPS_,
        uint256 quorumVotesBPS_
    ) public virtual {
        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(vetoer_ != address(0), 'NounsDAO::initialize: invalid timelock address');
        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
        ...

Affected source code:

• NounsDAOLogicV1.sol:116
• NounsDAOLogicV2.sol:156

4. Unsecure state order

Negative states must be processed before positive states, otherwise, a proposal that is Expired or Executed, but is Active or Pending will be returned as Active or Pending, this makes it necessary to check that the state is correct from outside this method. I mean, changing outside the variables that alter this state in the correct way.

    function state(uint256 proposalId) public view returns (ProposalState) {
        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
        Proposal storage proposal = proposals[proposalId];
        if (proposal.vetoed) {
            return ProposalState.Vetoed;
        } else if (proposal.canceled) {
            return ProposalState.Canceled;
        } else if (block.number <= proposal.startBlock) {
            return ProposalState.Pending;
        } else if (block.number <= proposal.endBlock) {
            return ProposalState.Active;
        } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;
        } else if (proposal.eta == 0) {
            return ProposalState.Succeeded;
        } else if (proposal.executed) {
            return ProposalState.Executed;
        } else if (block.timestamp >= proposal.eta + timelock.GRACE_PERIOD()) {
            return ProposalState.Expired;
        } else {
            return ProposalState.Queued;
        }
    }

Affected source code:

• NounsDAOLogicV1.sol:422-442
• NounsDAOLogicV2.sol:432-454

5. Lack of returns check

The NounsDAOLicV2._withdraw method sends ether to the admin account, but the return of sent is not verified, so it is possible to emit the Withdraw event with an error. This could affect dApps that process these types of events.

Affected source code:

• NounsDAOLogicV2.sol:789

Non critical

6. Wrong desing

decimals is not related to ERC721Checkpointable, and it should me moved to ERC721 contract.

abstract contract ERC721Checkpointable is ERC721Enumerable {
    /// @notice Defines decimals as per ERC-20 convention to make integrations with 3rd party governance platforms easier
    uint8 public constant decimals = 0;

Affected source code:

• ERC721Checkpointable.sol:41

7. Bad nomenclature

The use of _ before the name in public or external methods is not recommended, since by default it is used for private methods.

Affected source code:

• NounsDAOProxy.sol:78
• NounsDAOLogicV1.sol:529
• NounsDAOLogicV1.sol:545
• NounsDAOLogicV1.sol:562
• NounsDAOLogicV1.sol:580
• NounsDAOLogicV1.sol:597
• NounsDAOLogicV1.sol:615
• NounsDAOLogicV1.sol:637
• NounsDAOLogicV1.sol:649
• NounsDAOLogicV2.sol:621
• NounsDAOLogicV2.sol:637
• NounsDAOLogicV2.sol:654
• NounsDAOLogicV2.sol:673
• NounsDAOLogicV2.sol:701
• NounsDAOLogicV2.sol:726
• NounsDAOLogicV2.sol:748
• NounsDAOLogicV2.sol:783
• NounsDAOLogicV2.sol:799
• NounsDAOLogicV2.sol:817
• NounsDAOLogicV2.sol:839
• NounsDAOLogicV2.sol:851

8. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• NounsDAOInterfaces.sol:35
• NounsDAOInterfaces.sol:109
• NounsDAOInterfaces.sol:126
• NounsDAOInterfaces.sol:143
• NounsDAOInterfaces.sol:243
• NounsDAOInterfaces.sol:347

9. Optimize hierarchy in order to improve the readibility

The NounsDAOStorageV1Adjusted contract defines a Proposal struct that is exactly the same as the Proposal struct of NounsDAOStorageV1, so it is easier for conversions and readability to use the old structure and extend the fields outside of the new structure.

    struct Proposal {
        /// @notice proposal
        NounsDAOStorageV1.Proposal proposal;
        /// @notice The total supply at the time of proposal creation
        uint256 totalSupply;
        /// @notice The block at which this proposal was created
        uint256 creationBlock;
    }

Affected source code:

• NounsDAOInterfaces.sol:274-315

10. Lack of event emmit

The initialize method does not emit an event when setting the vetoer state variable, as it does for the rest of them.

Recommended change:

    function initialize(
        address timelock_,
        address nouns_,
        address vetoer_,
        uint256 votingPeriod_,
        uint256 votingDelay_,
        uint256 proposalThresholdBPS_,
        uint256 quorumVotesBPS_
    ) public virtual {
        ...
        emit VotingPeriodSet(votingPeriod, votingPeriod_);
        emit VotingDelaySet(votingDelay, votingDelay_);
        emit ProposalThresholdBPSSet(proposalThresholdBPS, proposalThresholdBPS_);
        emit QuorumVotesBPSSet(quorumVotesBPS, quorumVotesBPS_);
+       emit NewVetoer(address(0), vetoer_);
        ...
    }

Affected source code:

• NounsDAOLogicV1.sol:150
• NounsDAOLogicV2.sol:156

11. Lack of modifier

It is recommended to use modifiers in order to improve readability, order and avoid problems of logic failures due to duplication or modification of logics, where at a specific point it has been forgotten to modify.

Unify all these behaviors, as is the case of authentications, using a modifier, is highly recommended.

require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');

Affected source code:

• NounsDAOLogicV1.sol:123
• NounsDAOLogicV1.sol:530
• NounsDAOLogicV1.sol:546
• NounsDAOLogicV1.sol:563
• NounsDAOLogicV1.sol:581
• NounsDAOLogicV1.sol:599
• NounsDAOLogicV2.sol:134
• NounsDAOLogicV2.sol:622
• NounsDAOLogicV2.sol:638
• NounsDAOLogicV2.sol:655
• NounsDAOLogicV2.sol:674
• NounsDAOLogicV2.sol:702
• NounsDAOLogicV2.sol:727
• NounsDAOLogicV2.sol:784
• NounsDAOLogicV2.sol:801

12. Unify codding style

At the beginning of the NounsDAOLogicV1 contract you can see how _ is used in the numerical values to delimit the thousands in a more visual way.

/// @notice The minimum setable voting period
uint256 public constant MIN_VOTING_PERIOD = 5_760; // About 24 hours
/// @notice The max setable voting period
uint256 public constant MAX_VOTING_PERIOD = 80_640; // About 2 weeks

However there are areas in the code where this does not apply.

Reference:

• https://docs.soliditylang.org/en/v0.8.16/grammar.html#a4.SolidityLexer.DecimalNumber

Affected source code:

• NounsDAOLogicV1.sol:673
• NounsDAOLogicV2.sol:98
• NounsDAOLogicV2.sol:908
• NounsDAOLogicV2.sol:1007

13. Unify error handling

You see how the errors have been implemented differently in the safe32 and safe16 methods of the NounsDAOLogicV2 contract, where require is used in one and custom errors in another.

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

function safe16(uint256 n) internal pure returns (uint16) {
    if (n > type(uint16).max) {
        revert UnsafeUint16Cast();
    }
    return uint16(n);
}

Affected source code:

• NounsDAOLogicV2.sol:1018-1028

• Gas
  • 1. Reduce boolean comparison
  • 2. Shift right instead of dividing by 2
  • 3. ++i costs less gas compared to i++ or i += 1
  • 4. Reduce the size of error messages Long revert Strings
  • 5. Use Custom Errors instead of Revert Strings to save Gas
  • 6. There's no need to set default values for variables
  • 7. constants expressions are expressions, not constants
  • 8. Improve gas on error
  • 9. Reduce math operations
  • 10. Remove unnecessary variables
  • 11. Optimize proxy construction
  • 12. Reorder structure layout
  • 13. Take advantage of the default values of new structures
  • 14. Remove id from Proposal struct
  • 15. Remove duplicate authentication mechanism
  • 16. Use calldata instead of memory

Gas

1. Reduce boolean comparison

It's compared a boolean value using == true or == false, instead of using the boolean value. NOT opcode, it's cheaper to use EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814 
TesterB.testNot:     21796    

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814  
TesterB.testNot:     21799

Affected source code:

Use '!' instead of == false:

• NounsDAOLogicV1.sol:505
• NounsDAOLogicV2.sol:597

2. Shift right instead of dividing by 2

Shifting one to the right will calculate a division by two.

he SHR opcode only requires 3 gas, compared to the DIV opcode's consumption of 5. Additionally, shifting is used to get around Solidity's division operation's division-by-0 prohibition.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
  function testDiv(uint a) public returns (uint) { return a / 2; }
}

contract TesterB {
  function testShift(uint a) public returns (uint) { return a >> 1; }
}

Gas saving executing: 172 per entry

TesterA.testDiv:    21965
TesterB.testShift:  21793   

Affected source code:

• ERC721Checkpointable.sol:184
• NounsDAOLogicV2.sol:951

3. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• ERC721Checkpointable.sol:141
• NounsDAOLogicV1.sol:216
• NounsDAOLogicV1.sol:281
• NounsDAOLogicV1.sol:319
• NounsDAOLogicV1.sol:346
• NounsDAOLogicV1.sol:371
• NounsDAOLogicV2.sol:226
• NounsDAOLogicV2.sol:292
• NounsDAOLogicV2.sol:330
• NounsDAOLogicV2.sol:357
• NounsDAOLogicV2.sol:382

4. Reduce the size of error messages (Long revert Strings)

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.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
 require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886   
TesterB.testLongRevert:  21904       

Affected source code:

• ERC721Checkpointable.sol:80
• ERC721Checkpointable.sol:140
• ERC721Checkpointable.sol:141
• ERC721Checkpointable.sol:142
• ERC721Checkpointable.sol:164
• ERC721Checkpointable.sol:219
• ERC721Checkpointable.sol:226
• ERC721Checkpointable.sol:238-241
• ERC721Enumerable.sol:62
• ERC721Enumerable.sol:77
• NounsDAOLogicV1.sol:122
• NounsDAOLogicV1.sol:123
• NounsDAOLogicV1.sol:124
• NounsDAOLogicV1.sol:125
• NounsDAOLogicV1.sol:126-129
• NounsDAOLogicV1.sol:130-133
• NounsDAOLogicV1.sol:134-137
• NounsDAOLogicV1.sol:138-141
• NounsDAOLogicV1.sol:187-190
• NounsDAOLogicV1.sol:191-196
• NounsDAOLogicV1.sol:197
• NounsDAOLogicV1.sol:198
• NounsDAOLogicV1.sol:203-206
• NounsDAOLogicV1.sol:207-210
• NounsDAOLogicV1.sol:275-278
• NounsDAOLogicV1.sol:301-304
• NounsDAOLogicV1.sol:313-316
• NounsDAOLogicV1.sol:336
• NounsDAOLogicV1.sol:339-343
• NounsDAOLogicV1.sol:364
• NounsDAOLogicV1.sol:366
• NounsDAOLogicV1.sol:422
• NounsDAOLogicV1.sol:485
• NounsDAOLogicV1.sol:501
• NounsDAOLogicV1.sol:502
• NounsDAOLogicV1.sol:505
• NounsDAOLogicV1.sol:530
• NounsDAOLogicV1.sol:531-534
• NounsDAOLogicV1.sol:546
• NounsDAOLogicV1.sol:547-550
• NounsDAOLogicV1.sol:563
• NounsDAOLogicV1.sol:564-568
• NounsDAOLogicV1.sol:581
• NounsDAOLogicV1.sol:582-585
• NounsDAOLogicV1.sol:599
• NounsDAOLogicV1.sol:617
• NounsDAOLogicV1.sol:638
• NounsDAOLogicV1.sol:651
• NounsDAOLogicV2.sol:133
• NounsDAOLogicV2.sol:134
• NounsDAOLogicV2.sol:135
• NounsDAOLogicV2.sol:136
• NounsDAOLogicV2.sol:137-140
• NounsDAOLogicV2.sol:141-144
• NounsDAOLogicV2.sol:145-148
• NounsDAOLogicV2.sol:197-200
• NounsDAOLogicV2.sol:201-206
• NounsDAOLogicV2.sol:207
• NounsDAOLogicV2.sol:208
• NounsDAOLogicV2.sol:213-216
• NounsDAOLogicV2.sol:217-220
• NounsDAOLogicV2.sol:286-289
• NounsDAOLogicV2.sol:312-315
• NounsDAOLogicV2.sol:324-327
• NounsDAOLogicV2.sol:347
• NounsDAOLogicV2.sol:350-354
• NounsDAOLogicV2.sol:375
• NounsDAOLogicV2.sol:377
• NounsDAOLogicV2.sol:433
• NounsDAOLogicV2.sol:577
• NounsDAOLogicV2.sol:593
• NounsDAOLogicV2.sol:594
• NounsDAOLogicV2.sol:597
• NounsDAOLogicV2.sol:622
• NounsDAOLogicV2.sol:623-626
• NounsDAOLogicV2.sol:638
• NounsDAOLogicV2.sol:639-642
• NounsDAOLogicV2.sol:655
• NounsDAOLogicV2.sol:656-660
• NounsDAOLogicV2.sol:674
• NounsDAOLogicV2.sol:677-681
• NounsDAOLogicV2.sol:682-685
• NounsDAOLogicV2.sol:702
• NounsDAOLogicV2.sol:705-708
• NounsDAOLogicV2.sol:709-712
• NounsDAOLogicV2.sol:727
• NounsDAOLogicV2.sol:801
• NounsDAOLogicV2.sol:819
• NounsDAOLogicV2.sol:840
• NounsDAOLogicV2.sol:853
• NounsDAOLogicV2.sol:923
• NounsDAOProxy.sol:79
• NounsDAOProxy.sol:80

5. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

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).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• ERC721Checkpointable.sol:140
• ERC721Checkpointable.sol:141
• ERC721Checkpointable.sol:142
• ERC721Checkpointable.sol:164
• ERC721Checkpointable.sol:254
• ERC721Checkpointable.sol:259
• ERC721Checkpointable.sol:269
• ERC721Checkpointable.sol:278
• ERC721Enumerable.sol:62
• ERC721Enumerable.sol:77
• NounsDAOLogicV1.sol:122
• NounsDAOLogicV1.sol:123
• NounsDAOLogicV1.sol:124
• NounsDAOLogicV1.sol:125
• NounsDAOLogicV1.sol:126-129
• NounsDAOLogicV1.sol:130-133
• NounsDAOLogicV1.sol:134-137
• NounsDAOLogicV1.sol:138-141
• NounsDAOLogicV1.sol:187-190
• NounsDAOLogicV1.sol:191-196
• NounsDAOLogicV1.sol:197
• NounsDAOLogicV1.sol:198
• NounsDAOLogicV1.sol:203-206
• NounsDAOLogicV1.sol:207-210
• NounsDAOLogicV1.sol:275-278
• NounsDAOLogicV1.sol:301-304
• NounsDAOLogicV1.sol:313-316
• NounsDAOLogicV1.sol:336
• NounsDAOLogicV1.sol:339-343
• NounsDAOLogicV1.sol:364
• NounsDAOLogicV1.sol:365
• NounsDAOLogicV1.sol:366
• NounsDAOLogicV1.sol:422
• NounsDAOLogicV1.sol:485
• NounsDAOLogicV1.sol:501
• NounsDAOLogicV1.sol:502
• NounsDAOLogicV1.sol:505
• NounsDAOLogicV1.sol:530
• NounsDAOLogicV1.sol:531-534
• NounsDAOLogicV1.sol:546
• NounsDAOLogicV1.sol:547-550
• NounsDAOLogicV1.sol:563
• NounsDAOLogicV1.sol:564-568
• NounsDAOLogicV1.sol:581
• NounsDAOLogicV1.sol:582-585
• NounsDAOLogicV1.sol:599
• NounsDAOLogicV1.sol:617
• NounsDAOLogicV1.sol:638
• NounsDAOLogicV1.sol:651
• NounsDAOLogicV2.sol:133
• NounsDAOLogicV2.sol:134
• NounsDAOLogicV2.sol:135
• NounsDAOLogicV2.sol:136
• NounsDAOLogicV2.sol:137-140
• NounsDAOLogicV2.sol:141-144
• NounsDAOLogicV2.sol:145-148
• NounsDAOLogicV2.sol:197-200
• NounsDAOLogicV2.sol:201-206
• NounsDAOLogicV2.sol:207
• NounsDAOLogicV2.sol:208
• NounsDAOLogicV2.sol:213-216
• NounsDAOLogicV2.sol:217-220
• NounsDAOLogicV2.sol:286-289
• NounsDAOLogicV2.sol:312-315
• NounsDAOLogicV2.sol:324-327
• NounsDAOLogicV2.sol:347
• NounsDAOLogicV2.sol:350-354
• NounsDAOLogicV2.sol:375
• NounsDAOLogicV2.sol:376
• NounsDAOLogicV2.sol:377
• NounsDAOLogicV2.sol:433
• NounsDAOLogicV2.sol:577
• NounsDAOLogicV2.sol:593
• NounsDAOLogicV2.sol:594
• NounsDAOLogicV2.sol:597
• NounsDAOLogicV2.sol:622
• NounsDAOLogicV2.sol:623-626
• NounsDAOLogicV2.sol:638
• NounsDAOLogicV2.sol:639-642
• NounsDAOLogicV2.sol:655
• NounsDAOLogicV2.sol:656-660
• NounsDAOLogicV2.sol:674
• NounsDAOLogicV2.sol:677-681
• NounsDAOLogicV2.sol:682-685
• NounsDAOLogicV2.sol:702
• NounsDAOLogicV2.sol:705-708
• NounsDAOLogicV2.sol:709-712
• NounsDAOLogicV2.sol:727
• NounsDAOLogicV2.sol:801
• NounsDAOLogicV2.sol:819
• NounsDAOLogicV2.sol:840
• NounsDAOLogicV2.sol:853
• NounsDAOLogicV2.sol:1019
• NounsDAOProxy.sol:79
• NounsDAOProxy.sol:80

6. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• ERC721Checkpointable.sol:41
• ERC721Checkpointable.sol:181
• NounsDAOLogicV2.sol:948

7. constants expressions are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

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

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).

Affected source code:

• ERC721Checkpointable.sol:59-60
• ERC721Checkpointable.sol:63-64
• NounsDAOLogicV1.sol:97-98
• NounsDAOLogicV1.sol:101
• NounsDAOLogicV2.sol:101-102
• NounsDAOLogicV2.sol:105

8. Improve gas on error

It is possible to improve the gas consumption in certain cases of the ERC721Checkpointable.delegateBySig function since the checks that are not related to the signature of the message can be done first, and then the more expensive ones, in this way errors can be checked optimally.

Recommended change:

    function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {
+       require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
+       require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
        bytes32 domainSeparator = keccak256(
            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
        );
        bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
        bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
        address signatory = ecrecover(digest, v, r, s);
        require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
-       require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
-       require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
        return _delegate(signatory, delegatee);
    }

Affected source code:

• ERC721Checkpointable.sol:141-142

9. Reduce math operations

Several methods have been found in which it is possible to reduce the number of mathematical operations, the cases are detailed below.

Recommended change for ERC721Checkpointable.getPriorVotes:

It's possible to reuse the upper variable in order to save some operations in ERC721Checkpointable.sol:182.

    function getPriorVotes(address account, uint256 blockNumber) public view returns (uint96) {
        ...
        // First check most recent balance
+       uint32 upper = nCheckpoints - 1;
-       if (checkpoints[account][nCheckpoints - 1].fromBlock <= blockNumber) {
-           return checkpoints[account][nCheckpoints - 1].votes;
+       if (checkpoints[account][upper].fromBlock <= blockNumber) {
+           return checkpoints[account][upper].votes;
        }
        ...
        uint32 lower = 0;
-       uint32 upper = nCheckpoints - 1;
        while (upper > lower) {
            ...
        }
        return checkpoints[account][lower].votes;
    }

Recommended change for NounsDAOLogicV2.getDynamicQuorumParamsAt:

It's possible to reuse the upper variable in order to save some operations in NounsDAOLogicV2.sol:935-949.

    function getDynamicQuorumParamsAt(uint256 blockNumber_) public view returns (DynamicQuorumParams memory) {
        ...
+       uint256 upper = len - 1;
+       if (quorumParamsCheckpoints[upper].fromBlock <= blockNumber) {
+           return quorumParamsCheckpoints[upper].params;
-       if (quorumParamsCheckpoints[len - 1].fromBlock <= blockNumber) {
-           return quorumParamsCheckpoints[len - 1].params;
        }
        ...
        uint256 lower = 0;
-       uint256 upper = len - 1;
        while (upper > lower) {
            ...
        }
        return quorumParamsCheckpoints[lower].params;
    }

Recommended change for ERC721Checkpointable.safe32:

Change 2**32 to type(uint32).max in ERC721Checkpointable.sol:254:

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

Recommended change for ERC721Checkpointable.safe96:

Change 2**32 to type(uint96).max in ERC721Checkpointable.sol:259:

    function safe96(uint256 n, string memory errorMessage) internal pure returns (uint96) {
-       require(n < 2**96, errorMessage);
+       require(n <= type(uint96).max, errorMessage);
        return uint96(n);
    }

10. Remove unnecessary variables

In NounsDAOProxy.sol:82-85 it is possible to emit the event NewImplementation before and save the oldImplementation variable declaration.

    function _setImplementation(address implementation_) public {
        require(msg.sender == admin, 'NounsDAOProxy::_setImplementation: admin only');
        require(implementation_ != address(0), 'NounsDAOProxy::_setImplementation: invalid implementation address');

-       address oldImplementation = implementation;
+       emit NewImplementation(implementation, implementation_);
        implementation = implementation_;

-       emit NewImplementation(oldImplementation, implementation);
    }

This same logic can be applied to all the points detailed below.

Affected source code:

• NounsDAOProxy.sol:82-85
• NounsDAOLogicV1.sol:535-538
• NounsDAOLogicV1.sol:551-554
• NounsDAOLogicV1.sol:569-572
• NounsDAOLogicV1.sol:586-589
• NounsDAOLogicV1.sol:602-608
• NounsDAOLogicV1.sol:620-630
• NounsDAOLogicV2.sol:630
• NounsDAOLogicV2.sol:643-646
• NounsDAOLogicV2.sol:664
• NounsDAOLogicV2.sol:687-692
• NounsDAOLogicV2.sol:714-719
• NounsDAOLogicV2.sol:730-735
• NounsDAOLogicV2.sol:804-810
• NounsDAOLogicV2.sol:822-831
• NounsDAOLogicV2.sol:823-832

11. Optimize proxy construction

In NounsDAOProxy, the admin is faked by first setting it to msg.sender, that's because it prevents _setImplementation failures, however, it is more optimal to perform the logic of the method inline, and avoid this bypass that only generates an extra gas cost.

Recommended change:

    constructor(
        address timelock_,
        address nouns_,
        address vetoer_,
        address admin_,
        address implementation_,
        uint256 votingPeriod_,
        uint256 votingDelay_,
        uint256 proposalThresholdBPS_,
        uint256 quorumVotesBPS_
    ) {
-       // Admin set to msg.sender for initialization
-       admin = msg.sender;
+       admin = admin_;
+       implementation = implementation_;
+       emit NewImplementation(address(0), implementation_);

        delegateTo(
            implementation_,
            abi.encodeWithSignature(
                'initialize(address,address,address,uint256,uint256,uint256,uint256)',
                timelock_,
                nouns_,
                vetoer_,
                votingPeriod_,
                votingDelay_,
                proposalThresholdBPS_,
                quorumVotesBPS_
            )
        );
-
-       _setImplementation(implementation_);
-       admin = admin_;
    }

Affected source code:

• NounsDAOProxy.sol:53-71

12. Reorder structure layout

The following structs could be optimized moving the position of certains values in order to save slot storages:

Recommended change:

Put booleans types together and close to an address type.

    struct Proposal {
        /// @notice Unique id for looking up a proposal
        uint256 id;
        /// @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 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 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;
    }

Affected source code:

• NounsDAOInterfaces.sol:174-211
• NounsDAOInterfaces.sol:274-315

Recommended change:

Put booleans types together and close to an address type.

    struct ProposalCondensed {
        /// @notice Unique id for looking up a proposal
        uint256 id;
        /// @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 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 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;
    }

Affected source code:

• NounsDAOInterfaces.sol:389-394

13. Take advantage of the default values of new structures

In the propose method inside the NounsDAOLogicV1 contract, a new proposal is always created, this structure comes with all values with the default value, so there is no reason to set the initial value again.

Recommended change:

        proposalCount++;
        Proposal storage newProposal = proposals[proposalCount];

        newProposal.id = proposalCount;
        newProposal.proposer = msg.sender;
        newProposal.proposalThreshold = temp.proposalThreshold;
        newProposal.quorumVotes = bps2Uint(quorumVotesBPS, temp.totalSupply);
-       newProposal.eta = 0;
        newProposal.targets = targets;
        newProposal.values = values;
        newProposal.signatures = signatures;
        newProposal.calldatas = calldatas;
        newProposal.startBlock = temp.startBlock;
        newProposal.endBlock = temp.endBlock;
-       newProposal.forVotes = 0;
-       newProposal.againstVotes = 0;
-       newProposal.abstainVotes = 0;
-       newProposal.canceled = false;
-       newProposal.executed = false;
-       newProposal.vetoed = false;

Affected source code:

• NounsDAOLogicV1.sol:223-235
• NounsDAOLogicV2.sol:231-243

14. Remove id from Proposal struct

Since to obtain the propsal it is always done by accessing the proposals mapping with the proposalId as an index, it is not necessary to replicate this value within the structure itself.

    function queue(uint256 proposalId) external {
        require(
            state(proposalId) == ProposalState.Succeeded,
            'NounsDAO::queue: proposal can only be queued if it is succeeded'
        );
        Proposal storage proposal = proposals[proposalId];
        ...

This will eliminate one slot of storage per entry and will also optimize all read/write operations.

Affected source code:

• NounsDAOLogicV1.sol:219
• NounsDAOInterfaces.sol:176
• NounsDAOLogicV2.sol:228

15. Remove duplicate authentication mechanism

The NounsDAOLogicV1._setVetoer method already performs the vetoer checks, so it's possible to save gas removing the following lines:

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

Similarly, in NounsDAOLogicV2._setDynamicQuorumParams it is checked that the msg.sender is admin, so there is also no need to check in the NounsDAOLogicV2.initialize method.

Affected source code:

• NounsDAOLogicV1.sol:651
• NounsDAOLogicV2.sol:134
• NounsDAOLogicV2.sol:853

16. Use calldata instead of memory

The method propose is external, and the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended change:

    function propose(
-       address[] memory targets,
-       uint256[] memory values,
-       string[] memory signatures,
-       bytes[] memory calldatas,
-       string memory description
-   ) public returns (uint256) {
+       address[] calldata targets,
+       uint256[] calldata values,
+       string[] calldata signatures,
+       bytes[] calldata calldatas,
+       string calldata description
+   ) external returns (uint256) {
    ...
    }

Affected source code:

• NounsDAOLogicV1.sol:174-180
• NounsDAOLogicV2.sol:184-190