`VotiumStrategy.requestWithdraw()` does not account for immediately possible withdrawals

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/6b4867491350f8327d0ac4f496f263642cf3c1be/contracts/strategies/votium/VotiumStrategy.sol#L78

Vulnerability details

Impact

A user might be given an unnecessarily late withdrawal epoch. VotiumStrategy.requestWithdraw() might revert altogether.

Proof of Concept

VotiumStrategy.requestWithdraw() finds the epoch at which withdrawal is possible by the following logic:

(
    ,
    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));

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) {
        /*
        Withdraw epoch set to the unlock time of lockedBalances[i].
        */
    }
}
// should never get here
revert InvalidLockedAmount();

This fails to account for when totalLockedBalancePlusUnlockable is sufficient for immediate withdrawal (before the for-loop), and at best sets it to the epoch of the first locked balance. Withdrawal which should be possible immediately is thus artificially given an extended lock time. If all locks have expired then lockedBalances is a zero-length array and the for-loop is skipped and it reaches revert InvalidLockedAmount().

Recommended Mitigation Steps

Check whether totalLockedBalancePlusUnlockable >= cvxUnlockObligations before adding any lockedBalances[i].amount and set the withdrawal to the current epoch or immediately effectuate a withdrawal.

Assessed type

Invalid Validation

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked issue #49 as primary and marked this issue as a duplicate of 49

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-sponsor]

elmutt (sponsor) confirmed