Nouns DAO contest - Tomio's results

Title: Gas savings for using solidity 0.8.10

Proof of Concept: All contract in scope

Recommended Mitigation Steps: Consider to upgrade pragma to at least 0.8.10.

Solidity 0.8.10 has a useful change which reduced gas costs of external calls Reference: here

---

Title: Reduce the size of error messages (Long revert Strings)

Impact: 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: NounsDAOProxy.sol#L79-L80 ERC721Checkpointable.sol#L140-L142 NounsDAOLogicV1.sol (various line)

Recommended Mitigation Steps: Consider shortening the revert strings to fit in 32 bytes

---

Title: Consider make constant as private to save gas

Proof of Concept: ERC721Checkpointable.sol#L41 ERC721Checkpointable.sol#L59-L63 NounsDAOLogicV1.sol#L67-L101

Recommended Mitigation Steps: I suggest changing the visibility from public to internal or private

---

Title: >= is cheaper than >

Impact: Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)

Proof of Concept: ERC721Checkpointable.sol#L153

Recommended Mitigation Steps: Consider using >= instead of > to avoid some opcodes

---

Title: Expression for constant values such as a call to keccak256(), should use immutable rather than constant

Proof of Concept: ERC721Checkpointable.sol#L59-L63 NounsDAOLogicV1.sol#L97-L101

Recommended Mitigation Steps: Change from constant to immutable reference: here

---

Title: Gas improvement on returning chainId value

Proof of Concept: ERC721Checkpointable.sol#L283

Recommended Mitigation Steps: by set chainId in returns L#282 and delete L#283 can save gas

function getChainId() internal view returns (uint256 chainId) { //@audit-info: set `chainId` here L#282
        //@audit-info: delete this L#283
        assembly {
            chainId := chainid()
        }
        return chainId;
    }

---

Title: Comparison operators

Proof of Concept: ERC721Checkpointable.sol#L142 NounsDAOLogicV1.sol#L126-L141

Recommended Mitigation Steps: Replace <= with <, and >= with > for gas optimization

---

Title: Custom errors from Solidity 0.8.4 are cheaper than revert strings

Impact: Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information

Custom errors are defined using the error statement reference: https://blog.soliditylang.org/2021/04/21/custom-errors/

Proof of Concept: NounsDAOProxy.sol#L79-L80 ERC721Checkpointable.sol#L140-L142 NounsDAOLogicV1.sol (various line)

Recommended Mitigation Steps: Replace require statements with custom errors.

---

Title: Gas optimization to dividing by 2

Proof of Concept: ERC721Checkpointable.sol#L184

Recommended Mitigation Steps: Replace / 2 with >> 1

Reference: here

---

Title: Using multiple require instead && can save gas

Proof of Concept: NounsDAOLogicV1.sol#L126-L141

Recommended Mitigation Steps: Change to:

	require(votingPeriod_ >= MIN_VOTING_PERIOD, 'NounsDAO::initialize: invalid voting period');
	require(votingPeriod_ <= MAX_VOTING_PERIOD, 'NounsDAO::initialize: invalid voting period');

---

Title: Using == false cost more gas

Proof of Concept: NounsDAOLogicV1.sol#L505

Recommended Mitigation Steps: Change to:

	require(!receipt.hasVoted, 'NounsDAO::castVoteInternal: voter already voted');

---

Title: Consider remove empty block

impact: The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

Proof of Concept: ounsDAOLogicV2.sol#L1030

---

Title: Default value initialization

Impact: If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept: NounsDAOLogicV2.sol#L292 NounsDAOLogicV2.sol#L330 NounsDAOLogicV2.sol#L357 NounsDAOLogicV2.sol#L382 NounsDAOLogicV2.sol#L948

Recommended Mitigation Steps: Remove explicit initialization for default values.

---

Title: abi.encode() is less efficient than abi.encodePacked()

Proof of Concept: NounsDAOLogicV2.sol#L313

---

Title: Using unchecked and prefix increment is more effective for gas saving:

Proof of Concept: NounsDAOLogicV2.sol#L292 NounsDAOLogicV2.sol#L330 NounsDAOLogicV2.sol#L357 NounsDAOLogicV2.sol#L382

Recommended Mitigation Steps: Change to:

    for (uint256 i = 0; i < proposal.targets.length;) {
          // ... 
    unchecked { ++i; }
      }

---