c4-submissions commented 9 months ago

Lines of code

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

Vulnerability details

Summary

The VotiumStrategy withdrawal process involves relocking CVX tokens, which can potentially lead to a denial of service and loss of user funds if the underlying vlCVX contract is shutdown.

Impact

When withdrawals are executed in VotiumStrategy, the implementation of withdraw() will call relock() in order to relock any available excess (i.e. expired tokens minus the pending obligations) of CVX tokens to lock them again in the Convex protocol.

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

135:     function relock() public {
136:         (, uint256 unlockable, , ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(
137:             address(this)
138:         );
139:         if (unlockable > 0)
140:             ILockedCvx(VLCVX_ADDRESS).processExpiredLocks(false);
141:         uint256 cvxBalance = IERC20(CVX_ADDRESS).balanceOf(address(this));
142:         uint256 cvxAmountToRelock = cvxBalance > cvxUnlockObligations
143:             ? cvxBalance - cvxUnlockObligations
144:             : 0;
145:         if (cvxAmountToRelock > 0) {
146:             IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmountToRelock);
147:             ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmountToRelock, 0);
148:         }
149:     }

This seems fine at first, but if we dig into the implementation of lock() we can see that the preconditions of this function requires the contract not to be shutdown:

https://etherscan.io/address/0x72a19342e8F1838460eBFCCEf09F6585e32db86E#code#L1469

521:     function _lock(address _account, uint256 _amount, uint256 _spendRatio, bool _isRelock) internal {
522:         require(_amount > 0, "Cannot stake 0");
523:         require(_spendRatio <= maximumBoostPayment, "over max spend");
524:         require(!isShutdown, "shutdown");

Which means that any call to lock() after the contract is shutdown will revert. This is particularly bad because relock() is called as part of the withdraw process. If the vlCVX contract is shutdown, VotiumStrategy depositors won't be able to withdraw their position, causing a potential loss of funds.

Note that this also will cause a DoS while depositing rewards, since depositRewards() in VotiumStrategy also calls ILockedCvx::lock().

Proof of Concept

vlCVX contract is shutdown.
A user requests a withdrawal using requestWithdrawal()
The user calls withdraw() when the withdrawal epoch is reached.
The implementation will call relock(), which will call ILockedCvx::lock().
The implementation of lock() will throw an error because the vault is already shutdown.
The transaction will be reverted.

Recommendation

Add a condition to check if the contract is shutdown to avoid the call to lock() and the potential denial of service.

    function relock() public {
        (, uint256 unlockable, , ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(
            address(this)
        );
        if (unlockable > 0)
            ILockedCvx(VLCVX_ADDRESS).processExpiredLocks(false);
        uint256 cvxBalance = IERC20(CVX_ADDRESS).balanceOf(address(this));
        uint256 cvxAmountToRelock = cvxBalance > cvxUnlockObligations
            ? cvxBalance - cvxUnlockObligations
            : 0;
-       if (cvxAmountToRelock > 0) {
+       if (cvxAmountToRelock > 0 && !ILockedCvx(VLCVX_ADDRESS).isShutdown()) {
            IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmountToRelock);
            ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmountToRelock, 0);
        }
    }