VotiumStrategy.withdrawTime doesn't expect that balance can be already unlocked

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

VotiumStrategy.withdrawTime doesn't expect that balance can be already unlocked. As result user can wait more time to witdraw.

Proof of Concept

When user wants to withdraw, then he needs to initiate requestWithdraw. As some part of funds are locked as cvx token inside vlcvx that means that they should be withdrawn. When you lock cvx then amount is locked for some period, so you can't withdraw it for that time.

For such reason withdrawTime function exists, which purpose is to calculate time when needed amount will be available to withdraw.

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

    function withdrawTime(
        uint256 _amount
    ) external view virtual override returns (uint256) {
        uint256 _priceInCvx = cvxPerVotium();
        (
            ,
            uint256 unlockable,
            ,
            ILockedCvx.LockedBalance[] memory lockedBalances
        ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
        uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));

        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 + cvxAmount
            ) {
                return lockedBalances[i].unlockTime;
            }
        }
        revert InvalidLockedAmount();
    }

unlockable is amount that can be withdrawn right now from VLCVX_ADDRESS. And totalLockedBalancePlusUnlockable is total available balance at the moment.

As you can see, this code doesn't check that totalLockedBalancePlusUnlockable > amount, it checks it only inside the loop. And because of that, it's possible that balance is already enough, but function will use time of first lockedBalances[0].unlockTime as withdraw time, which will make user wait more to withdraw.

Exactly same thing exists in the requestWithdraw function as it actually does almost same and tries to find epoch, when funds will be available.

Tools Used

VsCode

Recommended Mitigation Steps

Consider possibility, that totalLockedBalancePlusUnlockable > amount even without additional withdraw.

Assessed type

Error

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as duplicate of #63

[c4-judge]

0xleastwood marked the issue as satisfactory