Nouns DAO contest - sach1r0'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: 77/160

Findings: 2

Award: $52.10

🌟 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.4386 USDC - $35.44

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

Non-Library/Interface files should use fixed compiler versions, not floating ones

Details

Contracts should be deployed with the same compiler version/flags where they have been tested with. Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version that might introduce bugs that affect the contract system negatively. see reference: https://github.com/code-423n4/2021-11-unlock-findings/issues/15, https://code4rena.com/reports/2022-03-paladin/ and https://swcregistry.io/docs/SWC-103

Mitigation

I suggest removing ^ in pragma solidity ^0.8.6 and change it to pragma solidity 0.8.15 to be consistent with the rest of the contracts.

Line of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L61 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L53 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOProxy.sol#L36

Lack of indexed parameters in events

Details

Some of the events throughout the codebase are not indexed. Indexing event parameters enable off-chain services to search and filter for specific events. see reference: Low severity finding from OpenZeppelin Audit of HoldeFi [L09] Lack of indexed parameters in events https://blog.openzeppelin.com/holdefi-audit/#low

Mitigation

Add the indexed keyword to the events.

Line of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOInterfaces.sol#L37-L47 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOInterfaces.sol#L50-L62

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.6573 USDC - $16.66

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

Breaking down multiple `require` statements instead of using `&&`

Details

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas. See reference: [G-09] of https://code4rena.com/reports/2022-04-backd/

Mitigation

I suggest breaking down multiple conditions into two require statement instead of using &&. Example: Changing from:

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

to:

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

Line of code

Custom Errors

Details

Starting from Solidity v0.8.4, using custom errors is a gas-efficient wat to explain to users why an operation failed. See reference: https://blog.soliditylang.org/2021/04/21/custom-errors/

Mitigation

I suggest making a custom error for example error Unauthorized();

Line of code

Functions that are not called within the contract must set its visibility to `external` instead of `public`

Details

Setting function's visibility to external when it is only called externally can save gas because external function’s parameters are not copied into memory and are instead read from calldata directly. see reference: https://github.com/code-423n4/2021-06-gro-findings/issues/37

Mitigation

Set function visibility to external

Line of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L113-L155 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L174-L211 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L124-L165 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L184-L279

Pre-increment cost less gas than post-increment

Details

proposalCount++; costs more gas than ++proposalCount; , for uint pre-decrement is cheaper than post-decrement see reference: https://github.com/code-423n4/2021-12-nftx-findings/issues/195

Mitigation

change proposalCount++; to ++proposalCount;

Line of code

Solidity compiler will always read the length of the array during each iteration

Details

.length in a loop can be extracted into a variable and used where necessary to reduce the number of storage reads see reference: https://github.com/code-423n4/2021-10-union-findings/issues/92

Mitigation:

This extra costs can be avoided by caching the array length. Example: uint256 proposalTargetsLength = proposal.targets.length; for (uint256 i = 0; i < proposalTargetsLength; i++) { }

Line of code

No need to explicitly initialize variables with their default values

Details

When variables are not set, it is assumed to have it's 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.

Mitigation

change uint256 i = 0; to uint256 i; see reference: https://code4rena.com/reports/2022-02-jpyc/ [G-07] GENERAL RECOMMENDATIONS

Nouns DAO contest - sach1r0's results

A DAO-driven NFT project on Ethereum.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-96] QA Report - $35.44

Findings Information

Awards

Labels

External Links

Non-Library/Interface files should use fixed compiler versions, not floating ones

Details

Mitigation

Line of code

Lack of indexed parameters in events

Details

Mitigation

Line of code

[G-55] Gas Report - $16.66

Findings Information

Awards

Labels

External Links

Breaking down multiple require statements instead of using &&

Details

Mitigation

Line of code

Custom Errors

Details

Mitigation

Line of code

Functions that are not called within the contract must set its visibility to external instead of public

Details

Mitigation

Line of code

Pre-increment cost less gas than post-increment

Details

Mitigation

Line of code

Solidity compiler will always read the length of the array during each iteration

Details

Mitigation:

Line of code

No need to explicitly initialize variables with their default values

Details

Mitigation

Line of code

Breaking down multiple `require` statements instead of using `&&`

Functions that are not called within the contract must set its visibility to `external` instead of `public`