31495 - [SC - Critical] Users cannot claim rewards from RevenueHandler ...

Submitted on May 20th 2024 at 13:55:07 UTC by @OxAnmol for Boost | Alchemix

Report ID: #31495

Report type: Smart Contract

Report severity: Critical

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

Impacts:

• Permanent freezing of unclaimed yield

Description

Brief/Intro

If a user extends their lock time to the maximum amount using votingEscrow:updateLockTime and sets maxLockEnabled to true, the RevenueHandler:claim function will fail when the user attempts to claim their rewards.

Vulnerability Details

The RevenueHandler:claim function allows users to earn rewards every epoch for maintaining a veALCX position. Users can call this function at any time after locking BPT. The function calculates and distributes all the epoch rewards since the last claim to the user.

   function claim(
        uint256 tokenId,
        address token,
        address alchemist,
        uint256 amount,
        address recipient
    ) external override {
        require(IVotingEscrow(veALCX).isApprovedOrOwner(msg.sender, tokenId), "Not approved or owner");

        uint256 amountBurned = 0;
->>     uint256 amountClaimable = _claimable(tokenId, token);
...SNIP...
  }

The claim function internally calls the RevenueHandler:_claimable function, which calculates the user's rewards for each epoch.

function _claimable(uint256 tokenId, address token) internal view returns (uint256) {
        uint256 totalClaimable = 0;
        uint256 lastClaimEpochTimestamp = userCheckpoints[tokenId][token].lastClaimEpoch;
        if (lastClaimEpochTimestamp == 0) {
            uint256 lastUserEpoch = IVotingEscrow(veALCX).userFirstEpoch(tokenId);
            lastClaimEpochTimestamp = (IVotingEscrow(veALCX).pointHistoryTimestamp(lastUserEpoch) / WEEK) * WEEK - WEEK;
        }
        for (
            uint256 epochTimestamp = lastClaimEpochTimestamp + WEEK;
            epochTimestamp <= currentEpoch;
            epochTimestamp += WEEK
        ) {
            uint256 epochTotalVeSupply = IVotingEscrow(veALCX).totalSupplyAtT(epochTimestamp);
            if (epochTotalVeSupply == 0) continue;
            uint256 epochRevenue = epochRevenues[epochTimestamp][token];
L322::          uint256 epochUserVeBalance = IVotingEscrow(veALCX).balanceOfTokenAt(tokenId, epochTimestamp);
            totalClaimable += (epochRevenue * epochUserVeBalance) / epochTotalVeSupply;
        }
        return totalClaimable + userCheckpoints[tokenId][token].unclaimed;
    }

In line L322, the VotingEscrow:balanceOfTokenAt function checks for the user's underlying voting power/BPT. If the user has opted for the maximum lock, the balanceOfTokenAt function will return the maximum balance.

  function _balanceOfTokenAt(uint256 _tokenId, uint256 _time) internal view returns (uint256) {
       ...SNIP...
            int256 biasCalculation = locked[_tokenId].maxLockEnabled
                ? int256(0)
                : lastPoint.slope * (int256(_time) - int256(lastPoint.ts));
           ...SNIP...
        }
    }

Let's say a user initially sets a lock end time of 90 days. After 2 epochs (4 weeks), they decide to extend the lock time to the maximum (1 year) by calling updateLockTime with maxLockEnabled set to true.

If they then try to claim the rewards for the first 2 epochs from the revenue handler, the _claimable function will behave as if they always had the maximum lock enabled. It will not account for the initial 90-day lock covering the first 2 epochs. The attempt to distribute rewards will fail because the contract will not have enough funds to distribute.

Impact Details

Users will lose rewards. This is a loss of unclaimed yield for user which is considered as high impact.

References

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/RevenueHandler.sol#L186

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/RevenueHandler.sol#L322

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L714

Proof of Concept

Paste this test in RevenueHandler.t.sol

We can see what are the steps taken here to reach the revert state.

function testChangeLockTimeAfterTwoEpochAndClaimAll() public {
        //Lock BPT for 3 months
        uint256 tokenId = createVeAlcx(admin, TOKEN_1, 3 * 30 days, false);
        uint256 revAmt = 1000e18;
        _accrueRevenueAndJumpOneEpoch(revAmt); // Deposit 1000 DAI

        hevm.startPrank(admin);
        veALCX.updateUnlockTime(tokenId, MAXTIME, true); // Enable max lock so we get max rewards from now onwards
        hevm.stopPrank();

        _accrueRevenueAndJumpOneEpoch(revAmt); // Deposit 1000 DAI

        hevm.startPrank(admin);
        expectError("Not enough revenue to claim");
        revenueHandler.claim(tokenId, alusd, address(alusdAlchemist), revenueHandler.claimable(tokenId, alusd), admin);

        hevm.stopPrank();
    }

Output

Ran 1 test for src/test/RevenueHandler.t.sol:RevenueHandlerTest
[FAIL. Reason: revert: Not enough revenue to claim] testChangeLockTimeAfterTwoEpochAndClaimAll() (gas: 3287974)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 145.80s (113.26s CPU time)

Ran 1 test suite in 148.30s (145.80s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/test/RevenueHandler.t.sol:RevenueHandlerTest
[FAIL. Reason: revert: Not enough revenue to claim] testChangeLockTimeAfterTwoEpochAndClaimAll() (gas: 3287974)