search

code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

`Governor` - Quorum could be less than intended #195

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

There could be tokens minted between the quorum computation and the vote, which would lead to a quorum lower than intended. It could be an issue at the beginning of the Token lifecycle, when the total supply is still low.

Proof of Concept

When creating a proposal in the Governor contract, proposal.quorumVotes is computed during the propose tx. However tokens could be minted after the propose in the same block. In this case, they'll still have a vote during the proposal due to how token.getPastVotes works, therefore the quorum will be lower than intended given the actual total supply.

Recommended Mitigation Steps

Either compute the total supply afterwards, for example at the beginning of the vote, either takes the vote with a timestamp of proposal.timeCreated - 1 to not count the block during which the tx was submitted.

GalloDaSballo commented 1 year ago

The Warden has shown how the quorum value can be manipulated because instead of checking for the totalSupply at time of creation, the Governor checks for a totalSupply that can change.

This is tied to the burning mechanism, so a refactor of both would be necessary.

Alternatively this could be a nofix, however admins and end users should be aware that the Quorum math will be inconsistent due to that check.

Because the quorum is tied to actions that can cause a leak of value or theft, I agree with Medium Severity

tbtstl commented 1 year ago

ACK on this confusion – but this is the current behaviour of both Nouns and Lil Nouns, so I'd say it is intentional for now – we may want to adjust this in a future upgrade.