Some voters will not be rewarded

Summary

Some voters who voted for the proposals will not be rewarded.

Vulnerability Detail

The only reasons why a voter cannot be rewarded is this:

// @param votingClientIds array of sorted client ids that were used to vote on the eligible proposals in
// this rewards distribution. reverts if contains duplicates. reverts if not sorted. reverts if a clientId had zero votes.

In other cases voter must be rewarded. But if voter submitted votes using functions castVoteBySig(), castVoteWithReason() or castVote(), he will not get rewards due to hardcoded clientId:

function castVote(NounsDAOTypes.Storage storage ds, uint256 proposalId, uint8 support) external {
        emit VoteCast(msg.sender, proposalId, support, castVoteInternal(ds, msg.sender, proposalId, support, 0), '');
    }

function castVoteInternal(
        NounsDAOTypes.Storage storage ds,
        address voter,
        uint256 proposalId,
        uint8 support,
 -->    uint32 clientId   <--
    ) internal returns (uint96 votes) {

        //...

        NounsDAOTypes.ClientVoteData memory voteData = ds._proposals[proposalId].voteClients[clientId];
        ds._proposals[proposalId].voteClients[clientId] = NounsDAOTypes.ClientVoteData({
            votes: uint32(voteData.votes + votes),
            txs: voteData.txs + 1
        });
    }

Votes by varriable clientId are saved in array, that will be used for rewards accounting in Rewards.sol:

NounsDAOTypes.ClientVoteData[] memory voteData = proposals[i].voteData;
            for (uint256 j; j < votingClientIds.length; ++j) {
                clientId = votingClientIds[j];
                uint256 votes = voteData[j].votes;
                didClientIdHaveVotes[j] = didClientIdHaveVotes[j] || votes > 0; 
                if (clientId != 0 && clientId <= t.maxClientId) {
                    m.inc(clientId, votes * t.rewardPerVote);
                }
                votesInProposal += votes;
            }

As we can see, voters with clientId = 0 will not be rewarded. It was made to avoid rewarding non-existed clientId(=0 or > maxClientId), but in reality it harms voters with existing clientId, because they was not allowed to pass it as a parameter in castVote() and other functions.

Impact

Many elgibile voters who voted for the elgibile proposals will not be rewarded.

Code Snippet

[https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol#L70]() [https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol#L145]() [https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/governance/NounsDAOVotes.sol#L164]()

Tool used

Manual Review

Recommendation

Allow users to pass clientId as a parameter in castVote() and other functions:

function castVote(
           NounsDAOTypes.Storage storage ds, 
           uint256 proposalId,  
           uint8 support,
+          uint32 clientId
   ) external {
-        emit VoteCast(msg.sender, proposalId, support, castVoteInternal(ds, msg.sender, proposalId, support, 0), '');
+        emit VoteCast(msg.sender, proposalId, support, castVoteInternal(ds, msg.sender, proposalId, support, clientId), '');
    }

sherlock-admin2 commented 4 months ago

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

WangAudit commented:

seems to be intended behaviour; if we take a look at castRefundableVoteWithReason it allows us to supply the clientId; If we look at castRefundableVoteInternal; devs say that client is the one facilitating posting the vote on chain; which looks to me to be specific for this function.

karanctf commented:

client id can't be 0

takarez commented:

seem invalid.

eladmallel commented 4 months ago

This is by design.

First to clarify, rewards go to apps that facilitate voting on Nouns, not to the voters themselves. In this context, of course we don't want to reward invalid client IDs :)

We reserved client ID 0 for "the current state" and not rewarding it is also by design - all clients that should be rewarded will register and receive IDs starting at 1.

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

DenTonylifer - Some voters will not be rewarded #7