ChinmayF - Voters will lose all bribe rewards forever if they do not claim their rewards after the last bribing period

[sherlock-admin4]

ChinmayF

High

Voters will lose all bribe rewards forever if they do not claim their rewards after the last bribing period

Summary

The claim() function in BribeRewarder is used to claim the rewards associated with a tokenID across all bribe periods that have ended : it iterates over all the voting periods starting from the bribe rewarder's _startVotingPeriod upto the last period that ended according to the Voter.sol contract, and collects and sends rewards to the NFT's owner.

The issue is that if the voter(ie. tokenID owner who earned bribe rewards for one or more bribe periods) does not claim his rewards by the lastVotingPeriod + 2, then all his unclaimed rewards for all periods will be lost forever.

Vulnerability Detail

Lets walk through an example to better understand the issue. Even though the issue occurs in all other cases, we are assuming that the voting period has just started to make it easy to understand.

1. The first voting period is about to start in the next block. The bribe provider deploys a bribe rewarder and registers it for a pool X for voting periods 1 to 5. ie. the startVotingPeriod in the BribeRewarder.sol = 1 and lastVotingPeriod = 5.
2. The first voting period starts in voter.sol. Users start voting for pool X and the BribeRewarder keeps getting notified and storing the rewards data in respective rewarder data structure (see here and here)
3. All 5 voting periods have ended. User voted in all voting periods and got a reward stored for all 5 bribe periods in the BribeRewarder contract. Now when he claims via claim(), he can get all his rewards.
4. Assume that the 6th voting period has ended. Still if the user calls claim(), he will get back all his rewards. His 6th period rewards will be empty but it does not revert.
5. Assume that the 7th voting period has ended. Now if the user calls claim(), his call will revert and from now on, he will never be able to claim any of his unclaimed rewards for all periods.

The reason of this issue is this :

    function claim(uint256 tokenId) external override {
        uint256 endPeriod = IVoter(_caller).getLatestFinishedPeriod();
        uint256 totalAmount;

        for (uint256 i = _startVotingPeriod; i <= endPeriod; ++i) {
            totalAmount += _modify(i, tokenId, 0, true);
        }

        emit Claimed(tokenId, _pool(), totalAmount);
    }

The claim function is the only way to claim a user's rewards after he has voted. This iterates over the voting periods starting from the _startVotingPeriod (which is equal to 1 in our example).

This loop's last iteration is the latest voting period that might have ended on the voter contract (regardless of if it was a declared as a bribe period in our own BribeRewarder since voting periods will be a forever going thing and we only want to reward upto a limited set of periods, defined by BribeRewarder:_lastVotingPeriod).

Lets see the Voter.getLatestFinishedPeriod() function :

    function getLatestFinishedPeriod() external view override returns (uint256) {
        if (_votingEnded()) {
            return _currentVotingPeriodId;
        }
        if (_currentVotingPeriodId == 0) revert IVoter__NoFinishedPeriod();
        return _currentVotingPeriodId - 1;
    }

Now if suppose the 6th voting period is running, it will return 5 as finished. if 7th is running, it will return 6, and if 8th period has started, it will return 7.

Now back to claim => _modify. It fetches the rewards data for that period in the _rewards array, which has (lastID - startID) + 2 elements. (see here). In our case, this array will consist of 6 elements (startId = 1 and lastID = 5).

Now when we see how it is fetching the reward data using periodID, it is using the value returned by _indexByPeriodId() as the index of the array.

    function _indexByPeriodId(uint256 periodId) internal view returns (uint256) {
        return periodId - _startVotingPeriod;
    }

So for a periodID = 7, this will return index 6.

Now back to the example above. When the 7th voting period has ended, getLatestFinishedPeriod() will return 7 and the claim function will try to iterate over all the periods that have ended. When the iteration comes to this last period = 7, the _modify function will try to read the array element at index 6 (again we can see this clearly here)

But now it will revert with index out of bounds, because the array only has 6 elements from index 0 to index 5, so trying to access index element 6 in _rewards array will now always revert.

This means that after 2 periods have passed after the last bribing period, no user can ever claim any of their rewards even if they voted for all the periods.

Impact

No user will be able to claim any of their unclaimed rewards for any periods from the BribeRewarder, after this time. The rewards will be lost forever, and a side effect of this is that these rewards will remain stuck in the BribeRewarder. But the main impact is the complete loss of rewards of many users.

High severity because users should always get their deserved rewards, and many users could lose rewards this way at the same time. The damage to the individual user depends on how many periods they didn't claim for and how much amount they used for voting, which could be a very large amount.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L159

Tool used

Manual Review

Recommendation

The solution is simple : in the claim function, limit the endPeriod used in the loop by the _lastVotingPeriod of a particular BribeRewarder.

    uint256 endPeriod = IVoter(_caller).getLatestFinishedPeriod();
    if (endPeriod > _lastVotingPeriod) endPeriod = _lastVotingPeriod;

[chinmay-farkya]

If these problems are solved separately, the integrity of the system can be compromised

I absolutely disagree that dupes are decided based on arriving at a common broader solution for two problems that are separate in the logic. That doesn't matter in issue duplication/separation on sherlock imo.

[Honour-d-dev]

@0xSmartContract set 1 of this escalation are actually invalid as mentioned in my previous comment.
Voters are intentionally allowed only one extra period to claim their rewards.
After which rewards are locked to be claimed by the briber.

If this is not the case then there will be conflict as to when the briber is allowed to sweep the contract for unclaimed rewards. hence the need for the deadline.

[WangSecurity]

I've hidden one comment cause it seemed to be repeating the same point and not adding value.

About this issue, I agree that duplicates are:
#145, #501, #495, #591.

#591 - says Claiming rewards for any period after _lastVotingPeriod + 2 will be impossible and not after _lastVotingPeriod + 2 as said in the escalation comment, so I assume it's correct, excuse me if wrong, please correct.
#371 and #161 agree to be incorrect.

But, @Honour-d-dev comment is very interesting, is there any evidence that this behaviour is not intended?

Additional :
#257 and #216 are duplicates of #52
#110 is not a duplicate and is actually invalid as it does not correctly identify the root cause and instead talks about out-of-gas due to loop which is impossible given the block gas limit of iota is 1B units as seen here https://explorer.evm.iota.org/block/424441.
#543 is definitely not a duplicate and actually invalid because the amount has been already scaled using “Constants.ACC_PRECISION_BITS” in the mentioned code.

Completely agree with the part quoted above and plan to apply these changes.

Additionally, I agree that the second set mentioned in the escalation comment is a different issue. The root causes, fixes and vulnerability paths are different. Even though the impact is similar, it's not a ground to duplicate all of them into 1 issue. The best will be #172, the duplicates are:
#94, #121, #135, #139, #172, #180, #183, #255, #263, #278, #291, #298, #315, #317, #374, #408, #417, #470, #473, #486, #586, #652

Waiting for an answer about lost rewards after 2 epochs before making the decision.

[Slavchew]

The @Honour-d-dev comment comes from just one piece of information in the docs, here's the full text:

https://docs.magicsea.finance/protocol/magic/magic-lum-voting#bribes

As far as I understand, this is just to say that the bribe rewards for the current period can be claimed after it ends.

"Voters can claim the rewards until the next epoch is ended."

The statement above - rewards not claimed on the period after bribe ends should not be claimable by voters - #164 (comment) is not true as it's never states in the docs that bribe rewards can only be claimed 1 period after the end and no more, it states that Voters can claim the rewards until the next epoch is ended, not the end one.

[Honour-d-dev]

From my understanding Until means "not after" , no?

The statement in the docs:
"Voters can claim the rewards until the next epoch is ended"

Is saying , "if the last bribe epoch for the BribeRewarder is epoch-3, then voters have until the end of epoch-4 (i.e. the next epoch) to claim rewards"

This makes even more sense ,considering the issue with sweep functionality so there needs to be a point where rewards not yet claimed can be considered as forfeit and the briber is allowed to sweep them.

@Slavchew I don't see how the full context of the docs disproves this

[chinmay-farkya]

Hey @WangSecurity this response is regarding the comment by honour-d-dev here : #164 (comment)

I believe that the docs, especially the relevant section, is outdated.

Here is the evidence for the docs being outdated.

1. The issue pashap9990 - Voters can claim their reward immediately in the end of epoch #346 says that "rewards can be claimed immediately after the epoch ends" which is non-impactful issue and thus invalid. The sponsor added a Won't fix label to that because the intention was not what the docs stated (the same section of docs relevant here), what makes sense is to allow reward claiming after the epoch, and that is what the code says.

Voters can claim the rewards until the next epoch is ended

If this statement in the docs was to be true then the claim function in BribeRewarder would have allowed only the one current finished period in the claim logic. See here.

The claim function forces to loop over all the finished periods(and not just the latest one) which means that the code was intended to allow staker to claim his rewards for all periods that have ended, and not just the one that ended latest.

So the code was really "meant to allow stakers to claim their rewards for all finished periods" otherwise the claim function would be having just the "getLatestFinishedPeriod()" in the claiming logic.

About #591 : yes, my bad it is a duplicate.
But see that #145 is not a duplicate (this one says it will revert after lastVotingPeriod + 1 which is not true). I misnumbered the issue in the original escalation.

TLDR, #164 shall remain a separate valid high severity issue with duplicates : #501, #495, #591

[Honour-d-dev]

@chinmay-farkya

1. I do not agree that sponsor adding won't fix tag is evidence that the docs is outdated , same docs was provided for the audit as relevant protocol resources, no?. pashap9990 - Voters can claim their reward immediately in the end of epoch #346 just has no impact.

If this statement in the docs was to be true then the claim function in BribeRewarder would have allowed only the one current finished period in the claim logic. See here.

The claim function forces to loop over all the finished periods(and not just the latest one) which means that the code was intended to allow staker to claim his rewards for all periods that have ended, and not just the one that ended latest.

I do not understand the argument here, if you're questioning why the bribe rewards for other periods (not the last one) are still claimable until the end of the last period+1 , then the answer is similar to #346, it's not impactful if the voter can still claim rewards for earlier periods as long as the BribeRewarder is still active.

It is Impactful ,however, if the voter can still claim rewards after the BribeRewarder is no longer active as this will interfere with the sweeping of unclaimed or undistributed tokens. If the voter is given unlimited time to claim rewards then at what point will it be safe to send back unclaimed rewards to the briber as stated by the docs, without the voters that has not claimed yet loosing their rewards?

[chinmay-farkya]

I do not understand the argument here, if you're questioning why the bribe rewards for other periods (not the last one) are still claimable until the end of the last period+1 , then the answer is similar to #346, it's not impactful if the voter can still claim rewards for earlier periods as long as the BribeRewarder is still active.

I'm saying that if we were to believe what the docs say, then it should not have been encoded into the claim function to loop over a set of epochs. if "rewards could be claimed only until the next epoch ended" : If this would have been true and intended then the claim function would have the _modify call for only one epoch ie. "getLatestFinishedPeriod()" : ie. after epoch 1 ends, claim should call _modify for only 1st epoch and the loop would not have been in place at all. And that is not the case, so the docs do not portray what the code is designed for.

The current claim function logic means that the rewards claim has to be allowed for all finished periods uptil now, and not just the "only one latest finished period" {that is what the docs say}. Code takes precedence over the docs.

So in short, the claim has been designed to allow claiming all periods at once as per code (and code > docs), and rest you can read the original bug description.

It is Impactful ,however, if the voter can still claim rewards after the BribeRewarder is no longer active as this will interfere with the sweeping of unclaimed or undistributed tokens.

In this context also, the docs are actually outdated/not updated acc to what the code is designed for currently. There was no sweep function in BribeRewarder at all. Sweeping out rewards earned by users also does not make sense, they should be able to claim them if they have been accrued. The bribeRewarder can only be used once, so the unclaimed funds can sit without any problems.

The current recommendation around "briberewarder should have a sweep function" pointed out in other bug is for "undistributed rewards" and not for "unclaimed rewards", because sweeping out funds which were not even accrued to anybody due to no votes in an apoch(for ex.) makes sense, but sweeping out rewards earned by users and still sitting in the briberewarder as unclaimed does not make sense.

If the voter is given unlimited time to claim rewards then at what point will it be safe to send back unclaimed rewards to the briber

They should never be sent back, only the undistributed rewards shall be. That can be easily done by recording all accrued rewards in a separate variable lets say "Rewardsdistributed" and only sweeping out balance - rewardsdistributed back to the protocol. Also see thats how it is done in the BaseRewarder here : maintaining a reserve variable.

[Slavchew]

@WangSecurity Instead of trying to descramble the words in the documentation, can't the sponsor just come and write if they think it's okay for users to lose bribe rewards if they haven't taken them max 2 periods after that? Everyone will agree that it is not acceptable, because the rewards are not sent and everyone has to claim them and not everyone would have the opportunity to claim them during this period.

I think that will settle the whole escalation.

[Honour-d-dev]

I'm saying that if we were to believe what the docs say, then it should not have been encoded into the claim function to loop over a set of epochs. if "rewards could be claimed only until the next epoch ended" : If this would have been true and intended then the claim function would have the _modify call for only one epoch ie. "getLatestFinishedPeriod()" : ie. after epoch 1 ends, claim should call _modify for only 1st epoch and the loop would not have been in place at all. And that is not the case, so the docs do not portray what the code is designed for.

So in short, the claim has been designed to allow claiming all periods at once as per code (and code > docs), and rest you can read the original bug description.

Looping over all epochs to calculate rewards is a design decision (as opposed to having the voter specify the exact epoch they voted/have rewards in) it does not contradict or invalidate the docs.

In this context also, the docs are actually outdated/not updated acc to what the code is designed for currently. There was no sweep function in BribeRewarder at all. Sweeping out rewards earned by users also does not make sense, they should be able to claim them if they have been accrued. The bribeRewarder can only be used once, so the unclaimed funds can sit without any problems.

Again, docs is not outdated , BribeRewarder can be used multiple times actually ( "The bribeRewarder can only be used once" is false) and sweeping unclaimed rewards makes sense as it is in the protocol docs(which makes it public information) so voters would be aware of it and know that they have a 14-day window to claim rewards

They should never be sent back, only the undistributed rewards shall be. That can be easily done by recording all accrued rewards in a separate variable lets say "Rewardsdistributed" and only sweeping out balance - rewardsdistributed back to the protocol. Also see thats how it is done in the BaseRewarder here : maintaining a reserve variable.

It's not possible to record all the rewards accrued by users in this implementation of the masterChef algo because rewards still accrue(or is distributed) even when nobody votes(as distribution is time-based i.e. emissionsPerSecond, to encourage voters to vote as early as possible) and a voter's specific reward is dependent on the time they voted and the total votes in that epoch. The _reserve variable in BaseRewarder is actually the total rewards available in the contract and the _totalUnclaimedRewards works a differently there because there's no concept of epochs that reset the total supply to 0, just a start and end time. Hence why Briberewarder is not BaseRewarder

[chinmay-farkya]

Looping over all epochs to calculate rewards is a design decision (as opposed to having the voter specify the exact epoch they voted/have rewards in) it does not contradict or invalidate the docs.

That is completely false. That does invalidate the docs. The docs say that only the latest period shall be allowed to be claimed, but the code loops over all periods including the latest one which means it is meant to allow claiming for all past periods, how are these two equivalent statements ? They are clearly not. So the code needs to be believed and the code has a bug ie. #164

BribeRewarder can be used multiple times actually ( "The bribeRewarder can only be used once" is false)

Again that is completely false. here is a surprise for you : the briberewarder can only be initialized once : see https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L227

It's not possible to record all the rewards accrued by users in this implementation of the masterChef algo because rewards still accrue(or is distributed) even when nobody votes

Again completely false the rewards would get stuck if no one voted thats what the other set of issues in another bug is. Please read #172

lets leave baserewarder out of the current discussion an focus on the current bug.

[Honour-d-dev]

The docs say that only the latest period shall be allowed to be claimed,

The docs does not say or imply this please. This is a false statement

Again that is completely false. here is a surprise for you : the briberewarder can only be initialized once

In original comment #164 (comment) you said used not initialized,

The bribeRewarder can only be used once, so the unclaimed funds can sit without any problems.

i interpreted as used to claim rewards (which can be done multiple times)

Again completely false the rewards would get stuck if no one voted thats what the other set of issues in another bug is.

I did not say rewards don't get stuck, i said rewards are still accrued if no one votes. For example if the BribeRewarder is to distribute 100usd to voters for that epoch, if the first voter votes after 7 days(half-way into the epoch) then they can only earn a maximum of 50usd assuming no one else votes on the bribed pool. Which means rewards still accrue regardless of votes or not (but to nobody if there's no votes). i can provide POC of this if you're still in doubt.

[WangSecurity]

Instead of trying to descramble the words in the documentation, can't the sponsor just come and write if they think it's okay for users to lose bribe rewards if they haven't taken them max 2 periods after that? Everyone will agree that it is not acceptable, because the rewards are not sent and everyone has to claim them and not everyone would have the opportunity to claim them during this period

I would like to do that, but the sponsor is OOS for 2 weeks, so we cannot get this information.

But I believe we have sufficient arguments to settle the discussion. The docs say:

Voters can claim the rewards until the next epoch is ended.

In this case, if the last epoch is 5 (arbitrary number), then we shouldn't be able to claim rewards after the 6th epoch has ended. But, as I understand and as this report shows, the rewards can be claimed after the 6th epoch has ended, but cannot be claimed after the 7th epoch. If I'm correct here, then we have contextual evidence that the docs are outdated.

Then the question about not having the sweep function for the briber to get back the rewards. In that sense, it's still a valid bug, since not all rewards can be distributed, so they should be swept by the briber.

Hope that answers the questions and I believe it's a fair decision here. About #145, agree it shouldn't be a duplicate. Hence, my decision is to accept the escalation, apply the changes expressed here, except #145 being invalid. Both families will have High severity.

[iamnmt]

@WangSecurity @chinmay-farkya

I believe #145 stating the correct condition in which the bug occurs

when Voter#getLatestFinishedPeriod() is greater than BribeRewarder#_lastVotingPeriod + 1

To elaborate on this, Voter#getLatestFinishedPeriod() is greater than BribeRewarder#_lastVotingPeriod + 1 if and only if (_currentVotingPeriodId is equal to BribeRewarder#_lastVotingPeriod + 2 and current voting period is ended) or _currentVotingPeriodId is greater than BribeRewarder#_lastVotingPeriod + 2

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/Voter.sol#L471

I do not see anything wrong with this condition. Please correct me if I am wrong.

This condition is the same as "after 2 periods have passed after the last bribing period".

But see that #145 is not a duplicate (this one says it will revert after lastVotingPeriod + 1 which is not true)

Nowhere in #145 mention that it will revert after lastVotingPeriod + 1.

[WangSecurity]

Users can not claim from BribeRewarder when Voter#getLatestFinishedPeriod() is greater than BribeRewarder#_lastVotingPeriod + 1, which will result in DoS on BribeRewarder#claim and loss of funds

Here's the impact from #145. It says the rewards will be unable to be claimed after _lastVotingPeriod + 1. The rewards will be unable to be claimed because the function will revert. But, that's not fully correct, it's still possible to claim the rewards after _lastVotingPeriod + 1 during _lastVotingPeriod + 2. That's why I believe the report is not sufficient because rewards become unable to be claimed after _lastVotingPeriod + 2 when the report says +1. Hence, I believe it's incorrect.

The decision remains as expressed here

[iamnmt]

@WangSecurity @chinmay-farkya

it's still possible to claim the rewards after _lastVotingPeriod + 1 during _lastVotingPeriod + 2

You are correct.

It says the rewards will be unable to be claimed after _lastVotingPeriod + 1

No, it is not. #145 intentionally mentions the function Voter#getLatestFinishedPeriod()

Voter#getLatestFinishedPeriod() is greater than BribeRewarder#_lastVotingPeriod + 1

If the current voting period is _lastVotingPeriod + 2 and the voting period is not over, then Voter#getLatestFinishedPeriod() will return _lastVotingPeriod + 1. If the current voting period is _lastVotingPeriod + 2 and the voting period is over, then Voter#getLatestFinishedPeriod() will return _lastVotingPeriod + 2. That is why I believe this condition is the same as "after 2 periods have passed after the last bribing period".

Another way to look at this condition is just looking at BribeRewarder#claim in isolation. There are _lastVotingPeriod - _startVotingPeriod + 2 elements in the _rewards array, meaning the array can be indexed up to _lastVotingPeriod + 1. Indexing an element greater than _lastVotingPeriod + 1 will cause revert. When BribeRewarder#claim is called, the Voter#getLatestFinishedPeriod()th element will be indexed. Thus, if Voter#getLatestFinishedPeriod() is greater than _lastVotingPeriod + 1, then the function call will revert.

Because of the reasons above, I believe the condition in #145 is correct. I kindly request you to review your decision again.

[chinmay-farkya]

Yeah I relooked at it, and now I see you are right.

#145 is a correct duplicate of this. "getLatestFinishedPeriod > lastVotingPeriod + 1" essentially means the same thing as "last finsihed period = lastVotingPeriod + 2"

Sorry got confused earlier with the wording.

[WangSecurity]

Excuse me, @iamnmt is correct and I didn't notice this. Planning to accept the escalation and apply the changes as here making 2 families with High severity.

[PeterSRWeb3]

@WangSecurity @0xSmartContract This issue should be a duplicate #409 of the second family of issues, because it's pointing out the same problem and it's proposing the same solution. Thanks in advance!

[WangSecurity]

As I understand #409 report is incorrect. The example given is that the claiming of rewards will revert when the latest finished voting period is 5 if the rewards are distributed from 2 to 4 periods. But, as I understand this is incorrect and the rewards can still be claimed until 6th voting period is finished, based on the discussion above and this report.

Simply put, the #409 report says the claiming of rewards is impossible after _latestVotingPeriod + 1, when in fact it's still possible and reverts when _latestVotingPeriod + 2. Correct me if my understanding of #409 is incorrect.

The decision remains as expressed here

[PeterSRWeb3]

@WangSecurity Thanks sir for taking a look! This is due to the fact that I've ran the test using the first reccomendation. Of course this is my mistake, but the report is valid for me, because it's pointing the same issue and it's proposing the same solution. Please take a look again sir. I totally understand you and I will do my best, to not make this type of mistakes again.

[WangSecurity]

Still, the report is incorrect, hence, my decision remains the same.

[PeterSRWeb3]

Okay, sir, my bad in this situation. Thanks for the effort

[WangSecurity]

Result:
High
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.