Nouns Builder contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/governance/governor/Governor.sol#L468 https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/governance/governor/Governor.sol#L475](https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/governance/governor/Governor.sol#L475

Vulnerability details

Proof of concept

/// @notice The current number of votes required to submit a proposal
    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

If settings.token.totalSupply() is still low, let’s say 50 and proposal and quorum thresholds are set below 2000 then both functions will return zero, essentially allowing any proposal to be submitted and reach quorum without even 1 token holder voting for it even though there were proposal and quorum thresholds set.

Impact

The impact is that until there are enough tokens in total supply for the Governor contract, any proposal can be submitted, accepted and then after the delay it can gather quorum and be executed, all of this with zero votes for it. This can be a big concern for the DAO because pretty much any holder can execute any code.

Recommendation

Make both functions return some sensible MIN_VALUE if they amount to zero, for example MIN_PROPOSAL_THRESHOLD = 50, MIN_QUORUM_THRESHOLD = 150.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/lib/token/ERC721Votes.sol#L144

Vulnerability details

Proof of concept

If a user calls the delegateBySig() function with the zero address (for example unintentionally) as the value of the _to argument then the new delegate of the _from address will become the zero address, effectively burning his vote. Ideally if the _to argument has a value of the zero address it is best to be set as the _from address, so the user keeps his vote instead of burning it.

Impact

This vulnerability can result in undesired loss of governance power for a user.

Recommendation

On the first line of delegateBySig add the following code:

if (_to == address(0)) {
		_to = _from;
}

Non library/interface files should use fixed compiler version

All contracts under src/lib/utils are using floating pragma versions like

pragma solidity ^0.8.0;

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.