sherlock-admin2 commented 3 months ago

Audinarey

High

Users will not be able to claim their complete rewards

Summary

The RewardsDistributor::claim() function does not always claim all of the pending rewards for a given tokenId. This is because the _claim() function has a finite for loop that does 50 iterations. This can lead to a permanent freezing of unclaimed yield if a user locks for 52 weeks and only claims rewards after their lock ends.

Vulnerability Detail

Users have the liberty of locking their tokens for up to 52 weeks. But before they withdraw, they first call RewardsDistributor::claim() and then VotingEscrow::withdraw(...) which burns the token entirely

File: RewardsDistributorV2.sol

File: RewardsDistributorV2.sol
64:     function _checkpoint_token() internal {
...
74: 
75:         for (uint i = 0; i < 20; i++) {
76:             next_week = this_week + WEEK; // the begining of next week or end of this week
77:             if (block.timestamp < next_week) { // loop can start and end here or it can end here after starting from else
78:                 if (since_last == 0 && block.timestamp == t) {
79:    @>>>              tokens_per_week[this_week] += to_distribute;
80:                 } else {
81:     @>>>             tokens_per_week[this_week] += to_distribute * (block.timestamp - t) / since_last;
82:                 }
83:                 break;
84:             } else { // block.timestamp is >= next_week firstly when this loop is encountered and next_week != t
85:                 if (since_last == 0 && next_week == t) { 
86:    @>>>              tokens_per_week[this_week] += to_distribute;
87:                 } else {
88:    @>>>              tokens_per_week[this_week] += to_distribute * (next_week - t) / since_last;
89:                 }
90:             } 
....
95:     }

169:     function _claim(uint _tokenId, address ve, uint _last_token_time) internal returns (uint) {
170:         uint user_epoch = 0;
SNIP
...
194: 
195:   @>>   for (uint i = 0; i < 50; i++) {
196:             if (week_cursor >= _last_token_time) break; // he has been claiming up to date
197: 
198:             if (week_cursor >= user_point.ts && user_epoch <= max_user_epoch) {
199:                 user_epoch += 1;
SNIP
...
206:             } else {
207:                 int128 dt = int128(int256(week_cursor - old_user_point.ts));
208:                 uint balance_of = Math.max(uint(int256(old_user_point.bias - dt * old_user_point.slope)), 0);
209:                 if (balance_of == 0 && user_epoch > max_user_epoch) break; 
210:                 if (balance_of != 0) {
211:    @>>              to_distribute += balance_of * tokens_per_week[week_cursor] / ve_supply[week_cursor];
212:                 } 
213:                 week_cursor += WEEK;
214:             } 
215:         }
SNIP
.....
223:         return to_distribute;
224:     }

However, using a 50 loop iteration within the _claim() function for calculation of claimable rewards, can be insufficient for claiming all of the user's unclaimed rewards. This is because the tokens_per_week is updated in the _checkpoint_token() function every week when the keeper distributes emissions, hence the user will get rewards due for 50 weeks instead of 52 weeks.

Alice lock her token for 52 weeks without touching it or claiming rewards
At the beginning of the 54th weeks she has accrued rewards and she calls claim(...) which calls _claim(...) internally
the tokens_per_week[week_cursor] has been updated during distribution up until at least the 52nd week
unfortunately, since the iteration is going to return rewards earned by Alice from only 50 of the 52 tokens_per_week[week_cursor] distributed, Alice would loose rewards from the last 2 weeks of her lock period
Alice calls withdraw with her token and she forgoes her entitled rewards from the last 2 weeks

This also affects the _claimable(...) and claimable(...) functions respectively as they could be used by external protocols which makes them susceptible to the same vulnerability

Impact

This can lead to loss of reward for users with the unclaimed portion likely stuck in the contract

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/RewardsDistributor.sol#L196-L216

Tool used

Manual Review

Recommendation

Consider

modifying the RewardsDistributor::_claim() function to ensure that the rewards can be accrued beyond 50 weeks but not above the the lock period of their token.
putting the claim function only inside withdraw(...) and the iteration should be done only for the amount of time the user locked their token and not beyond because that would open another path for theft.

Also there should be a mechanism for tracking the users who constantly call claim whether or not they lock for max period or not.

Duplicate of #330

Audinarey commented 2 months ago

Escalate

This issue was wrongly dupped

sherlock-admin3 commented 2 months ago

Escalate

This issue was wrongly dupped

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.

Arabadzhiew commented 2 months ago

Escalate

On his behalf

sherlock-admin3 commented 2 months ago

Escalate

On his behalf

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.

Audinarey commented 2 months ago

@nevillehuang following our discussion on discord, below is a POC to prove that a user does get all rewards if they don't claim regularly

CONTEST the 53rd week is when the 52 week epochs reward are earned completely

normal guy and victim both create lock with the same amount
normal guy claims regularly until the 53rd week
victim calls claim only at the end of the 53rd week
normal guy claims more reward than victim

Add the test case below to the RewardsDistributorV2.t.sol file and run forge test --mt testUserReceivesLessRewardsDistribution -vvv

    function testUserReceivesLessRewardsDistribution() public {

        initializeVotingEscrow();
        DAI.transfer(address(distributor), 10*TOKEN_1);
        // DAI.transfer(address(distributor), TOKEN_100K);

        //Victim and normal users create a lock
        flowDaiPair.approve(address(escrow), 2*TOKEN_1);
        uint victimId = escrow.create_lock(TOKEN_1,52 weeks);
        uint normalId = escrow.create_lock(TOKEN_1,52 weeks);

        uint256 normal_user_reward;
        uint256 victim_user_reward;

        //1st Epoch Finished
        _elapseOneWeek();
        voter.distribute();

        for (uint i; i < 52; i++) { // go another 51 more weeks
            //ith Epoch Finished
            _elapseOneWeek();
            // user claims for the previous (i-1)th epoch
            normal_user_reward += distributor.claimable(normalId);
            // distributionn for the ith epoch
            voter.distribute();

        }
        // 53rd week ends here
        _elapseOneWeek();
        // _elapseOneWeek();

        // normal guy claims LAST reward at the end of the 53rd week
        normal_user_reward += distributor.claimable(normalId);

        //Now victim claims ALL rewards at the end of the 53rd week but recieves less rewards
        victim_user_reward = distributor.claimable(victimId);

        // normal user claims more
        assertTrue(normal_user_reward > victim_user_reward);

    }

nevillehuang commented 2 months ago

Afaik, if the victim invoked another claim, the rewards will line up accordingly. Could you add that to the PoC and test it out?

Audinarey commented 2 months ago

@nevillehuang

I get your point, the problem is not whether the rewards align or not with an extra claim, but that the an unsuspecting user will withdraw immediately after claiming once thinking they have claimed their entitled rewards and forfeit the remaining rewards.

As I mentioned in my report,

...At the beginning of the 54th weeks she has accrued rewards and she calls claim(...) which calls _claim(...) internally... ...Alice calls withdraw with her token and she forgoes her entitled rewards from the last 2 weeks

But I added an extra claim and yet ...

    function testUserReceivesLessRewardsDistribution() public {

        initializeVotingEscrow();
        DAI.transfer(address(distributor), 10*TOKEN_1);
        // DAI.transfer(address(distributor), TOKEN_100K);

        //Victim and normal users create a lock
        flowDaiPair.approve(address(escrow), 2*TOKEN_1);
        uint victimId = escrow.create_lock(TOKEN_1,52 weeks);
        uint normalId = escrow.create_lock(TOKEN_1,52 weeks);

        uint256 normal_user_reward;
        uint256 victim_user_reward;

        //1st Epoch Finished
        _elapseOneWeek();
        voter.distribute();

        for (uint i; i < 52; i++) { // go another 51 more weeks
            //ith Epoch Finished
            _elapseOneWeek();
            // user claims for the previous (i-1)th epoch
            normal_user_reward += distributor.claimable(normalId);
            // distributionn for the ith epoch
            voter.distribute();

        }
        // 53rd week ends here
        _elapseOneWeek();
        // _elapseOneWeek();

        // normal guy claims LAST reward at the end of the 53rd week
        normal_user_reward += distributor.claimable(normalId);

        //Now victim claims ALL rewards at the end of the 53rd week but recieves less rewards
        victim_user_reward = distributor.claimable(victimId);
        victim_user_reward = distributor.claimable(victimId); // added extra claim

        // normal user claims more
        assertTrue(normal_user_reward > victim_user_reward);

    }

cvetanovv commented 2 months ago

I don't see a problem here. The user will call the function again before withdrawal. The protocol has limited the loops to 50 for no out-of-gas error.

If someone locks their tokens for 60 weeks, that doesn't mean we should raise the loop to 60.

In addition, if, for example, the rewards are claimed on the 5th week, then the scenario in the issue cannot happen.

Planning to reject the escalation and leave the issue as is.

Audinarey commented 2 months ago

@cvetanovv

In addition, if, for example, the rewards are claimed on the 5th week, then the scenario in the issue cannot happen.

Thanks for you input, in fact my report mentions that the this will be a problem for a user who locks without touching it and waits until the end of their lock period before claiming and then withdrawing.

For context,

Alice locks for 52(+) weeks
waits until the end of her lock period expires, assuming she has accrued all entitled rewards
claims once and then withdraws loosing the yield earned above week 50

...The user will call the function again before withdrawal.

The POC shows that calling the function a second time does not guarantee Alice received all her rewards (normal guy claims regularly and Alice is the victim)

I believe the likelihood of this scenario (also mentioned in the report) is high and this leads to a loss of yield for users.

cvetanovv commented 2 months ago

It's not a issue, it's a design decision, so it doesn't become a big loop and has OOG. There will be no problem if the user claims his rewards at least once or calls the function again before withdrawal(). If it doesn't, we can assume it's a user error.

Because of this, this issue is of low severity, and my decision to reject the escalation remains.

Audinarey commented 2 months ago

@cvetanovv

There is an even bigger problem here that elaborates the impact of the loss reported, logging the output shows that the user looses more than 2 weeks of rewards if he claims a few times during the epoch when his lock is active and again after his lock ends.

Just before you make your final judgement, I'll like @dawiddrzala to also take a look considering the amount of loss that the user incurs here. The previous POCs and the one provided below shows a definite loss of funds for the user.

...If it doesn't, we can assume it's a user error...

There is no user error here as it wasn't stated in the audit README or docs that users need to claim their rewards at the end of every epoch. Hence I this is not a case of user error.

Thanks for your time.

    function testUserReceivesLessRewardsDistributionfin() public {

        initializeVotingEscrow();
        DAI.transfer(address(distributor), 10*TOKEN_1);
        // DAI.transfer(address(distributor), TOKEN_100K);

        //Victim and normal users create a lock
        flowDaiPair.approve(address(escrow), 2*TOKEN_1);
        uint victimId = escrow.create_lock(TOKEN_1,52 weeks);
        uint normalId = escrow.create_lock(TOKEN_1,52 weeks);

        uint256 normal_user_reward;
        uint256 victim_user_reward;

        //1st Epoch Finished
        _elapseOneWeek();
        voter.distribute();

        // _elapseOneWeek();
        _elapseOneWeek();
            // user claims for the previous 1st epoch
        normal_user_reward += distributor.claimable(normalId);
        victim_user_reward = distributor.claimable(victimId);

        for (uint i; i < 50; i++) { // go another 51 more weeks
            //ith Epoch Finished
            _elapseOneWeek();
            // user claims for the previous (i-1)th epoch
            normal_user_reward += distributor.claimable(normalId);
            // distributionn for the ith epoch
            voter.distribute();

        }
        // 53rd week ends here
        _elapseOneWeek();
        // _elapseOneWeek();

        // normal guy claims LAST reward at the end of the 53rd week
        normal_user_reward += distributor.claimable(normalId);

        //Now victim claims ALL rewards at the end of the 53rd week but recieves less rewards
        victim_user_reward = distributor.claimable(victimId);
        victim_user_reward = distributor.claimable(victimId); // added extra claim

        console.log("victim users reward after 53 : %d", victim_user_reward);
        console.log("normal users reward after 53 : %d", normal_user_reward);
        console.log("number of weeks lost : %d", normal_user_reward / victim_user_reward);

        // normal user claims more
        assertTrue(normal_user_reward > victim_user_reward);

    }

OUTPUT

Logs:
  victim users reward after 53 : 3306872839165
  normal users reward after 53 : 165343641958250
  number of weeks lost : 50

0xklapouchy commented 2 months ago

@Audinarey @nevillehuang @cvetanovv

What is this PoC? Are you adding value from the claimable() view function into normal_user_reward in a for loop? Lol.

Just call claim() twice for both users, then recheck the balance. The rewards should be identical.

Please don't manipulate with an invalid PoC.

0xklapouchy commented 2 months ago

@Audinarey

You need to understand how the claim() function works, and that the limit of 50 applies to user_points, not weeks. A user can have even 100 user_points for one week out of the 52 they locked (if they perform additional operations like extra deposits). Therefore, to claim for that one week, they will need to call claim() twice.

The user is covered by the UX and the claimable() function, which should report that they constantly have something to claim, even though claimable() is limited to 50 user_points and does not report the entire possible claimable amount. However, it will always indicate that there is something to claim if there is anything available.

Therefore, it is the user’s responsibility to call the claim() function when they see that they have something to claim, as reported by the claimable() function.

Audinarey commented 2 months ago

What is this PoC? Are you adding value from the claimable() view function into normal_user_reward in a for loop? Lol.

Just call claim() twice for both users, then recheck the balance. The rewards should be identical.

Please don't manipulate with an invalid PoC.

@0xklapouchy

Please mind your language, I wasn't trying to manipulate anything. I used the claimable() function following the existing test in the RewardsDistributorV4.t.sol file, however I just changed that to claim(...) considering your comment and yes the rewards align irrespective of when the user claims.

Thanks for the clarity you provided here

I agree with @cvetanovv that there is no issue here.

WangSecurity commented 2 months ago

Result: Invalid Duplicate of #330

sherlock-admin2 commented 2 months ago

Escalations have been resolved successfully!

Escalation status:

Arabadzhiew: rejected

sherlock-audit / 2024-06-velocimeter-judging

Audinarey - Users will not be able to claim their complete rewards #499

Users will not be able to claim their complete rewards

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation