Dliteofficial - Proposal Submission, voting and auction rewards update could become really expensive in gas terms due to too many registered clients, approved or not

[sherlock-admin4]

Dliteofficial

high

Proposal Submission, voting and auction rewards update could become really expensive in gas terms due to too many registered clients, approved or not

Summary

With enough clients, Registered and unregistered, Auctions, and Proposal Submission and Voting can become really expensive (gas).

Vulnerability Detail

When the Rewards contract is being updated for both auctions, and proposal submission and voting rewards, updateRewardsForAuctions() and updateRewardsForProposalWritingAndVoting() rewards clients that facilitated a successful auction, proposal or participated in the voting. Particularly for updateRewardsForProposalWritingAndVoting(), only the clients in the successful proposal is rewarded for proposal submission. To do this, the function loops through the client registered and update their reward balance.

    function updateRewardsForAuctions(uint32 lastNounId) public whenNotPaused {
        .........
     uint256 numValues = m.numValues();
        for (uint32 i = 0; i < numValues; ++i) {
            ClientRewardsMemoryMapping.ClientBalance memory cb = m.getValue(i);
            uint256 reward = (cb.balance * auctionRewardBps) / 10_000;
            $._clientMetadata[cb.clientId].rewarded += SafeCast.toUint96(reward);

            emit ClientRewarded(cb.clientId, reward);
        ............
        }

The issue here is that the number of clients used in the loop is maxClient, which indicates that with more registered clients, approved or not, the number of times the loop happens also increases. With more loops comes more gas. And since a gas refund mechanism was implemented in the code, this is especially risky and could cause the refund value to be on the high side.

As an after thought, the number of clients could be intentionally increased by a malicious actor who could just call registerClient() multiple times to register as many unapproved clients as possible to grief the system.

Impact

Execution of reward update become really expensive. Plus, with gas refunds, the contract will definitely be draining more WETH than it ought to for gas refunds.

Code Snippet

Rewards::updateRewardsForAuctions()

    function updateRewardsForAuctions(uint32 lastNounId) public whenNotPaused {
        //code too long, see link for code
    }

Rewards::updateRewardsForProposalWritingAndVoting()

    function updateRewardsForProposalWritingAndVoting(uint32 lastNounId) public whenNotPaused {
        //code too long, see link for code
    }

Tool used

Manual Review

Recommendation

Create an array of approved clients which we can loop through rather than through the list of both approved and unsanctioned clients

[sherlock-admin4]

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

WangAudit commented:

invalid -> gas optimization

takarez commented:

this seem valid obersing the clinets can be added by anyone; high(1)

[Dliteofficial]

escalate

@WangSecurity please take a second look at this submission. The report may have come off as a gas optimization report, but it isn't one. These are the points to note:

1. As I have mentioned in the report, client Id can be increased by anyone which speaks to the feasibility of this vulnerability.
2. The short-term impact is expensive gas costs for function execution.
3. The long-term impact is the withdrawal, possibly the drain of the funds used for gas refunds. This functionality was added to incentivize the callers of the updateRewardsForAuctions() and updateRewardsForProposalWritingAndVoting().
4. An essential part of the protocol is also broken if this incentive becomes unavailable.

[sherlock-admin2]

escalate

@WangSecurity please take a second look at this submission. The report may have come off as a gas optimization report, but it isn't one. These are the points to note:

1. As I have mentioned in the report, client Id can be increased by anyone which speaks to the feasibility of this vulnerability.
2. The short-term impact is expensive gas costs for function execution.
3. The long-term impact is the withdrawal, possibly the drain of the funds used for gas refunds. This functionality was added to incentivize the callers of the updateRewardsForAuctions() and updateRewardsForProposalWritingAndVoting().
4. An essential part of the protocol is also broken if this incentive becomes unavailable.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

I still believe it should remain invalid. The entire purpose of the gas refund function is to refund gas for calling gas-intensive functions to incentivise users to call them. Therefore, it seems to me that it's working the way it was designed. There are sufficient checks to not refund too much gas.

Therefore:

As I have mentioned in the report, client Id can be increased by anyone which speaks to the feasibility of this vulnerability.

Just gas griefing other users -> invalid.

The short-term impact is expensive gas costs for function execution.

Gas optimization -> invalid

The long-term impact is the withdrawal, possibly the drain of the funds used for gas refunds. This functionality was added to incentivize the callers of the updateRewardsForAuctions() and updateRewardsForProposalWritingAndVoting().

I don't see how it drains the protocol, there are lots of checks for these functions to go through and the entire purpose of gas refund is to incentivise users call these functions, therefore, how are they draining the protocol if it should work like that?

An essential part of the protocol is also broken if this incentive becomes unavailable.

I don't see how this incentive is broken, tbh. Therefore, I believe this report should remain invalid.

[Dliteofficial]

Alright. I'll try to explain why this issue should be taken seriously.

Gas Refunds exist to incentivize addresses that call the auction, and proposal submission and voting rewards update function, and we can agree on that. I also want you to keep in mind that since the rewards contract doesn't accept deposits from users, the client's rewards and gas refund tokens are transferred by the DAO Treasury.

If the update function becomes too expensive to call, of which the gas cost is not the focus here, and either of the client's rewards or frequency of refunds deposited becomes affected, this becomes a problem, which is what I am explaining here.

Two things determine how inflated the gas cost of the update will be:

1. The interval between auction/proposal submission and voting update. I didn't mention this because I thought it was implicit and in the codebase

   require(
           lastNounId >= nextAuctionIdToReward_ + $.params.minimumAuctionsBetweenUpdates,
           'lastNounId must be higher'
       );

if (t.numEligibleProposals < $.params.numProposalsEnoughForReward) {
            require(
                t.lastProposal.creationTimestamp > $.lastProposalRewardsUpdate + $.params.minimumRewardPeriod,
                'not enough time passed'
            );
        }

2. The number of clients. With the incentive, the number of clients is bound to increase. Also, a malicious actor can artificially inflate the number of clients by registering as many clients as possible. They don't even have to be approved.

3. The actor/any other address calls the update, and the gas cost is magnified by the update interval and number of clients. This is enough to eat deep into the gas refunds purse, depending on how many tokens were transferred.

4. Another angle here is that the Reward contract holds the tokens for both client rewards and gas refunds, and since they are both paid of the same token and from the same purse, the high cost of gas could begin to eat into the rewards of the client when the refunds has been blown through.

5. The only reason some other addresses will agree to join the clients in calling the update function is because of the gas refunds. It is a basic yet implied assumption to ensure prompt updates. If the DAO Treasury refuses to fund the contract because of the high gas cost, the assumption is broken.

Hopefully, this answers all your questions. If not, I think I'd like the opinion of the head of judging here if you don't mind @WangSecurity

[WangSecurity]

Hm, but do we actually loop through all the clientIds? As I understand we don't loop through all clients, we only consider the votingClientIds, i.e. clients for eligible proposals only, correct? (don't get me wrong, I'm jsut trying to understand your reasoning better, thank you very much for a detailed explanation, just have little questions here.).

[Dliteofficial]

No we actually loop through all the clients.

Here, we use the ClientRewardsMemoryMapping Library to create a mapping with maxClientId which is all registered clients in the system:

ClientRewardsMemoryMapping.Mapping memory m = ClientRewardsMemoryMapping.createMapping({
            maxClientId: maxClientId
        });

Then we loop through them all, performing calculations on them all, only recording non-zero values for clients that have balances

        uint256 numValues = m.numValues();
        for (uint32 i = 0; i < numValues; ++i) {
            ClientRewardsMemoryMapping.ClientBalance memory cb = m.getValue(i);
            uint256 reward = (cb.balance * auctionRewardBps) / 10_000;
            $._clientMetadata[cb.clientId].rewarded += SafeCast.toUint96(reward);

            emit ClientRewarded(cb.clientId, reward);
        }

[WangSecurity]

As you can see here, we don't actually loop through the entire array of clientIds (as I understand) in L411-L418.

We use maxClientId only to verify that it's not too high and there is indeed such clientId (L416) and we only check clientIds for the eligible proposals (L415). There mapping m has only clientIds for eligible proposals (if I'm wrong correct me).

[eladmallel]

The basic premise of this issue is wrong: we do not loop through all the client IDs in existence. We loop through m.numValues() clientIDs. numValues is a function in our in-memory mapping library:

function numValues(Mapping memory m) internal pure returns (uint256) {
        return m.nextAvailableIndex - 1;
    }

as you can see, its value relates to nextAvailableIndex which we increment whenever a new clientID appears in the rewards calculation function. Moreover, we only loop through this in-memory mapping after a loop where we verify that every single clientID from the input votingClientIds has at least one vote on at least one eligible proposal.

we refund gas only at the end of the function if everything checks out.

sure, anyone can create a clientID, but no one can cause a gas refund by providing client IDs that have not contributed value to Nouns DAO.

I'm curious what am I missing that would make this a valid escalation? thanks!

[cvetanovv]

I agree with the sponsor's comment. Therefore, I'm planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• Dliteofficial: rejected