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
Rank: 67/160
Findings: 2
Award: $52.11
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
35.4484 USDC - $35.45
NounsDAOLogicV2.sol::86 Comment states 4_000 basis points but it’s really 6_000
NounsDAOLogicV2.sol::263 Comment states minQuorumVotes is always zero but that doesn’t seem to be the case
Internal functions should begin with a preceding _:
External functions should not follow internal naming conventions:
Redundant boolean checks (the variable being checked is a boolean value itself, no need for comparison):
Typo: NounsDAOLogicV1::46 veteor
NounsDAOLogicV1.sol Can make an onlyVetoer modifier to avoid repeating require statements
NounsDAOLogicV2.sol can replace all of the require(msg.sender == admin, 'NounsDAO::_setMinQuorumVotesBPS: admin only');
statements with the defined AdminOnly error or make a modifier to de-duplicate this logic.
NounsDAOLogicV2.sol::89 MAX_QUORUM_VOTES_BPS is unused
Functions can be declared external:
Centralization risk for the DAO logic V1 and V2:
🌟 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
16.6568 USDC - $16.66
NounsDAOLogicV1::480 can use block.chainid instead of getChainIdInternal to save ~40 gas – also makes the contract shorter for deployment.
NounsDAOLogicV2.sol::572 can use block.chainid instead of getChainIdInternal to save ~40 gas – also makes the contract shorter for deployment.
NounsDAOLogicV2::603,605,607 += is more efficient for these additions
NounsDAOLogicV2.sol::966 pos can be a uint32 since an invariant is that quorumParamsCheckpoints.length < block.number.
NounsDAOLogicV2.sol::924 len can be a uint32 since an invariant is that quorumParamsCheckpoints.length < block.number. Similarly lower and upper can be uint32 as well.
Variables should not be initialized to defaults (uint256 default is 0): contracts/base/ERC721Checkpointable.sol::181 => uint32 lower = 0; 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++) { 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/NounsDAOLogicV2.sol::948 => uint256 lower = 0;
Length of array should be computed outside of for-loop: contracts/base/ERC721Enumerable.sol::70 => return _allTokens.length; contracts/base/ERC721Enumerable.sol::121 => uint256 length = ERC721.balanceOf(to); contracts/base/ERC721Enumerable.sol::122 => _ownedTokens[to][length] = tokenId; contracts/base/ERC721Enumerable.sol::123 => _ownedTokensIndex[tokenId] = length; contracts/base/ERC721Enumerable.sol::131 => _allTokensIndex[tokenId] = _allTokens.length; contracts/base/ERC721Enumerable.sol::172 => uint256 lastTokenIndex = _allTokens.length - 1; contracts/governance/NounsDAOLogicV1.sol::192 => targets.length == values.length && contracts/governance/NounsDAOLogicV1.sol::193 => targets.length == signatures.length && contracts/governance/NounsDAOLogicV1.sol::194 => targets.length == calldatas.length, 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::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++) { contracts/governance/NounsDAOLogicV2.sol::202 => targets.length == values.length && contracts/governance/NounsDAOLogicV2.sol::203 => targets.length == signatures.length && contracts/governance/NounsDAOLogicV2.sol::204 => targets.length == calldatas.length, 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::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/NounsDAOLogicV2.sol::924 => uint256 len = quorumParamsCheckpoints.length; contracts/governance/NounsDAOLogicV2.sol::966 => uint256 pos = quorumParamsCheckpoints.length;
!= is more efficient than > 0 for uint comparisons: contracts/base/ERC721Checkpointable.sol::153 => return nCheckpoints > 0 ? checkpoints[account][nCheckpoints - 1].votes : 0; contracts/base/ERC721Checkpointable.sol::215 => if (srcRep != dstRep && amount > 0) { contracts/base/ERC721Checkpointable.sol::218 => uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0; contracts/base/ERC721Checkpointable.sol::225 => uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0; contracts/base/ERC721Checkpointable.sol::243 => if (nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber) { contracts/governance/NounsDAOLogicV2.sol::495 => * Users with > 0 votes receive refunds. Refunds are partial when using a gas priority fee higher than the DAO's cap. contracts/governance/NounsDAOLogicV2.sol::509 => * Users with > 0 votes receive refunds. Refunds are partial when using a gas priority fee higher than the DAO's cap. contracts/governance/NounsDAOLogicV2.sol::541 => if (votes > 0) { contracts/governance/NounsDAOLogicV2.sol::967 => if (pos > 0 && quorumParamsCheckpoints[pos - 1].fromBlock == blockNumber) {
Switching from division/multiplication to right-shift/left-shift can save gas: contracts/base/ERC721Checkpointable.sol::184 => uint32 center = upper - (upper - lower) / 2; // ceil, avoiding overflow contracts/governance/NounsDAOLogicV1.sol::88 => uint256 public constant MIN_QUORUM_VOTES_BPS = 200; // 200 basis points or 2% contracts/governance/NounsDAOLogicV1.sol::91 => uint256 public constant MAX_QUORUM_VOTES_BPS = 2_000; // 2,000 basis points or 20% contracts/governance/NounsDAOLogicV2.sol::80 => uint256 public constant MIN_QUORUM_VOTES_BPS_LOWER_BOUND = 200; // 200 basis points or 2% contracts/governance/NounsDAOLogicV2.sol::83 => uint256 public constant MIN_QUORUM_VOTES_BPS_UPPER_BOUND = 2_000; // 2,000 basis points or 20% contracts/governance/NounsDAOLogicV2.sol::86 => uint256 public constant MAX_QUORUM_VOTES_BPS_UPPER_BOUND = 6_000; // 4,000 basis points or 60% contracts/governance/NounsDAOLogicV2.sol::89 => uint256 public constant MAX_QUORUM_VOTES_BPS = 2_000; // 2,000 basis points or 20% contracts/governance/NounsDAOLogicV2.sol::951 => uint256 center = upper - (upper - lower) / 2;