Nouns DAO contest - Sm4rty's results

A DAO-driven NFT project on Ethereum.

General Information

Platform: Code4rena

Start Date: 22/08/2022

Pot Size: $50,000 USDC

Total HM: 4

Participants: 160

Period: 5 days

Judge: gzeon

Total Solo HM: 2

Id: 155

League: ETH

Nouns DAO

Findings Distribution

Researcher Performance

Rank: 43/160

Findings: 2

Award: $52.41

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Nouns DAO

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0bi, 0x040, 0x1337, 0x1f8b, 0xDjango, 0xNazgul, 0xNineDec, 0xRajeev, 0xSky, 0xSmartContract, 0xbepresent, 0xkatana, 0xmatt, 8olidity, Aymen0909, Bjorn_bug, Bnke0x0, CertoraInc, Ch_301, Chom, CodingNameKiki, Deivitto, DevABDee, DimitarDimitrov, Dravee, ElKu, Funen, GalloDaSballo, GimelSec, Guardian, Haruxe, JC, JansenC, Jeiwan, JohnSmith, KIntern_NA, Lambda, LeoS, Noah3o6, Olivierdem, R2, RaymondFam, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Saintcode_, Sm4rty, SooYa, Soosh, TomJ, Tomo, Trabajo_de_mates, Waze, _Adam, __141345__, ajtra, android69, asutorufos, auditor0517, berndartmueller, bobirichman, brgltd, c3phas, cRat1st0s, carlitox477, catchup, cccz, csanuragjain, d3e4, delfin454000, dipp, djxploit, durianSausage, erictee, exd0tpy, fatherOfBlocks, gogo, hyh, ladboy233, lukris02, mics, mrpathfindr, natzuu, oyc_109, p_crypt0, pashov, pauliax, pfapostol, prasantgupta52, rajatbeladiya, rbserver, ret2basic, rfa, robee, rokinot, rvierdiiev, sach1r0, saian, seyni, shenwilly, sikorico, simon135, sryysryy, sseefried, throttle, tnevler, tonisives, wagmi, xiaoming90, yixxas, z3s, zkhorse, zzzitron

Awards

35.4387 USDC - $35.44

Labels

bug

QA (Quality Assurance)

edited-by-warden

External Links

Link to GitHub issue

1. Unused receive() function will lock Ether in contract

If the intention is for the Ether to be used, the function should call another function, otherwise, it should revert.

Instances:

NounsDAOLogicV2.sol:L1030

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

References

https://code4rena.com/reports/2022-05-sturdy#l-06-unused-receive-function-will-lock-ether-in-contract

2. Avoid using 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.

Instances:

NounsDAOLogicV2.sol:L53 NounsDAOInterfaces.sol:L33 NounsDAOLogicV1.sol:L61 NounsDAOProxy.sol:L36 ERC721Checkpointable.sol:L35 ERC721Enumerable.sol:L28

contracts/governance/NounsDAOLogicV2.sol:53:pragma solidity ^0.8.6;
contracts/governance/NounsDAOInterfaces.sol:33:pragma solidity ^0.8.6;
contracts/governance/NounsDAOLogicV1.sol:61: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.0;

Recommend using fixed solidity version

References:

https://code4rena.com/reports/2022-04-phuture#g-20-use-a-more-recent-version-of-solidity

3. Multiple initialization due to initialize function not having initializer modifier.

Description

The attacker can initialize the contract, take malicious actions, and allow it to be re-initialized by the project without any error being noticed.

Instances

NounsDAOLogicV2.sol:124 NounsDAOLogicV1.sol:113

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

References:

https://github.com/code-423n4/2021-12-perennial-findings/issues/13

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x040, 0x1f8b, 0xDjango, 0xNazgul, 0xNineDec, 0xSmartContract, 0xbepresent, 0xc0ffEE, 0xkatana, 2997ms, ACai, Amithuddar, Aymen0909, Ben, BipinSah, Bjorn_bug, Bnke0x0, CertoraInc, Ch_301, Chom, CodingNameKiki, Deivitto, DevABDee, DimitarDimitrov, Diraco, Dravee, ElKu, EthLedger, Fitraldys, Funen, GalloDaSballo, GimelSec, Guardian, IgnacioB, JC, JohnSmith, Junnon, KIntern_NA, Lambda, LeoS, Noah3o6, Olivierdem, Polandia94, R2, Randyyy, RaymondFam, Respx, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, Ruhum, SaharAP, Saintcode_, SerMyVillage, Shishigami, Sm4rty, SooYa, TomJ, Tomio, Tomo, Waze, Yiko, _Adam, __141345__, a12jmx, ajtra, ak1, bobirichman, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, catchup, ch0bu, d3e4, delfin454000, djxploit, durianSausage, erictee, exolorkistis, fatherOfBlocks, francoHacker, gogo, hyh, ignacio, jag, joestakey, karanctf, ladboy233, lucacez, lukris02, m_Rassska, martin, medikko, mics, mrpathfindr, natzuu, newfork01, oyc_109, pauliax, peritoflores, pfapostol, prasantgupta52, rbserver, ret2basic, rfa, robee, rokinot, rotcivegaf, rvierdiiev, sach1r0, saian, samruna, seyni, shark, shr1ftyy, sikorico, simon135, sryysryy, tay054, tnevler, wagmi, zishansami

Awards

16.9732 USDC - $16.97

Labels

bug

G (Gas Optimization)

edited-by-warden

External Links

Link to GitHub issue

1. Pre-increment costs less gas as compared to Post-increment :

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper)

i++ increments i and returns 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 , no need for temporary variable here

In the first case, the compiler has create a temporary variable (when used) for returning 1 instead of 2.

Instances:

NounsDAOLogicV2.sol:L226 NounsDAOLogicV1.sol:L216 NounsDAOLogicV2.sol:L292 NounsDAOLogicV2.sol:L330 NounsDAOLogicV2.sol:L357 NounsDAOLogicV2.sol:L382 NounsDAOLogicV1.sol:L281 NounsDAOLogicV1.sol:L319 NounsDAOLogicV1.sol:L346 NounsDAOLogicV1.sol:L371

contracts/governance/NounsDAOLogicV2.sol:226:        proposalCount++;
contracts/governance/NounsDAOLogicV1.sol:216:        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++) {
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++) {

Recommendations:

Change post-increment to pre-increment.

2. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

Instances:

NounsDAOLogicV2.sol:L292 NounsDAOLogicV2.sol:L330 NounsDAOLogicV2.sol:L357 NounsDAOLogicV2.sol:L382 NounsDAOLogicV1.sol:L281 NounsDAOLogicV1.sol:L319 NounsDAOLogicV1.sol:L346 NounsDAOLogicV1.sol:L371

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++) {
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++) {

Reference:

https://code4rena.com/reports/2022-05-cally#g-11-ii-should-be-uncheckediuncheckedi-when-it-is-not-possible-for-them-to-overflow-as-is-the-case-when-used-in-for--and-while-loops

3. An array's length should be cached to save gas in for-loops

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

Instances:

NounsDAOLogicV2.sol:L292 NounsDAOLogicV2.sol:L330 NounsDAOLogicV2.sol:L357 NounsDAOLogicV2.sol:L382 NounsDAOLogicV1.sol:L281 NounsDAOLogicV1.sol:L319 NounsDAOLogicV1.sol:L346 NounsDAOLogicV1.sol:L371

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++) {
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++) {

Remediation:

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead.

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

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

Instances include:

NounsDAOLogicV2.sol:L819 NounsDAOLogicV1.sol:L617

contracts/governance/NounsDAOLogicV2.sol:819:        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
contracts/governance/NounsDAOLogicV1.sol:617:        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');

Mitigation:

Breakdown each condition in a separate require statement (though require statements should be replaced with custom errors)

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

Instances Includes:

contracts/governance/NounsDAOLogicV2.sol:133:        require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once');
contracts/governance/NounsDAOLogicV2.sol:134:        require(msg.sender == admin, 'NounsDAO::initialize: admin only');
contracts/governance/NounsDAOLogicV2.sol:135:        require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address');
contracts/governance/NounsDAOLogicV2.sol:136:        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
contracts/governance/NounsDAOLogicV2.sol:207:        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
contracts/governance/NounsDAOLogicV2.sol:208:        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
contracts/governance/NounsDAOLogicV2.sol:347:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
contracts/governance/NounsDAOLogicV2.sol:375:        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
contracts/governance/NounsDAOLogicV2.sol:376:        require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');
contracts/governance/NounsDAOLogicV2.sol:377:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');
contracts/governance/NounsDAOLogicV2.sol:433:        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
contracts/governance/NounsDAOLogicV2.sol:577:        require(signatory != address(0), 'NounsDAO::castVoteBySig: invalid signature');
contracts/governance/NounsDAOLogicV2.sol:593:        require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed');
contracts/governance/NounsDAOLogicV2.sol:594:        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');
contracts/governance/NounsDAOLogicV2.sol:597:        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
contracts/governance/NounsDAOLogicV2.sol:622:        require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');
contracts/governance/NounsDAOLogicV2.sol:638:        require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only');
contracts/governance/NounsDAOLogicV2.sol:655:        require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only');
contracts/governance/NounsDAOLogicV2.sol:674:        require(msg.sender == admin, 'NounsDAO::_setMinQuorumVotesBPS: admin only');
contracts/governance/NounsDAOLogicV2.sol:702:        require(msg.sender == admin, 'NounsDAO::_setMaxQuorumVotesBPS: admin only');
contracts/governance/NounsDAOLogicV2.sol:727:        require(msg.sender == admin, 'NounsDAO::_setQuorumCoefficient: admin only');
contracts/governance/NounsDAOLogicV2.sol:801:        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');
contracts/governance/NounsDAOLogicV2.sol:819:        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
contracts/governance/NounsDAOLogicV2.sol:840:        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
contracts/governance/NounsDAOLogicV2.sol:853:        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');
contracts/governance/NounsDAOLogicV1.sol:122:        require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once');
contracts/governance/NounsDAOLogicV1.sol:123:        require(msg.sender == admin, 'NounsDAO::initialize: admin only');
contracts/governance/NounsDAOLogicV1.sol:124:        require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address');
contracts/governance/NounsDAOLogicV1.sol:125:        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
contracts/governance/NounsDAOLogicV1.sol:197:        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
contracts/governance/NounsDAOLogicV1.sol:198:        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
contracts/governance/NounsDAOLogicV1.sol:336:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
contracts/governance/NounsDAOLogicV1.sol:364:        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
contracts/governance/NounsDAOLogicV1.sol:365:        require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');
contracts/governance/NounsDAOLogicV1.sol:366:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');
contracts/governance/NounsDAOLogicV1.sol:422:        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
contracts/governance/NounsDAOLogicV1.sol:485:        require(signatory != address(0), 'NounsDAO::castVoteBySig: invalid signature');
contracts/governance/NounsDAOLogicV1.sol:501:        require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed');
contracts/governance/NounsDAOLogicV1.sol:502:        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');
contracts/governance/NounsDAOLogicV1.sol:505:        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
contracts/governance/NounsDAOLogicV1.sol:530:        require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');
contracts/governance/NounsDAOLogicV1.sol:546:        require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only');
contracts/governance/NounsDAOLogicV1.sol:563:        require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only');
contracts/governance/NounsDAOLogicV1.sol:581:        require(msg.sender == admin, 'NounsDAO::_setQuorumVotesBPS: admin only');
contracts/governance/NounsDAOLogicV1.sol:599:        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');
contracts/governance/NounsDAOLogicV1.sol:617:        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
contracts/governance/NounsDAOLogicV1.sol:638:        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
contracts/governance/NounsDAOLogicV1.sol:651:        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');
contracts/governance/NounsDAOProxy.sol:79:        require(msg.sender == admin, 'NounsDAOProxy::_setImplementation: admin only');
contracts/governance/NounsDAOProxy.sol:80:        require(implementation_ != address(0), 'NounsDAOProxy::_setImplementation: invalid implementation address');
contracts/base/ERC721Checkpointable.sol:139:        require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
contracts/base/ERC721Checkpointable.sol:140:        require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
contracts/base/ERC721Checkpointable.sol:141:        require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
contracts/base/ERC721Checkpointable.sol:163:        require(blockNumber < block.number, 'ERC721Checkpointable::getPriorVotes: not yet determined');
contracts/base/ERC721Enumerable.sol:62:        require(index < ERC721.balanceOf(owner), 'ERC721Enumerable: owner index out of bounds');
contracts/base/ERC721Enumerable.sol:77:        require(index < ERC721Enumerable.totalSupply(), 'ERC721Enumerable: global index out of bounds');

Remediation:

I suggest replacing revert strings with custom errors.

6. Use Shift Right/Left instead of Division/Multiplication 2X

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity’s division operation also includes a division-by-0 prevention which is bypassed using shifting.

I suggest replacing / 2 with >> 1 here:

Instances

NounsDAOLogicV2.sol:L951 ERC721Checkpointable.sol:L183

contracts/governance/NounsDAOLogicV2.sol:951:            uint256 center = upper - (upper - lower) / 2;
contracts/base/ERC721Checkpointable.sol:183:            uint32 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-32-shift-right-instead-of-dividing-by-2

7. Strict inequalities (>) are more expensive than non-strict ones (>=)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas. I suggest using >= instead of > to avoid some opcodes here:

Instances:

NounsDAOLogicV2.sol:L198 NounsDAOLogicV2.sol:L758 NounsDAOLogicV1.sol:L188 ERC721Checkpointable.sol:L152 ERC721Checkpointable.sol:L217 ERC721Checkpointable.sol:L224

contracts/governance/NounsDAOLogicV2.sol:198:            nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold,
contracts/governance/NounsDAOLogicV2.sol:758:            newMinQuorumVotesBPS > MIN_QUORUM_VOTES_BPS_UPPER_BOUND
contracts/governance/NounsDAOLogicV1.sol:188:            nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold,
contracts/base/ERC721Checkpointable.sol:152:        return nCheckpoints > 0 ? checkpoints[account][nCheckpoints - 1].votes : 0;
contracts/base/ERC721Checkpointable.sol:217:                uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
contracts/base/ERC721Checkpointable.sol:224:                uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0;

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-31--is-cheaper-than

8. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0;

Instance Includes:

NounsDAOLogicV2.sol:L948 ERC721Checkpointable.sol:L41 ERC721Checkpointable.sol:L180

contracts/governance/NounsDAOLogicV2.sol:948:        uint256 lower = 0;
contracts/base/ERC721Checkpointable.sol:41:    uint8 public constant decimals = 0;
contracts/base/ERC721Checkpointable.sol:180:        uint32 lower = 0;

Recommendation:

I suggest removing explicit initializations for default values.

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

This results in the keccak operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas.

Instances:

NounsDAOLogicV2.sol:L105 NounsDAOLogicV1.sol:L101

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

References:

https://github.com/code-423n4/2021-10-slingshot-findings/issues/3

10. abi.encode() is less efficient than abi.encodepacked()

Use abi.encodepacked() instead of abi.encode()

Instances:

NounsDAOLogicV2.sol:L313 NounsDAOLogicV2.sol:L572 NounsDAOLogicV2.sol:L574 NounsDAOLogicV1.sol:L302 NounsDAOLogicV1.sol:L480 NounsDAOLogicV1.sol:L482 ERC721Checkpointable.sol:L134 ERC721Checkpointable.sol:L136

contracts/governance/NounsDAOLogicV2.sol:313:            !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
contracts/governance/NounsDAOLogicV2.sol:572:            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this))
contracts/governance/NounsDAOLogicV2.sol:574:        bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
contracts/governance/NounsDAOLogicV1.sol:302:            !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
contracts/governance/NounsDAOLogicV1.sol:480:            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this))
contracts/governance/NounsDAOLogicV1.sol:482:        bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
contracts/base/ERC721Checkpointable.sol:134:            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
contracts/base/ERC721Checkpointable.sol:136:        bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));

Reference:

https://code4rena.com/reports/2022-06-notional-coop#15-abiencode-is-less-efficient-than-abiencodepacked

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

Instances:

contracts/governance/NounsDAOLogicV2.sol:133:        require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once');
contracts/governance/NounsDAOLogicV2.sol:135:        require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address');
contracts/governance/NounsDAOLogicV2.sol:136:        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
contracts/governance/NounsDAOLogicV2.sol:207:        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
contracts/governance/NounsDAOLogicV2.sol:208:        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
contracts/governance/NounsDAOLogicV2.sol:347:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
contracts/governance/NounsDAOLogicV2.sol:375:        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
contracts/governance/NounsDAOLogicV2.sol:377:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');
contracts/governance/NounsDAOLogicV2.sol:433:        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
contracts/governance/NounsDAOLogicV2.sol:577:        require(signatory != address(0), 'NounsDAO::castVoteBySig: invalid signature');
contracts/governance/NounsDAOLogicV2.sol:593:        require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed');
contracts/governance/NounsDAOLogicV2.sol:594:        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');
contracts/governance/NounsDAOLogicV2.sol:597:        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
contracts/governance/NounsDAOLogicV2.sol:622:        require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');
contracts/governance/NounsDAOLogicV2.sol:638:        require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only');
contracts/governance/NounsDAOLogicV2.sol:655:        require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only');
contracts/governance/NounsDAOLogicV2.sol:674:        require(msg.sender == admin, 'NounsDAO::_setMinQuorumVotesBPS: admin only');
contracts/governance/NounsDAOLogicV2.sol:702:        require(msg.sender == admin, 'NounsDAO::_setMaxQuorumVotesBPS: admin only');
contracts/governance/NounsDAOLogicV2.sol:727:        require(msg.sender == admin, 'NounsDAO::_setQuorumCoefficient: admin only');
contracts/governance/NounsDAOLogicV2.sol:801:        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');
contracts/governance/NounsDAOLogicV2.sol:819:        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
contracts/governance/NounsDAOLogicV2.sol:840:        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
contracts/governance/NounsDAOLogicV2.sol:853:        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');
contracts/governance/NounsDAOLogicV1.sol:122:        require(address(timelock) == address(0), 'NounsDAO::initialize: can only initialize once');
contracts/governance/NounsDAOLogicV1.sol:124:        require(timelock_ != address(0), 'NounsDAO::initialize: invalid timelock address');
contracts/governance/NounsDAOLogicV1.sol:125:        require(nouns_ != address(0), 'NounsDAO::initialize: invalid nouns address');
contracts/governance/NounsDAOLogicV1.sol:197:        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
contracts/governance/NounsDAOLogicV1.sol:198:        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
contracts/governance/NounsDAOLogicV1.sol:336:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
contracts/governance/NounsDAOLogicV1.sol:364:        require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
contracts/governance/NounsDAOLogicV1.sol:366:        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');
contracts/governance/NounsDAOLogicV1.sol:422:        require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
contracts/governance/NounsDAOLogicV1.sol:485:        require(signatory != address(0), 'NounsDAO::castVoteBySig: invalid signature');
contracts/governance/NounsDAOLogicV1.sol:501:        require(state(proposalId) == ProposalState.Active, 'NounsDAO::castVoteInternal: voting is closed');
contracts/governance/NounsDAOLogicV1.sol:502:        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');
contracts/governance/NounsDAOLogicV1.sol:505:        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
contracts/governance/NounsDAOLogicV1.sol:530:        require(msg.sender == admin, 'NounsDAO::_setVotingDelay: admin only');
contracts/governance/NounsDAOLogicV1.sol:546:        require(msg.sender == admin, 'NounsDAO::_setVotingPeriod: admin only');
contracts/governance/NounsDAOLogicV1.sol:563:        require(msg.sender == admin, 'NounsDAO::_setProposalThresholdBPS: admin only');
contracts/governance/NounsDAOLogicV1.sol:581:        require(msg.sender == admin, 'NounsDAO::_setQuorumVotesBPS: admin only');
contracts/governance/NounsDAOLogicV1.sol:599:        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');
contracts/governance/NounsDAOLogicV1.sol:617:        require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');
contracts/governance/NounsDAOLogicV1.sol:638:        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
contracts/governance/NounsDAOLogicV1.sol:651:        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');
contracts/governance/NounsDAOProxy.sol:79:        require(msg.sender == admin, 'NounsDAOProxy::_setImplementation: admin only');
contracts/governance/NounsDAOProxy.sol:80:        require(implementation_ != address(0), 'NounsDAOProxy::_setImplementation: invalid implementation address');
contracts/base/ERC721Checkpointable.sol:139:        require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
contracts/base/ERC721Checkpointable.sol:140:        require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
contracts/base/ERC721Checkpointable.sol:141:        require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
contracts/base/ERC721Checkpointable.sol:163:        require(blockNumber < block.number, 'ERC721Checkpointable::getPriorVotes: not yet determined');
contracts/base/ERC721Enumerable.sol:62:        require(index < ERC721.balanceOf(owner), 'ERC721Enumerable: owner index out of bounds');
contracts/base/ERC721Enumerable.sol:77:        require(index < ERC721Enumerable.totalSupply(), 'ERC721Enumerable: global index out of bounds');

Recommendations:

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

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:

12. Boolean Comparision

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here

Instances:

NounsDAOLogicV2.sol:L597 NounsDAOLogicV1.sol:L505

contracts/governance/NounsDAOLogicV2.sol:597:        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');
contracts/governance/NounsDAOLogicV1.sol:505:        require(receipt.hasVoted == false, 'NounsDAO::castVoteInternal: voter already voted');

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-04-fundingonlywhenpricenotflagged-boolean-comparison-147

13. Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

Instances:

NounsDAOLogicV2.sol:L53 NounsDAOInterfaces.sol:L33 NounsDAOLogicV1.sol:L61 NounsDAOProxy.sol:L36 ERC721Checkpointable.sol:L35 ERC721Enumerable.sol:L28

contracts/governance/NounsDAOLogicV2.sol:53:pragma solidity ^0.8.6;
contracts/governance/NounsDAOInterfaces.sol:33:pragma solidity ^0.8.6;
contracts/governance/NounsDAOLogicV1.sol:61: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.0;

References:

https://code4rena.com/reports/2022-06-notional-coop/#10-use-a-more-recent-version-of-solidity

Nouns DAO contest - Sm4rty's results

A DAO-driven NFT project on Ethereum.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-68] QA Report - $35.44

Findings Information

Awards

Labels

External Links

1. Unused receive() function will lock Ether in contract

Instances:

References

2. Avoid using Floating Pragma:

Instances:

References:

3. Multiple initialization due to initialize function not having initializer modifier.

Description

Instances

References:

[G-24] Gas Report - $16.97

Findings Information

Awards

Labels

External Links

1. Pre-increment costs less gas as compared to Post-increment :

Instances:

Recommendations:

2. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

Instances:

Reference:

3. An array's length should be cached to save gas in for-loops

Instances:

Remediation:

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

Instances include:

Mitigation:

5. Custom Errors instead of Revert Strings to save Gas

Instances Includes:

Remediation:

6. Use Shift Right/Left instead of Division/Multiplication 2X

Instances

References:

7. Strict inequalities (>) are more expensive than non-strict ones (>=)

Instances:

References:

8. Variables: No need to explicitly initialize variables with default values

Instance Includes:

Recommendation:

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

Instances:

References:

10. abi.encode() is less efficient than abi.encodepacked()

Instances:

Reference:

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

Instances:

Recommendations:

12. Boolean Comparision

Instances:

References:

13. Use a more recent version of solidity

Instances:

References: