Nouns Builder contest - pedr02b2's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L69 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L594

Vulnerability details

Vetoe

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L594

At this point in the Governer.sol contract #line594 it is possible to create and burn a vetoer, how ever, it would appear the ability to burn a vetoer and set the address of vetoer to 0 and leave the governance without a vetoer is possible, as no checks are carried out when burning said vetoer, there are also no checks at the start of setting up governance to ensure there is a vetoer either.

There is a 0 address check when storing governance setting for _treasury if (_treasury == address(0)) revert ADDRESS_ZERO(); and _token if (_token == address(0)) revert ADDRESS_ZERO(); , but you do not check to see if the there is a vetoer address, add into the "if" statements the following to mitigate.

// Ensure non-zero addresses are provided if (_vetoer == adress(0)) revert ADDRESS_ZERO();

Or simply combine the create and burn vetoer functions and create a 0 address check so that if a vetoer is burnt another is created so that governance is never left without one.

This could have a massive impact if onlyOwner was to burn a vetoer and then forget to create another, especially if an unsavoury character was to create some type of dangerous proposal, maybe one that drains funds or similar and there is no way of stopping the proposal from going through.

I still queston the Vetoe function within a DAO as handing power to one party mesans the DAO is no longer decentralised, how ever i do understand the need for one under theses circumstances.

(sidenote) Maybe the dev could find a way to write into the project a way of a vetoe vote, than can be cast by mulitple members, in order to keep the decentralised nature of a DAO, rather than giving power to a centralised agent for what ever reason is required.

     @notice Updates the vetoer
     @param _newVetoer The new vetoer address
function updateVetoer(address _newVetoer) external onlyOwner {
    if (_newVetoer == address(0)) revert ADDRESS_ZERO();

    emit VetoerUpdated(settings.vetoer, _newVetoer);

    settings.vetoer = _newVetoer;
}

/// @notice Burns the vetoer
function burnVetoer() external onlyOwner {
    emit VetoerUpdated(settings.vetoer, address(0));

    delete settings.vetoer;
}

No limits to maximum auction duration

/// @param _duration The duration of each auction

There are no checks to see if the auction time has been set by some unreasonable character to unreasonable end time, or even by human error, maybe a check could be put in place to limit the max time of an auction to mitigate the posibility of this happening by accident or other wise.

mitigate by creating 1/3/7 day auction times as a standard

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L52

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L58

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L77

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L281

Fix pragma version before deployment to mainet

src= https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

ALL files under "lib/interfaces/" source file (total 9 contracts), ALL files under "lib/proxy" (total 3 contracts), ALL files under "lib/token" (total 2 contracts), ALL files under "lib/utils/" (total 8 contracts)