31078 - [SC - High] withdraw doesnt claim all rewards before burnin...

Submitted on May 12th 2024 at 11:07:38 UTC by @Holterhus for Boost | Alchemix

Report ID: #31078

Report type: Smart Contract

Report severity: High

Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol

Impacts:

• Permanent freezing of unclaimed yield

Description

Brief/Intro

The claim() function in the RewardsDistributor does not always claim all of the pending rewards for a given tokenId. This is because the _claimable() function has a finite for loop that does 50 iterations. However, the withdraw() function in veALCX only calls claim() once before permanently burning the token. This can lead to a permanent freezing of unclaimed yield if the 50 iterations are not sufficient.

Vulnerability Details

The _claimable() function in the RewardsDistributor has the following main loop:

for (uint256 i = 0; i < 50; i++) {
    if (weekCursor >= _lastTokenTime) break;

    if (weekCursor >= userPoint.ts && userEpoch <= maxUserEpoch) {
        userEpoch += 1;
        oldUserPoint = userPoint;
        if (userEpoch > maxUserEpoch) {
            userPoint = IVotingEscrow.Point(0, 0, 0, 0);
        } else {
            userPoint = IVotingEscrow(_ve).getUserPointHistory(_tokenId, userEpoch);
        }
    } else {
        int256 dt = int256(weekCursor - oldUserPoint.ts);
        int256 calc = oldUserPoint.bias - dt * oldUserPoint.slope > int256(0)
            ? oldUserPoint.bias - dt * oldUserPoint.slope
            : int256(0);
        uint256 balanceOf = uint256(calc);
        if (balanceOf == 0 && userEpoch > maxUserEpoch) break;
        if (balanceOf != 0) {
            toDistribute += (balanceOf * tokensPerWeek[weekCursor]) / veSupply[weekCursor];
        }
        weekCursor += WEEK;
    }
}

Notice that this loop only does a maximum of 50 iterations. This is fine on its own, because calling claim() multiple times will progress the rewards claiming in multiples of 50 iterations.

However, consider the following code in the withdraw() function of the veALCX contract:

// Claim any unclaimed ALCX rewards and FLUX
IRewardsDistributor(distributor).claim(_tokenId, false);
IFluxToken(FLUX).claimFlux(_tokenId, IFluxToken(FLUX).getUnclaimedFlux(_tokenId));

// Burn the token
_burn(_tokenId, value);

Since this only calls claim() once, this will only do 50 iterations within the _claimable() calculation, which can be insufficient for claiming all of the user's unclaimed ALCX rewards.

Impact Details

Yield is permanently frozen whenever a user calls withdraw() on a tokenId that needs more than 50 iterations to claim remaining rewards. This can happen if the tokenId has been deposited and left alone for a long time (perhaps with maxLockEnabled == true). It can also happen if the user has checkpointed many times before (notice that weekCursor += WEEK only happens in the else case of the main loop, so claim() may not even progress a single week).

References

See the PoC below.

Proof of Concept

I have created the following test file and added it to the tests/ directory:

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.15;

import "./BaseTest.sol";

contract NotFullClaimBugTest is BaseTest {

    constructor() {
        setupContracts(block.timestamp);
    }

    function testNotFullClaimBug() public {

        vm.startPrank(admin);

        uint256 tokenId1 = createVeAlcx(admin, TOKEN_100K, MAXTIME, true);
        uint256 tokenId2 = createVeAlcx(admin, TOKEN_100K, MAXTIME, true);

        for (uint256 i; i < 100; ++i) {
            vm.warp(newEpoch());
            voter.distribute();
        }

        veALCX.updateUnlockTime(tokenId1, 365 days, false);
        veALCX.updateUnlockTime(tokenId2, 365 days, false);

        for (uint256 i; i < 30; ++i) {
            vm.warp(newEpoch());
            voter.distribute();
        }

        veALCX.startCooldown(tokenId1);
        veALCX.startCooldown(tokenId2);

        vm.warp(block.timestamp + 1 weeks);

        address rewardsToken = distributor.rewardsToken();
        uint256 balBefore;
        uint256 balAfter;


        console.log("claimable 1:", distributor.claimable(tokenId1));
        console.log("claimable 2:", distributor.claimable(tokenId2));

        /*************************************************************
            Amount from withdraw()
        *************************************************************/

        balBefore = IERC20(rewardsToken).balanceOf(admin);
        veALCX.withdraw(tokenId1);
        balAfter = IERC20(rewardsToken).balanceOf(admin);
        console.log("claimed from withdraw():", balAfter - balBefore);

        /*************************************************************
            Amount from claim()
        *************************************************************/

        balBefore = IERC20(rewardsToken).balanceOf(admin);
        for (uint256 i; i < 20; ++i) distributor.claim(tokenId2, false);
        balAfter = IERC20(rewardsToken).balanceOf(admin);
        console.log("claimed from claim():", balAfter - balBefore);

        vm.stopPrank();
    }

}

Running the command forge test -vvv --match-test testNotFullClaimBug --rpc-url $ETH_RPC_URL gives the following result:

[PASS] testNotFullClaimBug() (gas: 109527243)
Logs:
  claimable 1: 85548935342535580780653
  claimable 2: 85548935342535580780653
  claimed from withdraw(): 85548935342535580780653
  claimed from claim(): 113317958706345737195718

which shows that calling withdraw() will permanently freeze unclaimed yield.