hyh - Reward recording can be manipulated to steal from other proposers and voters

[sherlock-admin4]

hyh

medium

Reward recording can be manipulated to steal from other proposers and voters

Summary

On the one hand there is a volatility in realized auction prices, and even no bid auction settlements are recorded (i.e. there can be any number of zero amount records in the settlement history). On the other revenue structure isn't a part of rewards distribution criteria (any positive revenue is deemed correct for reward distribution). This way a malicious caller can run updateRewardsForProposalWritingAndVoting() to allocate the minimized reward to proposals of other participants, up to only one auction revenue allocated to dozens proposals and hundreds of votes.

I.e. the ability to pinpoint the proposal reward to a particular set of auctions by optimizing the updateRewardsForProposalWritingAndVoting() call time allows to depress the rewards for all other actors.

Vulnerability Detail

For example, when after some days with no bids for the auctioned Nouns the first sale came and current proposal beneficiary runs updateRewardsForProposalWritingAndVoting(lastProposalId, ...) with lastProposalId being the previous currently ended proposal just before their own. This way this one Noun sale revenue will be allocated to the proposals up to lastProposalId and the caller will get rid of these proposals so they won't participate in the further profit sharing, so caller's further proposal will receive undiluted revenue share.

Same example can be extended to attacker calling reward distribution on observing that Nouns price goes up to allocate low price period to proposals and votes of others. On the other hand, when price drops the attacker might want to allocate higher prices to the proposal they benefit from, cutting off the subsequent ones. In other words any minimal revenue is now enough for reward recording, which is immutable, can be run by anyone, so it is a race condition setup.

In the same time value of proposals and votes has no link to the current Nouns price, i.e. shouldn't depend on how successful are auctions right now. Ideally the reward to be based on some long term moving average of the auction revenue, not on the late set of auctions.

Impact

Currently there is a possibility for reward revenue manipulation proportionally to the volatility of Nouns price.

The prerequisite is price volatility, but that is quite typical. Revenue share impact is proportional to it, so there is a medium probability of medium impact (typical volatility is frequently available, but the resulting reward misallocation is limited) or low probability of high impact (but sometimes there are substantial price movements, including price impacting Nouns forks, and executing the attack yields more in that periods), i.e. medium severity.

Likelihood: Medium/Low + Impact: Medium/High = Severity: Medium.

Code Snippet

There is no control on how many prices are contributing to the current reward allocation:

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L333-L342

        (uint256 auctionRevenue, uint256 lastAuctionIdForRevenue) = getAuctionRevenue({
            firstNounId: t.firstAuctionIdForRevenue,
            endTimestamp: t.lastProposal.creationTimestamp
        });
        $.nextProposalRewardFirstAuctionId = uint32(lastAuctionIdForRevenue) + 1;

>>      require(auctionRevenue > 0, 'auctionRevenue must be > 0');

>>      t.proposalRewardForPeriod = (auctionRevenue * $.params.proposalRewardBps) / 10_000;
>>      t.votingRewardForPeriod = (auctionRevenue * $.params.votingRewardBps) / 10_000;

Even one successful auction is enough:

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L694-L698

    function sumAuctions(INounsAuctionHouseV2.Settlement[] memory s) internal pure returns (uint256 sum) {
        for (uint256 i = 0; i < s.length; ++i) {
            sum += s[i].amount;
        }
    }

I.e. arbitrary low amount of price data points can form rewards for the arbitrary high number of proposal and votes.

Tool used

Manual Review

Recommendation

Consider adding a control parameter, for example minAuctionsPerProposal, and requiring that the number of non-zero auctions to be greater than $.minAuctionsPerProposal * t.numEligibleProposals, e.g.:

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L333-L336

-       (uint256 auctionRevenue, uint256 lastAuctionIdForRevenue) = getAuctionRevenue({
+       (uint256 auctionRevenue, uint256 lastAuctionIdForRevenue, uint256 successfulAuctions) = getAuctionRevenue({
            firstNounId: t.firstAuctionIdForRevenue,
            endTimestamp: t.lastProposal.creationTimestamp
        });

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L531-L542

    function getAuctionRevenue(
        uint256 firstNounId,
        uint256 endTimestamp
-   ) public view returns (uint256 sumRevenue, uint256 lastAuctionId) {
+   ) public view returns (uint256 sumRevenue, uint256 lastAuctionId, uint256 successfulAuctions) {
        INounsAuctionHouseV2.Settlement[] memory s = auctionHouse.getSettlementsFromIdtoTimestamp(
            firstNounId,
            endTimestamp,
            true
        );
-       sumRevenue = sumAuctions(s);
+       (sumRevenue, successfulAuctions) = sumAuctions(s);
        lastAuctionId = s[s.length - 1].nounId;
    }

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L694-L698

    function sumAuctions(INounsAuctionHouseV2.Settlement[] memory s) internal pure returns (uint256 sum, uint256 numAuctions) {
        for (uint256 i = 0; i < s.length; ++i) {
+           if (s[i].amount > 0) {
                sum += s[i].amount;
+               numAuctions++;
+           }
        }
    }

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L370-L382

        //// Check that distribution is allowed:
        //// 1. At least one eligible proposal.
        //// 2. One of the two conditions must be true:
        //// 2.a. Number of eligible proposals is at least `numProposalsEnoughForReward`.
        //// 2.b. At least `minimumRewardPeriod` seconds have passed since the last update.

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

[eladmallel]

• to date we haven't had high short-term auction volatility; prices have moved but more gradually.
• we haven't had any zero auctions yet, let alone a period of zeros.
• if we do enter a period of zero or near-zero auctions, we have much bigger troubles than potential gaming of auction periods for rewards.

In summary, while this issue is correct in theory, in practice we assign extremely low likelihood and low impact.

We recommend severity be lowered.

[WangSecurity]

Yep, agree with the sponsor above and going to invalidate the finding accordingly.

[0rpse]

The counter argument provided by the sponsor is that zero and high short-term volatile auctions have not happened and even if it did, it would cause greater trouble anyways that gaming these rewards would not matter at that point, which makes sense.

However, as described in #38, there is no need for zero nor high short-term volatile auctions. The race condition is inherently in the function for clients to take advantage of.

In a situation where proposals are written close to each other, after minimumRewardPeriod passes, the client that facilitated writing the proposal with the lower id can make the call to allocate all of the rewards for those proposals' reward period to itself. In this situation even if the bids are proceeding normally rewards are gamed. Now the next proposal will have to wait for future proposals and most likely will receive less rewards than deserved.

[WangSecurity]

But in your report, you also say that the prerequisites are proposals are created one by one and no revenue made in the auction, which to me it looks quite impossible. Therefore, to me it looks low-severity, but would like to hear you elaborate more on the likelihood side of this issue and the prerequisites, maybe I'm missing something and want to know what exactly.

[0rpse]

I never said no revenue made in the auction was a prerequisite, only said that no revenue made between the creation of proposals, this was said to simplify the attack scenario and this is where the impact of this attack would be at highest; one proposal would get all rewards for that period instead of each proposal getting their share. There can still be losses even if there are auction revenue generated between proposals but it would get more and more smaller. Note that one Noun is auctioned off every 24 hours, so you would have a 24 hour gap to have two proposals with no auction revenue generated between their creation.

I realized that this report and #38 are describing completely different scenarios, this report is building on price volatility or no bids, counts it as a prerequisite, and suggests a different fix where as in #38 is describing the occurence of a race condition between closely created proposals. I think you should deduplicate these issues and consider them seperately.

Notice that proposals' creation timestamps limit their rewards, Rewards.sol keeps a pointer to the next auction id to start rewarding from(nextProposalRewardFirstAuctionId), updateRewardsForProposalWritingAndVoting function takes that and the creation timestamp of the lastProposalId, and sums revenue of auctions up until the creation timestamp of the lastProposalId. So proposals are getting rewarded for auctions that took place before their creation, starting from nextProposalRewardFirstAuctionId. Naturally if there is no revenue generated between the creation of proposals or they are created very close to each other(24 hours) gaming of rewards would be at highest. Which is already likely to happen but as i said this is not a prerequisite, we can consider another scenario:

1- 5 auctions take place and revenue is made with all of them 2- Proposal:1 is created 3- 2 days pass, 2 more nouns are auctioned of and Proposal:2 is created 4- minimum reward period has passed, either a occurs or b occured 4.a- Proposal:1 settles after Proposal:2 settles, this blocks callers from making the call with lastProposalId:2 as Proposal:1 is the next proposal to be rewarded and it is not settled yet, so this opens up the race condition where call can be made with lastProposalId:1 to get more rewards as soon as it settles 4.b- Proposals settled in any order before minimumRewardPeriod has passed 5- At this point if call is made with lastProposalId:2, proposal 1 and 2 will share revenue of 7 auctions accordingly, but client who facilitated Proposal:1 can make the call with lastProposalId:1 and get all of the revenue generated from the first 5 auctions, considering bids are around the same price this will lead to more revenue allocation for that client. In this scenario there might be a gap of up to 3 days between the creation of proposals.

In conclusion prerequisites are that: 1- Proposals are created closely, up to 24 hours is the most impactful 2- Proposals are ready to be rewarded at the same time (due to minimum reward period or proposals with lower ids not settling before proposals that come after them)

[WangSecurity]

@0rpse Thanks for such a detailed explanation, I will give my detailed response tomorrow. Thank you very much again, escalating on your behalf, this issue is indeed more interesting than I though initially. Thank you very much.

Escalate

[sherlock-admin2]

@0rpse Thanks for such a detailed explanation, I will give my detailed response tomorrow. Thank you very much again, escalating on your behalf, this issue is indeed more interesting than I though initially. Thank you very much.

Escalate

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]

Here's my reasoning why it should remain invalid. There are 2 requirements to payout the rewards:

1. There have to be at least some number of eligible proposals waiting for the rewards distribution (numProposalsEnoughForReward).
2. Minimum time period to distribute the rewards (minimumRewardsPeriod).

I've aksed them team what the values of them will be, they responded numProposalsEnoughForReward = 15 and minimumRewardsPeriod = 1 month.

I understand that it's not written anywhere in the code/docs and only in discord. But, any watson could ask it during the contest and get the answers. But, for the sake of example, let's cut these in 2, so numProposalsEnoughForReward = 7 and minimumRewardsPeriod = 2 weeks. Firstly, we check if there are 7 eligible proposals (if yes, send the rewards) if not, then check if the minimumRewardsPeriod has passed and if yes distribute the rewards.

Now, the scenario: We create the proposals everyday Mon-Fri (1 day each). Now, we have 5 proposals and let's pretend all of them were eligible and quickly accepted. On Sunday of that week, we cannot distribute the rewards, there are only 5 of 7 eligible rewards and only 1 week of 2 has passed. Another week goes and we create 2 new proposals on mon and tue. They both got accepted on thu and fri let's say. Now, we have 7 eligible proposals and we can distribute the rewards. The problem here is that we cannot distribute the rewards with the proposalId less than 7, cause it will revert due to not enough eligible proposals. Therefore, the rewards will be distribute fairly.

What I'm trying to say here is that the unfair distribution of the rewards will only happen if there are enough eligible proposals and minimumRewardsPeriod has passed, but no one distributes the rewards. I think there is very low probability of this happening and rewards will be distributed as soon as one of there two requirements is met.

But this, of course a great edge case. I'm going to send the message here from the sponsor regarding this issue:

"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, I know that it was written in discord and not in README/code/docs. But what I'm trying to note here that it is a designed behaviour and the team plans to prevent this issue by setting minimumRewardsPeriod and numProposalsEnoughForReward to a very large number, i.e. 1 month and 15 respectively. Therefore, I still think this should remain low.

But, I understand that this report and #38 both address an edge case that can happen, but since the likelihood is very low and as Elad said above impact is low as well, I believe the report should remain low.

Thank you for the watson for highlighting this and #38 and I will accept any decision from the Head of Judging cause I see how this can be judged as medium, but I believe it should remain low.

[WangSecurity]

@0rpse I just wannt thank you for not arguing back and forth, repeating the same arguments all over again and just calmly waiting for the deicision by the head of judging. Thank you very much ❤️

[cvetanovv]

I agree that we can dedupe #15 and #38, but I also agree with the Lead Judge that this issue is Low rather than Medium.

As the Lead Judge mentions, unfair prize distribution only occurs when there are enough eligible proposals and minimumRewardsPeriod has passed, but no one distributes the rewards.

This I think is a rare edge case making the chance Low. We can also consider this a "design decision" as can be seen from the comments of the sponsor.

They can also change minimumRewardsPeriod and numProposalsEnoughForReward high enough so that the auction proceeds are distributed more evenly.

Because of this, I planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• WangSecurity: rejected