sherlock-audit / 2024-03-nouns-dao-2-judging

1 stars 0 forks source link

jasonxiale - user who doesn't have vote power can votes #48

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

jasonxiale

medium

user who doesn't have vote power can votes

Summary

Function NounsDAOVotes.castVoteInternal doesn't check if voter has vote power. With checking if the voter has vote power, a malicious user can extend a proposal's endBlock in some case.

Vulnerability Detail

I will take NounsDAOLogicV4.castVote as an example. With in NounsDAOLogicV4.castVote, NounsDAOVotes.castVote is called. And then NounsDAOVotes.castVoteInternal is called.

In NounsDAOVotes.castVoteInternal:

193     function castVoteInternal(
194         NounsDAOTypes.Storage storage ds,
195         address voter,
196         uint256 proposalId,
197         uint8 support,
198         uint32 clientId
199     ) internal returns (uint96 votes) {
200         NounsDAOTypes.ProposalState proposalState = ds.stateInternal(proposalId);
201 
202         if (proposalState == NounsDAOTypes.ProposalState.Active) {
203             votes = castVoteDuringVotingPeriodInternal(ds, proposalId, voter, support);
204         } else if (proposalState == NounsDAOTypes.ProposalState.ObjectionPeriod) {
205             if (support != 0) revert CanOnlyVoteAgainstDuringObjectionPeriod();
206             votes = castObjectionInternal(ds, proposalId, voter);
207         } else {
208             revert('NounsDAO::castVoteInternal: voting is closed');
209         }
210 
211         NounsDAOTypes.ClientVoteData memory voteData = ds._proposals[proposalId].voteClients[clientId];
212         ds._proposals[proposalId].voteClients[clientId] = NounsDAOTypes.ClientVoteData({
213             votes: uint32(voteData.votes + votes),
214             txs: voteData.txs + 1
215         });
216     }
  1. Supposed the proposal's state is NounsDAOTypes.ProposalState.Active In such case, NounsDAOVotes.sol#L203 is executed. In NounsDAOVotes.castVoteDuringVotingPeriodInternal, the function calls ds.nouns.getPriorVotes to get the voter's voting power. And then objectionPeriodEndBlock is updated if the conditions are met

    But the issue is that the function doesn't check if the voter has voting power, so anyone can call this function to extend objectionPeriodEndBlock

  2. another issue about the zero voting-power is in NounsDAOVotes.sol#L214 txs: voteData.txs + 1 is updated. If a user doesn't have voting power, voteData.txs shouldn't be updated.

Impact

Some proposals' end time can be extended by users who don't have voting power, and the proposals' state isn't correct

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol#L193-L216

Tool used

Manual Review

Recommendation

diff --git a/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol b/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol index 78001b8..dc8b2f3 100644 --- a/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol +++ b/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol @@ -236,6 +236,7 @@ library NounsDAOVotes {

     uint96 votes = ds.nouns.getPriorVotes(voter, proposal.startBlock);
sherlock-admin3 commented 4 months ago

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

firstly; if the voter has no voting power (i.e. votes == 0); then it looks like nothing will change and they just waste money on running this function; therefore; end time won't be exteneded after it; as for your second point; I don't see how txs: voteData.txs + 1 harms anyone; yes it not good to update it if it's not needed; but I don't see it higher than Low/info