Nouns DAO contest - CodingNameKiki's results

1. Unused/empty receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))

contracts/governance/NounsDAOLogicV2.sol 1030: receive() external payable {}

2. Front-runable initializer

There is nothing preventing another account from calling the initializer before the contract owner. In the best case, the owner is forced to waste gas and re-deploy. In the worst case, the owner does not notice that his/her call reverts, and everyone starts using a contract under the control of an attacker.

contracts/governance/NounsDAOLogicV2.sol 285: function queue(uint256 proposalId) external { 323: function execute(uint256 proposalId) external { 346: function cancel(uint256 proposalId) external { 374: function veto(uint256 proposalId) external { 489: function castVote(uint256 proposalId, uint8 support) external { 503: function castRefundableVote(uint256 proposalId, uint8 support) external { 518: function castRefundableVoteWithReason( 552: function castVoteWithReason( 564: function castVoteBySig(

3. Non-assembly method available

assembly{ id := chainid() } => uint256 id = block.chainid;

contracts/governance/NounsDAOLogicV2.sol 1013: chainId := chainid()

contracts/base/ERC721Checkpointable.sol 285: chainId := chainid()

4. Constants should be defined rather than using magic numbers

contracts/governance/NounsDAOLogicV2.sol 909: uint256 quorumAdjustmentBPS = (params.quorumCoefficient * againstVotesBPS) / 1e6; 1007: return (number * bps) / 10000;

contracts/base/ERC721Checkpointable.sol 254: require(n < 232, errorMessage); 259: require(n < 296, errorMessage);

5. Inconsistent spacing in comments

contracts/governance/NounsDAOLogicV2.sol 37: // - _setMinQuorumVotesBPS(uint16 newMinQuorumVotesBPS) 38: // - _setMaxQuorumVotesBPS(uint16 newMaxQuorumVotesBPS) 39: // - _setQuorumCoefficient(uint32 newQuorumCoefficient) 43: // - totalSupply used in dynamic quorum calculation. 44: // - creationBlock used for retrieving checkpoints of votes and dynamic quorum params. This now

6. Function state mutability can be restricted to pure

Function doesn't change and can be declared as pure:

contracts/governance/NounsDAOLogicV2.sol 432: function state(uint256 proposalId) public view returns (ProposalState) { 862: function proposalThreshold() public view returns (uint256) { 877: function quorumVotes(uint256 proposalId) public view returns (uint256) { 922: function getDynamicQuorumParamsAt(uint256 blockNumber_) public view returns (DynamicQuorumParams memory) { 995: function minQuorumVotes() public view returns (uint256) { 1002: function maxQuorumVotes() public view returns (uint256) {

contracts/base/ERC721Checkpointable.sol 79: function votesToDelegate(address delegator) public view returns (uint96) { 88: function delegates(address delegator) public view returns (address) { 151: function getCurrentVotes(address account) external view returns (uint96) { 163: function getPriorVotes(address account, uint256 blockNumber) public view returns (uint96) { 282: function getChainId() internal view returns (uint256) {

7. Abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

contracts/governance/NounsDAOLogicV2.sol 575: bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));

contracts/base/ERC721Checkpointable.sol 138: bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));

8. Use of floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

contracts/governance/NounsDAOLogicV2.sol 53: pragma solidity ^0.8.6;

contracts/governance/NounsDAOProxy.sol 36: pragma solidity ^0.8.6;

contracts/governance/NounsDAOInterfaces.sol 33: pragma solidity ^0.8.6;

contracts/base/ERC721Checkpointable.sol 35: pragma solidity ^0.8.6;

contracts/base/ERC721Enumerable.sol 28: pragma solidity ^0.8.0;

9. Initialize functions can be frontrun

The initialize function used to initialize important contract state can be called by anyone. The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract. In the best case for the victim, they notice it and have to redeploy their contract costing gas.

contracts/governance/NounsDAOLogicV2.sol 124: function initialize(

10. Boolean comprasions

There is no need to compate constant to (true or false)

contracts/governance/NounsDAOLogicV2.sol (Before) 329: proposal.executed = true; (After) 329: !proposal.executed;

(Before) 356: proposal.canceled = true; (After) 356: !proposal.canceled;

(Before) 381: proposal.vetoed = true; (After) 381: !proposal.vetoed;

(Before) 610: receipt.hasVoted = true; (After) 610: !receipt.hasVoted;

11. Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

contracts/governance/NounsDAOLogicV2.sol 101: bytes32 public constant DOMAIN_TYPEHASH = 105: bytes32 public constant BALLOT_TYPEHASH = keccak256('Ballot(uint256 proposalId,uint8 support)');

contracts/base/ERC721Checkpointable.sol 59: bytes32 public constant DOMAIN_TYPEHASH = 63: bytes32 public constant DELEGATION_TYPEHASH =

12. Consider addings checks for signature malleability

Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly

contracts/base/ERC721Checkpointable.sol 139: address signatory = ecrecover(digest, v, r, s);

13. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

contracts/base/ERC721Checkpointable.sol 254: require(n < 232, errorMessage); 259: require(n < 296, errorMessage);

14. Not using the named return variables anywhere in the function is confusing

Consider changing the variable to be an unnamed one

contracts/base/ERC721Enumerable.sol 54: function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC721) returns (bool) {

1. For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

contracts/governance/NounsDAOLogicV2.sol (Before) 292: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 292: uint256 len = proposal.targets.length; for (uint256 i = 0; i < len; i++) {

(Before) 330: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 330: uint256 len = proposal.targets.length; for (uint256 i = 0; i < len; i++) {

(Before) 357: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 357: uint256 len = proposal.targets.length; for (uint256 i = 0; i < len; i++) {

(Before) 382: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 382: uint256 len = proposal.targets.length; for (uint256 i = 0; i < len; i++) {

2. For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks. In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

contracts/governance/NounsDAOLogicV2.sol (Before) 292: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 292: for (uint256 i = 0; i < proposal.targets.length;) { unchecked { ++i; }

(Before) 330: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 330: for (uint256 i = 0; i < proposal.targets.length;) { unchecked { ++i; }

(Before) 357: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 357: for (uint256 i = 0; i < proposal.targets.length;) { unchecked { ++i; }

(Before) 382: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 382: for (uint256 i = 0; i < proposal.targets.length;) { unchecked { ++i; }

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

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

contracts/governance/NounsDAOLogicV2.sol 226: proposalCount++;

4. Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

contracts/governance/NounsDAOLogicV2.sol 541: if (votes > 0) { 967: if (pos > 0 && quorumParamsCheckpoints[pos - 1].fromBlock == blockNumber) {

contracts/base/ERC721Checkpointable.sol 153: return nCheckpoints > 0 ? checkpoints[account][nCheckpoints - 1].votes : 0; 215: if (srcRep != dstRep && amount > 0) { 243: if (nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber) {

5. Arithmetics: Unchecking arithmetic operations that cannot underflow/overflow

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks. In some instances, an overflow/underflow is impossible, such as: 1.A check is in place before the arithmetic operation 2.The value realistically cannot overflow, such as amount of ETH sent

As such, gas can be saved by using an unchecked block to remove the implicit checks:

contracts/governance/NounsDAOLogicV2.sol (Before) 949: uint256 upper = len - 1; (After) 949: unchecked { uint256 upper = len - 1; }

(Before) 958: upper = center - 1; (After) 958: unchecked { upper = center - 1; }

(Before) 223: temp.startBlock = block.number + votingDelay; (After) 223: unchecked { temp.startBlock = block.number + votingDelay; }

(Before) 224: temp.endBlock = temp.startBlock + votingPeriod; (After) 224: unchecked { temp.endBlock = temp.startBlock + votingPeriod; }

(Before) 603: proposal.againstVotes = proposal.againstVotes + votes; (After) 603: unchecked { proposal.againstVotes = proposal.againstVotes + votes; }

(Before) 605: proposal.forVotes = proposal.forVotes + votes; (After) 605: unchecked { proposal.forVotes = proposal.forVotes + votes; }

(Before) 607: proposal.abstainVotes = proposal.abstainVotes + votes; (After) 607: unchecked { proposal.abstainVotes = proposal.abstainVotes + votes; }

(Before) 908: uint256 againstVotesBPS = (10000 * againstVotes) / totalSupply; (After) 908: unchecked { uint256 againstVotesBPS = (10000 * againstVotes) / totalSupply; }

(Before) 909: uint256 quorumAdjustmentBPS = (params.quorumCoefficient * againstVotesBPS) / 1e6; (After) 909: unchecked { uint256 quorumAdjustmentBPS = (params.quorumCoefficient * againstVotesBPS) / 1e6; }

(Before) 910: uint256 adjustedQuorumBPS = params.minQuorumVotesBPS + quorumAdjustmentBPS; (After) 910: unchecked { uint256 adjustedQuorumBPS = params.minQuorumVotesBPS + quorumAdjustmentBPS; }

(Before) 951: uint256 center = upper - (upper - lower) / 2; (After) 951: unchecked { uint256 center = upper - (upper - lower) / 2; }

contracts/base/ERC721Checkpointable.sol (Before) 247: numCheckpoints[delegatee] = nCheckpoints + 1; (After) 247: unchecked { numCheckpoints[delegatee] = nCheckpoints + 1; }

(Before) 268: uint96 c = a + b; (After) 268: unchecked { uint96 c = a + b; }

(Before) 279: return a - b; (After) 279: unchecked { return a - b; }

(Before) 182: uint32 upper = nCheckpoints - 1; (After) 182: unchecked { uint32 upper = nCheckpoints - 1; }

(Before) 191: upper = center - 1; (After) 191: unchecked { upper = center - 1; }

(Before) 184: uint32 center = upper - (upper - lower) / 2; (After) 184: unchecked { uint32 center = upper - (upper - lower) / 2; }

6. Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

contracts/governance/NounsDAOLogicV2.sol 59: string public constant name = 'Nouns DAO'; 62: uint256 public constant MIN_PROPOSAL_THRESHOLD_BPS = 1; 65: uint256 public constant MAX_PROPOSAL_THRESHOLD_BPS = 1_000; 68: uint256 public constant MIN_VOTING_PERIOD = 5_760; 71: uint256 public constant MAX_VOTING_PERIOD = 80_640; 74: uint256 public constant MIN_VOTING_DELAY = 1; 77: uint256 public constant MAX_VOTING_DELAY = 40_320; 80: uint256 public constant MIN_QUORUM_VOTES_BPS_LOWER_BOUND = 200; 83: uint256 public constant MIN_QUORUM_VOTES_BPS_UPPER_BOUND = 2_000; 86: uint256 public constant MAX_QUORUM_VOTES_BPS_UPPER_BOUND = 6_000; 89: uint256 public constant MAX_QUORUM_VOTES_BPS = 2_000; 92: uint256 public constant proposalMaxOperations = 10; 95: uint256 public constant MAX_REFUND_PRIORITY_FEE = 2 gwei; 98: uint256 public constant REFUND_BASE_GAS = 36000; 101: bytes32 public constant DOMAIN_TYPEHASH = 105: bytes32 public constant BALLOT_TYPEHASH = keccak256('Ballot(uint256 proposalId,uint8 support)');

contracts/base/ERC721Checkpointable.sol 41: uint8 public constant decimals = 0; 59: bytes32 public constant DOMAIN_TYPEHASH = 63: bytes32 public constant DELEGATION_TYPEHASH =

7. Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

contracts/governance/NounsDAOLogicV2.sol 184: function propose( 432: function state(uint256 proposalId) public view returns (ProposalState) { 748: function _setDynamicQuorumParams( 839: function _setVetoer(address newVetoer) public { 851: function burnVetoPower() public { 862: function proposalThreshold() public view returns (uint256) { 877: function quorumVotes(uint256 proposalId) public view returns (uint256) { 903: function dynamicQuorumVotes( 922: function getDynamicQuorumParamsAt(uint256 blockNumber) public view returns (DynamicQuorumParams memory) { 995: function minQuorumVotes() public view returns (uint256) { 1002: function maxQuorumVotes() public view returns (uint256) {

8. Unecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

1.uint: 0 2.bool: false 3.address: address(0)

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

(Before) 357: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 357: for (uint256 i; i < proposal.targets.length; i++) {

(Before) 382: for (uint256 i = 0; i < proposal.targets.length; i++) { (After) 382: for (uint256 i; i < proposal.targets.length; i++) {

(Before) 948: uint256 lower = 0; (After) 948: uint256 lower;

contracts/base/ERC721Checkpointable.sol (Before) 41: uint8 public constant decimals = 0; (After) 41: uint8 public constant decimals;

(Before) 181: uint32 lower = 0; (After) 181: uint32 lower;

9. Errors: Reduce the length 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. In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

contracts/governance/NounsDAOLogicV2.sol 133: require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once'); 134: require(msg.sender == admin, 'NounsDAO::initialize: admin only'); 135: require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address'); 136: require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address'); 137: require( 141: require( 145: require( 197: require( 201: require( 207: require(targets.length != 0, 'NounsDAO::propose: must provide actions'); 208: require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions'); 213: require( 217: require( 286: require( 312: require( 324: require( 347: require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal'); 350: require( 375: require(vetoer != address(0), 'NounsDAO::veto: veto power burned'); 376: require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer'); 377: 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: require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed'); 594: require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type'); 597: require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted'); 622: require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only'); 623: require( 638: require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only'); 639: require( 655: require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only'); 656: require( 674: require(msg.sender == admin, 'NounsDAO::_setMinQuorumVotesBPS: admin only'); 677: require( 682: require( 702: require(msg.sender == admin, 'NounsDAO::_setMaxQuorumVotesBPS: admin only'); 705: require( 709: require( 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');

10. = is cheaper than > (and <= cheaper than <)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <. Consider replacing strict inequalities with non-strict ones to save some gas here:

contracts/governance/NounsDAOLogicV2.sol (Before) 541: if (votes > 0) { (After) 541: if (votes >= 1) {

(Before) 967: if (pos > 0 && quorumParamsCheckpoints[pos - 1].fromBlock == blockNumber) { (After) 967: if (pos >= 1 && quorumParamsCheckpoints[pos - 1].fromBlock == blockNumber) {

11. Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated.

Use >> 1 instead of / 2 Use >> 2 instead of / 4 Use << 3 instead of * 8

contracts/governance/NounsDAOLogicV2.sol 951: uint256 center = upper - (upper - lower) / 2;

12. Splitting require() statements that use && saves gas

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 3 GAS per &&:

contracts/governance/NounsDAOLogicV2.sol 137: require( 141: require( 145: require( 201: require( 623: require( 639: require( 656: require( 677: require(

13. Use calldata instead of memory to save gas for read-only arguments

contracts/governance/NounsDAOLogicV2.sol 189: string memory description 536: string memory reason

14. Internal functions only called once can be inlined to save gas

contracts/governance/NounsDAOLogicV2.sol 866: function proposalCreationBlock(Proposal storage proposal) internal view returns (uint256) { 974: function _refundGas(uint256 startGas) internal { 1010: function getChainIdInternal() internal view returns (uint256) {