ether_sky - Users may call the updateRewardsForProposalWritingAndVoting function with incorrect parameters for votingClientIds.

[sherlock-admin3]

ether_sky

high

Users may call the updateRewardsForProposalWritingAndVoting function with incorrect parameters for votingClientIds.

Summary

The updateRewardsForProposalWritingAndVoting function is important and is really gas-intensive. And it's really hard to identify the correct votingClientIds parameter. We have getVotingClientIds function in order to determin the correct votingClientIds parameter. However this function returns incorrect results.

Vulnerability Detail

In the updateRewardsForProposalWritingAndVoting function, proposals that fail to meet the eligible quorum threshold are skipped.

function updateRewardsForProposalWritingAndVoting(
    uint32 lastProposalId,
    uint32[] calldata votingClientIds
) public whenNotPaused {
    for (uint256 i; i < proposals.length; ++i) {
        // make sure proposal finished voting
        uint endBlock = max(proposals[i].endBlock, proposals[i].objectionPeriodEndBlock);
        require(block.number > endBlock, 'all proposals must be done with voting');

        // skip non eligible proposals
        if (proposals[i].forVotes < (proposals[i].totalSupply * proposalEligibilityQuorumBps_) / 10_000) {  // @audit, here
            delete proposals[i];
            continue;
        }

        // proposal is eligible for reward
        ++t.numEligibleProposals;

        uint256 votesInProposal = proposals[i].forVotes + proposals[i].againstVotes + proposals[i].abstainVotes;
        t.numEligibleVotes += votesInProposal;
    }
}

However, this aspect is not taken into account in the getVotingClientIds function.

function getVotingClientIds(uint32 lastProposalId) public view returns (uint32[] memory) {
    RewardsStorage storage $ = _getRewardsStorage();

    uint256 numClientIds = nextTokenId();
    uint32[] memory allClientIds = new uint32[](numClientIds);
    for (uint32 i; i < numClientIds; ++i) {
        allClientIds[i] = i;
    }
    NounsDAOTypes.ProposalForRewards[] memory proposals = nounsDAO.proposalDataForRewards(
        $.nextProposalIdToReward,
        lastProposalId,
        allClientIds
    );

    uint32[] memory sumVotes = new uint32[](numClientIds);
    for (uint256 i; i < proposals.length; ++i) {
        for (uint256 j; j < numClientIds; ++j) {
            sumVotes[j] += proposals[i].voteData[j].votes;
        }
    }

    uint256 idx;
    uint32[] memory nonZeroClientIds = new uint32[](numClientIds);
    for (uint32 i; i < numClientIds; ++i) {
        if (sumVotes[i] > 0) nonZeroClientIds[idx++] = i;
    }

    assembly {
        mstore(nonZeroClientIds, idx)
    }

    return nonZeroClientIds;
}

As a result, the updateRewardsForProposalWritingAndVoting function can be reverted due to some client IDs have no eligible votes.

function updateRewardsForProposalWritingAndVoting(
    uint32 lastProposalId,
    uint32[] calldata votingClientIds
) public whenNotPaused {
    for (uint256 i = 0; i < didClientIdHaveVotes.length; ++i) {
        require(didClientIdHaveVotes[i], 'all clientId must have votes');
    }
}

Impact

Users' funds can be lost due to high gas fees(as this function is particularly gas-intensive), and manually identifying the revert reason will be really challenging.

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L358-L361 https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L494-L526 https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L437-L439

Tool used

Manual Review

Recommendation

Duplicate of #14

[sherlock-admin2]

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

WangAudit commented:

seems to be working correctly since it deletes non-eligible proposals; and in the second loop it only accounts only for eligible proposals

[eladmallel]

We think this issue is correct, at low severity, and is the same as #14

[WangSecurity]

Refer to #14