thank_you - Temporary DOS of payment delays for proposal rewards

[sherlock-admin3]

thank_you

medium

Temporary DOS of payment delays for proposal rewards

Summary

To redeem a client reward for an eligible proposal, anyone can call Rewards.updateRewardsForProposalWritingAndVoting(). This function allows users to provide how many proposals should be reviewed for reward distribution. In addition, there are checks to determine how frequently proposal rewards can be rewarded. Because of these two factors, it's possible for clients who should receive eligible proposals to be locked out of their funds longer than a week.

Vulnerability Detail

The Rewards.updateRewardsForProposalWritingAndVoting() is a complex function that rewards clients with eligible proposal rewards. There are two aspects that are critical to this bug, the lastProposalId input argument and the eligibility proposal time check.

The first aspect to this bug is that any user can set how many proposals the function should review for rewards. The lastProposalId represents the last proposal to include in the rewards distribution. The updateRewardsForProposalWritingAndVoting() pulls the reviewed proposals via:

NounsDAOTypes.ProposalForRewards[] memory proposals = nounsDAO.proposalDataForRewards(
    t.nextProposalIdToReward,
    lastProposalId, // AUDIT: user input. can be set to any accepted value up to the last proposal that ended
    votingClientIds 
);

This function will retrieve all proposals up to the lastProposalId. These proposals will then be processed to check if any are eligible for rewards. Once the number of eligible proposals is calculated, the function will then run the following checks:

require(t.numEligibleProposals > 0, 'at least one eligible proposal');
if (t.numEligibleProposals < $.params.numProposalsEnoughForReward) {
    require(
        t.lastProposal.creationTimestamp > $.lastProposalRewardsUpdate + $.params.minimumRewardPeriod,
        'not enough time passed'
    );
}
$.lastProposalRewardsUpdate = uint40(t.lastProposal.creationTimestamp);

These checks are:

• Check there is at least one eligible proposal for rewards
• If the # of eligible proposals is below the numProposalsEnoughForReward, check that enough time has passed since the last reward was distributed.

We are most interested in the second check. This check enforces a waiting period for proposal reward distribution. If the minimumRewardPeriod ends up being longer than a week and no more eligible proposals are created, it's possible for the reward clients to be locked out of their funds till the minimum reward period is reached.

Let's take the following example to see how this can be exploited.

Pre-reqs:

• Imagine there are 10 proposals that are eligible for rewards.
• The minimumRewardPeriod is set to 14 days.
• Proposals take on average 7 days to be marked as completed.

Steps: 1) Client A votes in proposals 9 and 10. Both are eligible for rewards. 1) User A calls Rewards.updateRewardsForProposalWritingAndVoting() and sets the lastProposalId to the proposal 8 id. This leaves proposal 9 and 10 not processed. 2) In the next transaction, Client A calls Rewards.updateRewardsForProposalWritingAndVoting(). Because the reward distribution just occurred, the transaction will revert. Client A will then have to wait a week before enough time has passed to be rewarded.

Impact

A client who is eligible for a proposal reward may have their proposal reward funds locked for a week or longer.

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol?plain=1#L376-L383

Tool used

Manual Review

Recommendation

If a proposal reward is eligible to be redeemed, then it should be redeemed when Rewards.updateRewardsForProposalWritingAndVoting() is called. Do not allow users the ability to specify how many eligible proposals should be redeemed. Alternatively, remove the time restriction to reward eligible proposals.

[sherlock-admin2]

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

takarez commented:

seem valid; medium(2)

[eladmallel]

To us this issue has very low severity.

From your criteria for Medium severity, the most relevant one seems to be:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

We don't quite view this as "loss of funds", and even if it is a loss we classify it as small and finite, while your definition calls for "must exceed small finite amount".

We suggest this issue's severity be lowered.

[WangSecurity]

@eladmallel the problem that according to our rules, DOS of funds for more than a week a valid medium, therefore, I need a confirmation from you if they can indeed be locked for longer than a week?

[eladmallel]

It is possible. However it's important to note that by design we're not rushing to pay clients, but rather we want to make sure we accrue enough information and enough valuable interactions. So I continue to disagree with the framing of "loss of funds", but I don't want to belabor this point further; we're ok with medium, just wanted to add more info on how we view this issue.

[WangSecurity]

Yeah, it's great, thank you for providing your point of view. We'll take a bit more to decide on this one.

[WangSecurity]

Going to keep it valid medium, cause funds may indeed be locked for over a week, which is considered DOS.

[eladmallel]

I continue to disagree with this point of view. We never intended to offer guarantees to clients on when they get rewards.

The reward schedule is designed to balance between (1) fair rewards which would rely on maximum information (more proposals, more time) and (2) rewarding clients at a reasonable frequency.

Your analysis seems to assume that it is a requirement that clients be rewarded on a specific schedule, but this was never an explicit objective of our design.

Wdyt?

[WangSecurity]

I wou;dn't say that it's a schedule, but the problem is that the proposal is eligigble and user should be able to receive rewards. Let's take the example from the report. User should get the rewards for proposals 9 and 10, but someone called the Rewards.updateRewardsForProposalWritingAndVoting with the lastProposalId set to 8. Then the user who should be able to receive funds at this moment, cannot do so and has to wait for more than a week. Correct or am I missing something?

[WangSecurity]

But one other point we can say that we expect users to call Rewards.updateRewardsForProposalWritingAndVoting after every eligible proposals to get the rewards, therefore, as soon as we meet the next eligible proposal, the time lock should've already reset, correct?

[WangSecurity]

Going through the report and rules again, this might be invalid due to user experience and design improvement. What I mean here is that users don't lose their initial funds, but the rewards that they've got for a work, moreover, as we can see there's a specific time when users will be able to receive their rewards.

Thus, I'm going to consult with the Head Of Judging on this one @eladmallel

[WangSecurity]

I've consulted with Head of Judging and this is design decision, timelock for rewards is constrained, therefore, this report falls into category of "User experience and design improvement issues" which are invalid under Sherlock's rules.

[mrthankyou]

Escalate

Hi,

Thank you for taking the time to review this ticket and escalation. I believe this issue should be a medium.

The crux of this bug report is focused on the practical implications of a predictable and reliable reward system for clients. While I acknowledge that reward distributions can occur within a finite and known timeframe, the variability and permissionless nature of the reward system (anyone can call this function at any time) introduces a level of unpredictability that undermines a fair and efficient reward system. While in theory the reward distribution can happen in a single cycle, this is predicated on an assumption of ideal user behavior when calling updateRewardsForProposalWritingAndVoting(), which can not be guaranteed nor enforced.

A system that can be easily manipulated or disrupted, intentionally or accidentally, by user input to the extent that it significantly delays reward distribution longer than 1 week constitutes a DOS. This is because it enables a scenario where users, through no fault of their own, are unable to access funds they are entitled to in a timely manner, based on the variable actions of others. I do not believe this falls in line with a design decision but instead represents an inherent issue with the reward system itself.

In light of these considerations, it is more reasonable to argue that users would reasonably expect the rewards distribution mechanism to be robust enough to distribute all eligible proposal rewards promptly and predictably, irrespective of individual user actions. The current design, by allowing for significant delays based on user-dependent parameters, fails to meet this expectation and thus represents a genuine DOS concern rather than a mere user experience or design improvement issue.

[sherlock-admin2]

Escalate

Hi,

Thank you for taking the time to review this ticket and escalation. I believe this issue should be a medium.

The crux of this bug report is focused on the practical implications of a predictable and reliable reward system for clients. While I acknowledge that reward distributions can occur within a finite and known timeframe, the variability and permissionless nature of the reward system (anyone can call this function at any time) introduces a level of unpredictability that undermines a fair and efficient reward system. While in theory the reward distribution can happen in a single cycle, this is predicated on an assumption of ideal user behavior when calling updateRewardsForProposalWritingAndVoting(), which can not be guaranteed nor enforced.

A system that can be easily manipulated or disrupted, intentionally or accidentally, by user input to the extent that it significantly delays reward distribution longer than 1 week constitutes a DOS. This is because it enables a scenario where users, through no fault of their own, are unable to access funds they are entitled to in a timely manner, based on the variable actions of others. I do not believe this falls in line with a design decision but instead represents an inherent issue with the reward system itself.

In light of these considerations, it is more reasonable to argue that users would reasonably expect the rewards distribution mechanism to be robust enough to distribute all eligible proposal rewards promptly and predictably, irrespective of individual user actions. The current design, by allowing for significant delays based on user-dependent parameters, fails to meet this expectation and thus represents a genuine DOS concern rather than a mere user experience or design improvement issue.

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]

Thank you for raising this escalation, but I still believe it should remain low. As I understand there are to requirements to pay out the rewards the number of eligible proposlas have to be higher than the set threshold (numProposalsEnoughForReward) OR the wait time since last reward distribution has passed (minimumRewardsPeriod).

And in edge case, the described in the issue scenario may indeed occur. But here's the answer of sponsors:

"I agree with the watson that #15 and #38 can be deduped. regarding #38, I agree with the general analysis. the way the reward contract is currently designed, there's slight room for clients to affect the reward distribution based on which lastProviderId they provide. our plan to mitigate this risk is to set minimumRewardsPeriod and numProposalsEnoughForReward high enough so that auction revenue is spread more equaly, but besides that, we accept it as part of the "luck" that this system has, which also includes: how much auction revenue was generated in a period, how many proposals were created in a period."

Yes, it's about issue 38, but the root cause is similar, tho the impact is different. Sponsors are aware of such flow and plan to fix it by setting these two thresholds high enough.

I know it's a message from discord and it's not written anywhere in README/code and this still can happen in the ende case, adding it here for an additional context for the head of judging.

I think it should remain low since it's a designed and known by users behaviour and the time that the rewards are locked inside the protocol is finite and known, therefore, I think it's not a DOS per se and the report should remain low.

[WangSecurity]

Additional message, I think if escalation on #15 will be accepted, then we can set this report as a duplicate of it. As I understand, the root cause is the same: users are able to give any lastProposalId even if it's not the last one actually. Yes, I see there are different impacts, in this one the rewards cannot be withdrawn for extended period of time and in #15 users lose part of the reward.

@mrthankyou what do you think?

[mrthankyou]

@WangSecurity,

I haven't reviewed #15 in detail but from my cursory look it appears to contain the same root cause so I think that is a fair assumption to consider it a duplicate. Thanks!

[cvetanovv]

I agree that this is a "design decision". From the sponsor's comment, it is clear that this is by design.

There is indeed a rule in the documentation that DOS can be Medium if is more than a week, but it also says: "Additional constraints related to the issue may decrease its severity accordingly."

I also agree that we can duplicate it with #15 because of the same root.

For now, I plan to reject the escalation, but if #15 is valid, we will duplicate them.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• mrthankyou: rejected