VotiumStrategy withdrawal queue fails to consider available unlocked tokens causing different issues in the withdraw process

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L54-L103

Vulnerability details

Summary

Withdrawals in VotiumStrategy are executed in queue since CVX tokens are potentially locked in Convex. However, the implementation fails to consider the case where unlocked assets are already enough to cover the withdrawal, leading to different issues.

Impact

VotiumStrategy withdrawals are executed in queue since the underlying CVX tokens may be locked in the Convex platform. Depositors must request a withdrawal and wait in queue until the epoch associated with their withdrawal is reached in order to exit their position. The core of this logic is present in the function requestWithdraw():

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L54-L103

054:     function requestWithdraw(
055:         uint256 _amount
056:     ) public override returns (uint256 withdrawId) {
057:         latestWithdrawId++;
058:         uint256 _priceInCvx = cvxPerVotium();
059: 
060:         _burn(msg.sender, _amount);
061: 
062:         uint256 currentEpoch = ILockedCvx(VLCVX_ADDRESS).findEpochId(
063:             block.timestamp
064:         );
065:         (
066:             ,
067:             uint256 unlockable,
068:             ,
069:             ILockedCvx.LockedBalance[] memory lockedBalances
070:         ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
071:         uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
072:         cvxUnlockObligations += cvxAmount;
073: 
074:         uint256 totalLockedBalancePlusUnlockable = unlockable +
075:             IERC20(CVX_ADDRESS).balanceOf(address(this));
076: 
077:         for (uint256 i = 0; i < lockedBalances.length; i++) {
078:             totalLockedBalancePlusUnlockable += lockedBalances[i].amount;
079:             // we found the epoch at which there is enough to unlock this position
080:             if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations) {
081:                 (, uint32 currentEpochStartingTime) = ILockedCvx(VLCVX_ADDRESS)
082:                     .epochs(currentEpoch);
083:                 uint256 timeDifference = lockedBalances[i].unlockTime -
084:                     currentEpochStartingTime;
085:                 uint256 epochOffset = timeDifference /
086:                     ILockedCvx(VLCVX_ADDRESS).rewardsDuration();
087:                 uint256 withdrawEpoch = currentEpoch + epochOffset;
088:                 withdrawIdToWithdrawRequestInfo[
089:                     latestWithdrawId
090:                 ] = WithdrawRequestInfo({
091:                     cvxOwed: cvxAmount,
092:                     withdrawn: false,
093:                     epoch: withdrawEpoch,
094:                     owner: msg.sender
095:                 });
096: 
097:                 emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
098:                 return latestWithdrawId;
099:             }
100:         }
101:         // should never get here
102:         revert InvalidLockedAmount();
103:     }

The implementation first considers available tokens that should be ready to be withdrawn. Line 74-75 sets totalLockedBalancePlusUnlockable to the amount of unlockable tokens (expired tokens in Convex that can be withdrawn) plus any available CVX balance in the contract.

However, the implementation fails to consider that this available balance may be already enough to cover the withdrawal, and proceeds to search within the locked balances by epoch. This will lead to different issues:

• A user that requests a withdrawal of an amount such that there is enough available balance to cover for it, will still need to wait until the end of the next locked cycle. The implementation will start searching the locked balances array and stop at the first match, and set the withdrawal epoch as the end of the matched period. Even if there are enough tokens to cover for the withdrawal, the user is forced into an unnecessary delay.
• If there are none locked balances, meaning everything is already in an unlockable state, the implementation will cause a denial of service. The for loop in line 77 won't be executed and the execution will be reverted due to the revert in line 102.

Proof of Concept

Let's illustrate the denial of service case. We assume all deposits in VotiumStrategy were done before the 16 weeks period, which means all vlCVX should be in an unlocked state.

1. User calls requestWithdraw(amount).
2. The implementation fetches current position from vlCVX contract using ILockedCvx.lockedBalances(). This will return the entire position as unlockable and an empty array for lockedBalances.
3. The function sets totalLockedBalancePlusUnlockable as unlockable + IERC20(CVX_ADDRESS).balanceOf(address(this)). This should be enough to cover the requested amount by the user.
4. The implementation continues to search through the lockedBalances, but since this array is empty the for loop is never executed.
5. The function reaches the end and is reverted with a InvalidLockedAmount() error.

Recommendation

Before searching through the lockedBalances array, check if there available unlocked tokens are enough to cover the withdrawal. If so, the withdrawal can be set for the current epoch. This will fix the unnecessary delay and the potential denial of service.

    function requestWithdraw(
        uint256 _amount
    ) public override returns (uint256 withdrawId) {
        latestWithdrawId++;
        uint256 _priceInCvx = cvxPerVotium();

        _burn(msg.sender, _amount);

        uint256 currentEpoch = ILockedCvx(VLCVX_ADDRESS).findEpochId(
            block.timestamp
        );
        (
            ,
            uint256 unlockable,
            ,
            ILockedCvx.LockedBalance[] memory lockedBalances
        ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
        uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
        cvxUnlockObligations += cvxAmount;

        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));

+       if (totalLockedBalancePlusUnlockable >= amount) {
+           withdrawIdToWithdrawRequestInfo[
+               latestWithdrawId
+           ] = WithdrawRequestInfo({
+               cvxOwed: cvxAmount,
+               withdrawn: false,
+               epoch: currentEpoch,
+               owner: msg.sender
+           });
+           emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
+           return latestWithdrawId;
+       }

        for (uint256 i = 0; i < lockedBalances.length; i++) {
            totalLockedBalancePlusUnlockable += lockedBalances[i].amount;
            // we found the epoch at which there is enough to unlock this position
            if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations) {
                (, uint32 currentEpochStartingTime) = ILockedCvx(VLCVX_ADDRESS)
                    .epochs(currentEpoch);
                uint256 timeDifference = lockedBalances[i].unlockTime -
                    currentEpochStartingTime;
                uint256 epochOffset = timeDifference /
                    ILockedCvx(VLCVX_ADDRESS).rewardsDuration();
                uint256 withdrawEpoch = currentEpoch + epochOffset;
                withdrawIdToWithdrawRequestInfo[
                    latestWithdrawId
                ] = WithdrawRequestInfo({
                    cvxOwed: cvxAmount,
                    withdrawn: false,
                    epoch: withdrawEpoch,
                    owner: msg.sender
                });

                emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
                return latestWithdrawId;
            }
        }
        // should never get here
        revert InvalidLockedAmount();
    }

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as duplicate of #63

[0xleastwood]

It seems pretty severe that requestWithdraw() would fail when the lockedBalances array is empty right?

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[elmutt]

It seems pretty severe that requestWithdraw() would fail when the lockedBalances array is empty right?

Absolutely agree. we are working on a fix to address this issue and will post it here in the coming days when ready

[romeroadrian]

It seems pretty severe that requestWithdraw() would fail when the lockedBalances array is empty right?

Giving the potential DoS here, do you think this could be upgraded to a high?

[0xleastwood]

Giving the potential DoS here, do you think this could be upgraded to a high?

So I think this DoS is recoverable. If all locks are unlocked/expired, then it will not be possible to request a withdrawal. However, if relock() is called, then additional requests to withdraw can be processed and the final impact is that the request is delayed more than it needs to be.

[0xleastwood]

It's not possible to prevent withdrawal finalisation by calling relock() first either because cvxUnlockObligations is used to reserve cvx that is owed to existing withdrawal requests.

[0xleastwood]

I think I will leave it as is unless there is a way to brick funds by preventing users from finalising their withdrawals