Forced relock in VotiumStrategy withdrawal causes denial of service if Convex locking contract is shutdown
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.
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.
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()
.
- 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 callILockedCvx::lock()
. - The implementation of
lock()
will throw an error because the vault is already shutdown. - The transaction will be reverted.
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);
}
}