[M1] quorumVotes() at NounsDAOLogicV2 returns zero for invalid proposalId

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L877-L889

Vulnerability details

Impact

External protocol or contract can be negatively impacted.

PoC

quorumVotes is external so it can be called from outside. At this time it returns zero for incorrect proposalId .

function quorumVotes(uint256 proposalId) public view returns (uint256) {
    Proposal storage proposal = _proposals[proposalId];
    if (proposal.totalSupply == 0) {
        return proposal.quorumVotes;  @audit will return zero here if invalid proposal Id
    }

Returning zero for incorrect proposalId is at least a bad idea from design point of view.

According to documentation

quorumVotes Rules:

• Given a proposal ID, the quorumVotes function must return the number of votes required for the proposal to succeed.
• quorumVotes must return the minimum quorum if the proposal was created prior to the deployment of NounsDAOLogicV2.
• quorumVotes must read the checkpointed configuration values from the proposal creation block.

If you return zero that means the external contract or protocol will think that needs zero votes to reach the quorum so it might cause an impact in the external code.

If you really plan to let this you should add to your documentation "Returns zero for invalid proposalId" so external contract can handle that .

Recommended:

Revert for invalid proposal id .

require(proposalId<=proposalCount && proposalId!=0, "Incorrect proposalId");

or add this behaviour in your documentation

[Shungy]

This is a nice suggestion. But there is no standard for quorumVotes function, so reverting or not would be solely up to the preference of the NounsDAO team. So it is the responsibility of the devs of the external contracts that interact with this contract to read the code and determine which return value means what.

[davidbrai]

agree with @Shungy