Uneven distribution of rewards in updateRewardsForProposalWritingAndVoting
Summary
Users can call updateRewardsForProposalWritingAndVoting with lower proposal ids to get more rewards allocated to those proposals.
Vulnerability Detail
Rewards::updateRewardsForProposalWritingAndVoting calculates rewards accumulated from auctions until lastProposalId's creation timestamp, starting from the lastProposalId's creation timestamp that was used in the previous invocation. It requires that at least 1 proposal is eligible for rewards and at least 2 weeks have passed or at least 5 proposals are eligible for rewards in this time period.
In an ideal scenario this function would be called with the last proposal that is eligible for rewards and rewards would be distributed accordingly but users have the ability to call updateRewardsForProposalWritingAndVoting with lower proposal ids and allocate more rewards than intended in certain scenarios.
Consider a simple scenario where two proposals have been made and during a 2 week period where 200 ETH revenue was gathered. Proposal id:111 with Client id:1 and Proposal id:112 with Client id:2, only considering rewards for writing the proposals and with proposalRewardBps set to 100 bips, 2 ETH would be split among clientIds 1 and 2 if the call were made with lastProposalId:112. Now if a user makes the call with lastProposalId:111, all of the 2 ETH for writing proposals will end up going to clientId 1. This is considering there were no auction revenue generated between the creation of proposals. At this point Proposal id:112 will have to wait until future proposals are made and all conditions are met to get rewards as the nextProposalRewardFirstAuctionId points to an auction newer than Proposal id:112.
Also note that it is possible for proposals with lower ids to not settle before later proposals due to objection periods, this increases the likelihood of this scenario occurring. Even though you can set one settled proposal to be enough to distribute rewards, due to an unsettled past proposal going into objection period this attack would still be possible.
Impact
This vulnerability is most impactful when proposals are created approximately next to each other or when no auction revenue is generated between them. Considering many different things have to align to make this attack have major effect and its impact on reward distribution, impact is medium.
Consider querying last settled proposal and use its id in the function instead of using lastProposalId parameter or apply access control to updateRewardsForProposalWritingAndVoting with a trusted role.
WangSecurity
commented
4 months ago
0rpse
medium
Uneven distribution of rewards in updateRewardsForProposalWritingAndVoting
Summary
Users can call updateRewardsForProposalWritingAndVoting with lower proposal ids to get more rewards allocated to those proposals.
Vulnerability Detail
Rewards::updateRewardsForProposalWritingAndVoting calculates rewards accumulated from auctions until lastProposalId's creation timestamp, starting from the lastProposalId's creation timestamp that was used in the previous invocation. It requires that at least 1 proposal is eligible for rewards and at least 2 weeks have passed or at least 5 proposals are eligible for rewards in this time period.
In an ideal scenario this function would be called with the last proposal that is eligible for rewards and rewards would be distributed accordingly but users have the ability to call updateRewardsForProposalWritingAndVoting with lower proposal ids and allocate more rewards than intended in certain scenarios.
Consider a simple scenario where two proposals have been made and during a 2 week period where 200 ETH revenue was gathered. Proposal id:111 with Client id:1 and Proposal id:112 with Client id:2, only considering rewards for writing the proposals and with proposalRewardBps set to 100 bips, 2 ETH would be split among clientIds 1 and 2 if the call were made with lastProposalId:112. Now if a user makes the call with lastProposalId:111, all of the 2 ETH for writing proposals will end up going to clientId 1. This is considering there were no auction revenue generated between the creation of proposals. At this point Proposal id:112 will have to wait until future proposals are made and all conditions are met to get rewards as the nextProposalRewardFirstAuctionId points to an auction newer than Proposal id:112.
Also note that it is possible for proposals with lower ids to not settle before later proposals due to objection periods, this increases the likelihood of this scenario occurring. Even though you can set one settled proposal to be enough to distribute rewards, due to an unsettled past proposal going into objection period this attack would still be possible.
Impact
This vulnerability is most impactful when proposals are created approximately next to each other or when no auction revenue is generated between them. Considering many different things have to align to make this attack have major effect and its impact on reward distribution, impact is medium.
Code Snippet
https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L307C1-L449C6
Tool used
Manual Review
Recommendation
Consider querying last settled proposal and use its id in the function instead of using lastProposalId parameter or apply access control to updateRewardsForProposalWritingAndVoting with a trusted role.
Duplicate of #15