Nouns DAO contest - Ch_301'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: 16/160

Findings: 3

Award: $1,094.57

🌟 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: TomJ

Also found by: 0xDjango, 0xSmartContract, Aymen0909, Ch_301, Deivitto

Labels

bug

duplicate

2 (Med Risk)

Awards

1039.0663 USDC - $1,039.07

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L839-L845

Vulnerability details

Impact

the vetoer can burn the Veto Power by mistake without invoking the _burnVetoPower()

Proof of Concept

the vetoer could make a mistake by passing address(0x0) when he tries to set a new vetoer address by invoking _setVetoer()

Recommended Mitigation Steps

Add check for address(0x0)

#0 - davidbrai
2022-08-29T15:42:43Z

Duplicate of #315

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

38.8441 USDC - $38.84

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

add a `require()` rather than `if()` and update the function pattern

This function have a bad pattern castRefundableVoteInternal() By updating it, castRefundableVote() and castRefundableVoteWithReason() will get a clear response

Finding

File: contracts/governance/NounsDAOLogicV2.sol

        uint256 startGas = gasleft();
        uint96 votes = castVoteInternal(msg.sender, proposalId, support);
        emit VoteCast(msg.sender, proposalId, support, votes, reason);
        if (votes > 0) {
            _refundGas(startGas);
        }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L533-L544

Recommended Mitigation Steps

The function will be like this

               uint96 votes = castVoteInternal(msg.sender, proposalId, support);
                require(votes > 0, ' ….');
                emit VoteCast(msg.sender, proposalId, support, votes, reason);
                _refundGas(gasleft());

`castVoteWithReason()` move it up

For better flow (when someone reads the code) and organization ## Finding

File: contracts/governance/NounsDAOLogicV2.sol

function castVoteWithReason(
        uint256 proposalId,
        uint8 support,
        string calldata reason
    ) external {
        emit VoteCast(msg.sender, proposalId, support, castVoteInternal(msg.sender, proposalId, support), reason);
    }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L552-L558

Recommended Mitigation Steps

it better to move this function up to line 492 after castVote()

add check for `votes == 0`

any user can try to vote without having any votes so the will be receipt.hasVoted = trueand receipt.support will take one of these numbers 2||1||0 and receipt.votes = 0

Finding

File:      contracts/governance/NounsDAOLogicV2.sol

uint96 votes = nouns.getPriorVotes(voter, proposalCreationBlock(proposal));

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L600

Recommended Mitigation Steps

Add a require

require(votes >0, '...');

So it will be like this

uint96 votes = nouns.getPriorVotes(voter, proposalCreationBlock(proposal));
require(votes >0, '...');

#check if the _withdraw() is successful or not

Finding

File: contracts/governance/NounsDAOLogicV2.sol

 (bool sent, ) = msg.sender.call{ value: amount }('');

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L789

Recommended Mitigation Steps

Add check

  (bool success, ) = msg.sender.call{amount}("");
   require(success, "Transfer failed.");

Missing checks for address(0x0)

Finding

File: contracts/governance/NounsDAOLogicV2.sol

    function _setPendingAdmin(address newPendingAdmin) external {
        // Check caller = admin
        require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');

        // Save current value, if any, for inclusion in log
        address oldPendingAdmin = pendingAdmin;

        // Store pendingAdmin with value newPendingAdmin
        pendingAdmin = newPendingAdmin;

        // Emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin)
        emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);
    }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L799-L811

Recommended Mitigation Steps

Add this check

require(msg.sender == admin, 'NounsDAO::_setPendingAdmin: admin only');

require(newPendingAdmin != address(0x0) , "...");

add a `timeLock` for `_burnVetoPower()`

it is better to add a timeLock here for more trusting

Finding

File:    contracts/governance/NounsDAOLogicV2.sol

function _burnVetoPower() public {
        // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');

        _setVetoer(address(0));
    }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L851-L856

Recommended Mitigation Steps

add a timeLock

this function needs to be `external`

Finding

File: contracts/governance/NounsDAOLogicV2.sol

function _burnVetoPower() public {...

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L851

wrong comment

Finding

File: contracts/governance/NounsDAOLogicV2.sol

        // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L852

unnecessarily check `proposalCreationBlock()`

The if (proposal.creationBlock == 0) statement on proposalCreationBlock() will will never be true That is because it’s an internal function called only by castVoteInternal() which granite the proposalId is exists by invoking state() Here require(state(proposalId) == ProposalState.Active, '...'); So we can delete proposalCreationBlock() if they need it only with castVoteInternal() and suffice with proposal.creationBlock directly

Finding

File:  contracts/governance/NounsDAOLogicV2.sol

        if (proposal.creationBlock == 0) {
            return proposal.startBlock - votingDelay;
        }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L867-L869

#unnecessarily check quorumVotes() if (proposal.totalSupply == 0) this only will be true if the proposalIdis doesn't exist yet and this return proposal.quorumVotes will be always 0

Finding

File:  contracts/governance/NounsDAOLogicV2.sol

        if (proposal.totalSupply == 0) {
            return proposal.quorumVotes;
        }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L879-L881

Recommended Mitigation Steps

add check for the proposalIdis exists or not

require(proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');

need to unify styles between `_setDynamicQuorumParams()` and `_setMaxQuorumVotesBPS(), _setMinQuorumVotesBPS()`

_setDynamicQuorumParams() use < and > to check the values and _setMaxQuorumVotesBPS(), _setMinQuorumVotesBPS() use <= and >=

Finding

File:  contracts/governance/NounsDAOLogicV2.sol

 function _setMinQuorumVotesBPS(uint16 newMinQuorumVotesBPS) external…
 function _setMaxQuorumVotesBPS(uint16 newMaxQuorumVotesBPS) external {...
 function _setDynamicQuorumParams(...

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L673-L720 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L748-L781

Recommended Mitigation Steps

One of them needs to edit also update their comments to be consistent

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

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

Update the `castRefundableVoteInternal()` function pattern

This function have a bad pattern castRefundableVoteInternal() By updating it, castRefundableVote() and castRefundableVoteWithReason() will save more gas

Finding

File: contracts/governance/NounsDAOLogicV2.sol

        uint256 startGas = gasleft();
        uint96 votes = castVoteInternal(msg.sender, proposalId, support);
        emit VoteCast(msg.sender, proposalId, support, votes, reason);
        if (votes > 0) {
            _refundGas(startGas);
        }

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L533-L544

Recommended Mitigation Steps

The function will be like this

               uint96 votes = castVoteInternal(msg.sender, proposalId, support);
                require(votes > 0, ' ….');
                emit VoteCast(msg.sender, proposalId, support, votes, reason);
                _refundGas(gasleft());

Duplicated `require()/revert()` checks should be refactored to a modifier

Finding

File:  contracts/governance/NounsDAOLogicV2.sol

require(msg.sender == admin, 'NounsDAO::_setMaxQuorumVotesBPS: admin only');

 if (msg.sender != admin) {
            revert AdminOnly();
        }

require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');

Recommended Mitigation Steps

Replace them with a modifier

no need for the second part on this check

Finding

File:  contracts/governance/NounsDAOLogicV2.sol

 require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L819

use `Memory` rather than `storage`

Finding

File: contracts/governance/NounsDAOLogicV2.sol

Proposal storage proposal = _proposals[proposalId];

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L878

Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas

Finding

File: contracts/governance/NounsDAOLogicV2.sol

    function safe32(uint256 n, string memory errorMessage) internal pure returns (uint32) {

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L1018

zero-initialization

Leaving the variables on the default variable bool = false and unit = 0 will save you some gas

Finding

File:  contracts/governance/NounsDAOLogicV2.sol

        newProposal.eta = 0;
        newProposal.forVotes = 0;
        newProposal.againstVotes = 0;
        newProposal.abstainVotes = 0;
        newProposal.canceled = false;
        newProposal.executed = false;
        newProposal.vetoed = false;

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L238-L243 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L231

Nouns DAO contest - Ch_301's results

A DAO-driven NFT project on Ethereum.

General Information

Findings Distribution

Researcher Performance

External Links

[M-03] the `vetoer` could burn the Veto Power by mistake - $1,039.07

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Recommended Mitigation Steps

#0 - davidbrai2022-08-29T15:42:43Z

[Q-17] QA Report - $38.84

Findings Information

Awards

Labels

External Links

add a require() rather than if() and update the function pattern

Finding

Recommended Mitigation Steps

castVoteWithReason() move it up

Recommended Mitigation Steps

add check for votes == 0

Finding

Recommended Mitigation Steps

Finding

Recommended Mitigation Steps

Missing checks for address(0x0)

Finding

Recommended Mitigation Steps

add a timeLock for _burnVetoPower()

Finding

Recommended Mitigation Steps

this function needs to be external

Finding

wrong comment

Finding

unnecessarily check proposalCreationBlock()

Finding

Finding

Recommended Mitigation Steps

need to unify styles between _setDynamicQuorumParams() and _setMaxQuorumVotesBPS(), _setMinQuorumVotesBPS()

Finding

Recommended Mitigation Steps

[G-98] Gas Report - $16.66

Findings Information

Awards

Labels

External Links

Update the castRefundableVoteInternal() function pattern

Finding

Recommended Mitigation Steps

Duplicated require()/revert() checks should be refactored to a modifier

Finding

Recommended Mitigation Steps

no need for the second part on this check

Finding

use Memory rather than storage

Finding

Using calldata instead of memory for read-only arguments in external functions saves gas

Finding

zero-initialization

Finding

#0 - davidbrai
2022-08-29T15:42:43Z

add a `require()` rather than `if()` and update the function pattern

`castVoteWithReason()` move it up

add check for `votes == 0`

add a `timeLock` for `_burnVetoPower()`

this function needs to be `external`

unnecessarily check `proposalCreationBlock()`

need to unify styles between `_setDynamicQuorumParams()` and `_setMaxQuorumVotesBPS(), _setMinQuorumVotesBPS()`

Update the `castRefundableVoteInternal()` function pattern

Duplicated `require()/revert()` checks should be refactored to a modifier

use `Memory` rather than `storage`

Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas