jasonxiale - `Rewards.getVotingClientIds` doesn't consistent with `Rewards.updateRewardsForProposalWritingAndVoting`

[sherlock-admin3]

jasonxiale

medium

Rewards.getVotingClientIds doesn't consistent with Rewards.updateRewardsForProposalWritingAndVoting

Summary

Rewards.getVotingClientIds is used to return the clientIds that are needed to be passed as a parameter to Rewards.updateRewardsForProposalWritingAndVoting, without this helper function, it will be impossible for non-tech user to get the correct clientIds, In current implementation, there is a special case the function doesn't return the correct clientIds, which will causes Rewards.updateRewardsForProposalWritingAndVoting revert

Vulnerability Detail

In Rewards.getVotingClientIds, the function fetch all the clientIds from $.nextProposalIdToReward to lastProposalId in Rewards.sol#L502-L506 And then filter all the clientIds those have been used in Rewards.sol#L508-L519

The issue is that when those clientIds passed into Rewards.updateRewardsForProposalWritingAndVoting, the function will delete some proposal in Rewards.sol#L358-L361.

If a clientIdX is used only once for a proposalX(and the clientIdX is not used for the rest proposals), and the proposalX is deleted in Rewards.sol#L358-L361 because of proposals[i].forVotes < (proposals[i].totalSupply * proposalEligibilityQuorumBps_, in such case, the function will revert in Rewards.sol#L438-L438 because didClientIdHaveVotes[clientIdX] is 0

Impact

In current implementation, there is a special case the function doesn't return the correct clientIds, which will causes Rewards.updateRewardsForProposalWritingAndVoting revert

Code Snippet

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

Tool used

Manual Review

Recommendation

diff --git a/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol b/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol
index ee2ea88..48309a6 100644
--- a/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol
+++ b/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol
@@ -507,6 +507,9 @@ contract Rewards is

         uint32[] memory sumVotes = new uint32[](numClientIds);
         for (uint256 i; i < proposals.length; ++i) {
+            if (proposals[i].forVotes < (proposals[i].totalSupply * proposalEligibilityQuorumBps_) / 10_000) {
+                continue;
+            }
             for (uint256 j; j < numClientIds; ++j) {
                 sumVotes[j] += proposals[i].voteData[j].votes;
             }

[sherlock-admin4]

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 #29

[WangSecurity]

Invalidation reasons:

1. getVotingClientIds is a view helper function, therefore, it's not mandatory to use it and users can get the clientId array themselves manually. Hence, it's invalida under Sherlock's rules about return value of view functions.
2. Even if we use getVotingClientIds and then get a revert, we can extract incorrect clientId of the array, run it again successfully.

Therefore, it's a low severity therefore, invalid.

[eladmallel]

We are working on a fix that will better filter out proposals.

[etherSky111]

Escalate

This should be at least medium.

Even though the issue lies within the view function, its impact is significant. Users heavily depend on this function to obtain parameters for the updateRewardsForProposalWritingAndVoting function, which is known to consume a considerable amount of gas. Consequently, if the getVotingClientIds function returns incorrect results, users may incur substantial losses due to massive gas fees. Additionally, manually identifying the revert reason is challenging, as I highlighted in my issue #29.

[sherlock-admin2]

Escalate

This should be at least medium.

Even though the issue lies within the view function, its impact is significant. Users heavily depend on this function to obtain parameters for the updateRewardsForProposalWritingAndVoting function, which is known to consume a considerable amount of gas. Consequently, if the getVotingClientIds function returns incorrect results, users may incur substantial losses due to massive gas fees. Additionally, manually identifying the revert reason is challenging, as I highlighted in my issue #29.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[WangSecurity]

As I've said earlier, this is helper function, users can use it, but not obligated to do so. Moreover, as it's said the correct clientId can be manually constructed. Therefore, I believe reasons that it's challenging to construct such array or it's gas intensive functions are not sufficient enough for a medium.

[crazy4linux]

Escalate

I think this should be medium

1. getVotingClientIds is a view helper function, therefore, it's not mandatory to use it and users can get the clientId array themselves manually. Hence, it's invalida under Sherlock's rules about return value of view functions.

As comments in Rewards.sol#L491, Returns the clientIds that are needed to be passed as a parameter to updateRewardsForProposalWritingAndVoting, getVotingClientIds is not just a view function, it's also a helper for updateRewardsForProposalWritingAndVoting

And quoting from sherlock judging rule

16.Incorrect values in View functions are by default considered low.
Exception: In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

2. Even if we use getVotingClientIds and then get a revert, we can extract incorrect clientId of the array, run it again successfully.

Yes, if updateRewardsForProposalWritingAndVoting get reverted, we can extract incorrect clientId of the array, run it again. But it's not the case for a non-tech user. For a non-tech user, he/she relies on getVotingClientIds's return value to call updateRewardsForProposalWritingAndVoting.

Another issue is that: suppose non-tech users calls updateRewardsForProposalWritingAndVoting at timestamp T1, he/she uses P1 as lastProposalId, and if success, all related clientId will gets X1 as rewardPerVote. But his call revert, and then sometime later, a tech user will call updateRewardsForProposalWritingAndVoting again, but this time he might not use the same lastProposalId as P1, he might use some other value instead. In such case, the rewardPerVote might not be equal to previous one, so the rewards for one clientId might be changed because of different parameters.

Just because it can be fixed by tech user, I think it should be medium. If it can't be fixed by a tech user, I think it should be high

[sherlock-admin2]

Escalate

I think this should be medium

1. getVotingClientIds is a view helper function, therefore, it's not mandatory to use it and users can get the clientId array themselves manually. Hence, it's invalida under Sherlock's rules about return value of view functions.

As comments in Rewards.sol#L491, Returns the clientIds that are needed to be passed as a parameter to updateRewardsForProposalWritingAndVoting, getVotingClientIds is not just a view function, it's also a helper for updateRewardsForProposalWritingAndVoting

And quoting from sherlock judging rule

16.Incorrect values in View functions are by default considered low.
Exception: In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

2. Even if we use getVotingClientIds and then get a revert, we can extract incorrect clientId of the array, run it again successfully.

Yes, if updateRewardsForProposalWritingAndVoting get reverted, we can extract incorrect clientId of the array, run it again. But it's not the case for a non-tech user. For a non-tech user, he/she relies on getVotingClientIds's return value to call updateRewardsForProposalWritingAndVoting.

Another issue is that: suppose non-tech users calls updateRewardsForProposalWritingAndVoting at timestamp T1, he/she uses P1 as lastProposalId, and if success, all related clientId will gets X1 as rewardPerVote. But his call revert, and then sometime later, a tech user will call updateRewardsForProposalWritingAndVoting again, but this time he might not use the same lastProposalId as P1, he might use some other value instead. In such case, the rewardPerVote might not be equal to previous one, so the rewards for one clientId might be changed because of different parameters.

Just because it can be fixed by tech user, I think it should be medium. If it can't be fixed by a tech user, I think it should be high

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]

Yes, the rule says, that Exception: In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

This function is not a part of larger function. It's a helper function. Users are not obligated to use it and they can get the clientId array. I don't think that argument "it's challenging for non-tech users" is not enough for it being a medium.

I believe the rule about view functions correctly invalidates this issue, because it's NOT a part of a large function, it's a helper for that large function, which can be successfully completed without running getVotingClientIds

[etherSky111]

Despite it is a helper function, it yields incorrect results. I believe that it is difficult to determine the correct input for the updateRewardsForProposalWritingAndVoting function, particularly when a significant amount of time has passed since the last update. As indicated in the comment below for getVotingClientIds function, most users will depend on this function for accurate input even though it is not mandatory to use.

/**
     * @notice Returns the clientIds that are needed to be passed as a parameter to updateRewardsForProposalWritingAndVoting
     * @dev This is not meant to be called onchain because it may be very gas intensive.
     */

However, when users call updateRewardsForProposalWritingAndVoting function with wrong parameters , it results in a revert and consumes a large amount of gas.

I'm new to Sherlock, so I'm not familiar with its rules. Nevertheless, if all view functions are considered as low, what purpose does it serve to include them in the scope? We could just skip view functions.

I appreciate your kind review

[WangSecurity]

Not all view functions are considered low. There are lots of view functions (in every contest) that is called during the larger non-view function. Therefore, the only view functions that are out of scope are the functions that are used separately and all other functions can be successfully called without this view functions.

This is our case, we can successfully call updateRewardsForProposalWritingAndVoting without calling getVotingCliendIds upfront, therefore, under Sherlock's rule, this specific case is considered low.

[crazy4linux]

Not all view functions are considered low. There are lots of view functions (in every contest) that is called during the larger non-view function. Therefore, the only view functions that are out of scope are the functions that are used separately and all other functions can be successfully called without this view functions.

According to your explanation, it'll be Exception: any view functions called by a larger function...., in such case, I don't think it can be called Exception, because it's obvious that the entire code flow has flaw.

[etherSky111]

I am 100% sure that getVotingCliendIds function is important view function. Even though using this function is not mandatory, almost users will rely on it. It is impossible to mention all possible scenarios in the rule. I suggest we could update rules regarding this kind of issues.

[crazy4linux]

Yes, the rule says, that Exception: In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

This function is not a part of larger function. It's a helper function.

Quoting from Hierarchy of truth:

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

I think your declaration about the function can be classified as protocol answer, and so the code comments in Rewards.sol#L491-L492 should have more priority than your declaration :-)

Users are not obligated to use it and they can get the clientId array. I don't think that argument "it's challenging for non-tech users" is not enough for it being a medium.

I think we can agree that non-tech will be reverted, and the tech user can call updateRewardsForProposalWritingAndVoting by filter out the incorrect clientId. In such case

1. for tech user, he has the advantage over the non-tech user, because non-tech user can't claim the rewards. While tech-user, he can use different lastProposalId for function updateRewardsForProposalWritingAndVoting to make his favorite clientId more profit. Suppose there are N...N+9 ten proposals pending for rewards, and proposalN has the issue. For the non-tech users, they can't claim reward. But for tech-user, he can use N+2 , or N+5 to call function updateRewardsForProposalWritingAndVoting. Because by using different lastProposalId, there will be different proposals in Rewards.sol#L323-L326, and different proposals will result different auctionRevenue(because different auction has different biding price) in Rewards.sol#L333, then different auctionRevenue will cause different t.votingRewardForPeriod in Rewards.sol#L342, and different t.votingRewardForPeriod will generate different t.rewardPerVote in Rewards.sol#L387, and the rewards is calculated as votes * t.rewardPerVote in Rewards.sol#L427 So by using different lastProposalId, he can manipulates the rewards after the proposal is closed.
2. For non-tech user, because his call get reverted, I think this can be classified as DOS, and sherlock also has rules for DOS which I think can be used here

   Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue: The issue causes locking of funds for users for more than a week. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly. Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

[WangSecurity]

I disagree it can be classified as a DOS. At maximum it's a DOS of one block, cause as I've said users can extract incorrect values manually and call the function successfully. Moreover, others as you call "tech" users can call the function and all the "non-tech" will be able to get the rewards.

Moreover, as I've said earlier, I don't think that the argument "it's challenging for non-tech users to call updateRewardsForProposalWritingAndVoting without calling getVotingClientIds is vaible for medium.

I don't understand how my argument can be classified as a "protocol answer". My judging is made according to rule 16 regarding view functions and confirmed by the comment inside the code.

[etherSky111]

It's obvious that determining the correct input parameters of updateRewardsForProposalWritingAndVoting function will be challenging for both of tech and non-tech users. There is a convenient getVotingClientIds function for this purpose. Do you think tech users will not trust this function in any case and calculate always the input parameters manually? I believe almost users(nearly 100%) will use this function. Once it is reverted, the tech users may try to extract incorrect values and won't believe this function anymore. Do you believe this is acceptable scenario for protocol? This kind of vulnerability should be prevented. I don't think that normal users love the protocol which provides an incorrect functionality in the UI and require complex tech operations to execute this functionality manually.

@WangSecurity , I respect your decision regarding the Sherlock rule, however this is an exceptional case in the technical viewpoint and need comments from the head of judge.

Thanks in advance.

[WangSecurity]

I agree that it may be an exceptional case, even tho as of now I still insist on it to remain low, and as I understand, there are no more new comments from either of the side, therefore, we can leave it for the head of judging to decide. Thank you for bringing up this issue, it may actually be more interesting than it seems.

[crazy4linux]

I disagree it can be classified as a DOS. At maximum it's a DOS of one block, cause as I've said users can extract incorrect values manually and call the function successfully. Moreover, others as you call "tech" users can call the function and all the "non-tech" will be able to get the rewards.

I am not saying this issue should be classified as DOS, what I meant is that the side affect for non-tech user is DOS. Please show me the code or documents that can support your point of view about At maximum it's a DOS of one block. For my understanding if the issue is not fixed by tech user, the function will always revert. And there is no guarantees that the tech user will call the function every block.

Moreover, as I've said earlier, I don't think that the argument "it's challenging for non-tech users to call updateRewardsForProposalWritingAndVoting without calling getVotingClientIds is vaible for medium.

As I said in previous comments, there will be affects from both sides(tech user and non-tech user). For tech user, he can chose different lastProposalId to make more profit than expected, if a user can manipulate the parameter to make more profile, I think it's a valid issue.

I don't understand how my argument can be classified as a "protocol answer". My judging is made according to rule 16 regarding view functions and confirmed by the comment inside the code.

It's obvious that we have different understanding about rule 16, that's OK. The issue is that if getVotingClientIds can't return correct result, the protocol will create a reward mechanism that looks fair, but it's not. For all clientIds, they think they can get rewards fairly, but in fact, tech user can manipulate the rewards.

[WangSecurity]

This issue is a maximum DOS of one block cause if the user calls it with incorrect parameters, then they can extract incorrect clientIds and run the function successfully.

My main argument is mainly that the protocol can function as it should be without getVotingClientIds function ever called. I know it's hard to get the correct clientId. And the argument that web3 users are not technical enough to make an actually correct clientId array is not a viable argument for this issue to be valid medium.

I see you mention that users can get profit in edge case manipulating lastProposalId, but it's nowhere mentioned in this report, there is a different escalation on issue #15 regarding it. But it's irrelevant for this report, since I don't see how it mentions getting profit from manipulating lastProposalId.

These are my arguments and they have not changed since the start of the escalation. At this point we just argue back and forth saying the same words.

If there are no new arguments from your side, we can leave the decision to the head of judging.

[crazy4linux]

I think I'll the the issue to the judge now. @WangSecurity thanks for your time :-D

[dmitriia]

The issue is fixed in PR-839 as now the filtration logic is moved to proposalDataForRewards() and the calls to it are the same in both functions.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/nounsDAO/nouns-monorepo/pull/839

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.

[cvetanovv]

Yes, the rule says, that Exception: In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

This function is not a part of larger function. It's a helper function. Users are not obligated to use it and they can get the clientId array. I don't think that argument "it's challenging for non-tech users" is not enough for it being a medium.

I believe the rule about view functions correctly invalidates this issue, because it's NOT a part of a large function, it's a helper for that large function, which can be successfully completed without running getVotingClientIds

I think this comment from the Lead Judge best describes why this problem is Low severity.

By default, every view function is Low severity. It should be called from another function and there should be a vulnerability because of it and then it can be Medium/High. You can check the Sherlock documentation.

I'm 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:

• crazy4linux: rejected