Summary
The Protocol is designed in such a way that each vault has deposit limits i.e. the minimum deposit that a vault should hold and the maximum deposit that a vault can hold. The protocol is not supposed to permit withdrawing from a vault such that deposits in the vault go below the minimum deposit amount.
Unfortunately, with the VaultControllerStrategy::withdraw, withdrawal can be made from a vault such that the amount left in the vault goes below the minimum amount the vault is supposed to hold, breaking the protocol's functionality.
Vulnerability Details
The issue with the VaultControllerStrategy::withdraw function is that when withdrawing from a vault, the amount to be withdrawn is not calculated in such a manner as to ensure the vault still holds the minimum deposit required. See the code snippet below:
function withdraw(uint256 _amount, bytes calldata _data) external {
if (!fundFlowController.claimPeriodActive() || _amount > totalUnbonded)
revert InsufficientTokensUnbonded();
GlobalVaultState memory globalState = globalVaultState;
uint64[] memory vaultIds = abi.decode(_data, (uint64[]));
VaultGroup memory group = vaultGroups[globalState.curUnbondedVaultGroup];
if (vaultIds[0] != group.withdrawalIndex) revert InvalidVaultIds();
uint256 toWithdraw = _amount;
uint256 unbondedRemaining = totalUnbonded;
(uint256 minDeposits, ) = getVaultDepositLimits();
for (uint256 i = 0; i < vaultIds.length; ++i) {
if (vaultIds[i] % globalState.numVaultGroups != globalState.curUnbondedVaultGroup)
revert InvalidVaultIds();
group.withdrawalIndex = uint64(vaultIds[i]);
IVault vault = vaults[vaultIds[i]];
uint256 deposits = vault.getPrincipalDeposits();
if (deposits != 0 && vault.claimPeriodActive() && !vault.isRemoved()) {
if (toWithdraw > deposits) {
@> vault.withdraw(deposits);
unbondedRemaining -= deposits;
toWithdraw -= deposits;
} else if (deposits - toWithdraw > 0 && deposits - toWithdraw < minDeposits) {
@> vault.withdraw(deposits);
unbondedRemaining -= deposits;
break;
} else {
@> vault.withdraw(toWithdraw);
unbondedRemaining -= toWithdraw;
break;
}
}
}
uint256 totalWithdrawn = totalUnbonded - unbondedRemaining;
token.safeTransfer(msg.sender, totalWithdrawn);
totalDeposits -= totalWithdrawn;
totalPrincipalDeposits -= totalWithdrawn;
totalUnbonded = unbondedRemaining;
group.totalDepositRoom += uint128(totalWithdrawn);
vaultGroups[globalVaultState.curUnbondedVaultGroup] = group;
}
Here is the github link to the code snippet above https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L111-L163
In fact, irrespective of the amount to be withdrawn, the minimum vault balance is likely to be broken. To see this clearly, let us look at the three different scenario in the above code snippet.
-
If the amount we intend to withdraw is greater than the deposits in the vault, the protocol withdraws the entire deposits in the vault without considering the vault balance after the withdrawal
if (toWithdraw > deposits) {
@> vault.withdraw(deposits);
unbondedRemaining -= deposits;
toWithdraw -= deposits;
}
where the above code is executed, the vault can be left with a zero balance after the withdrawal.
-
On the other hand, where the amount to withdraw is less than the vault deposits but the positive difference is less than the minimum deposits that the vault should hold, the protocol will go ahead and withdraw the entire vault deposits. Not only does this operation deplete the entire vault deposits, it also withdraws even more than was intended for no justifiable reason. Besides, this section will hardly be executed because of the first code block which encompases the conditions to execute this code block.
else if (deposits - toWithdraw > 0 && deposits - toWithdraw < minDeposits) {
@> vault.withdraw(deposits);
unbondedRemaining -= deposits;
break;
}
-
Lastly, where the amount to withdraw is less than the vault deposits, the protocol goes ahead to withdraw the intended amount. Here also, the protocol does not check what the vault balance would be after the withdrawal.
@> vault.withdraw(toWithdraw); // @audit-comment this withdraws the intended amount without ensuring minimum vault balance is preserved
unbondedRemaining -= toWithdraw;
break;
}
By and large, the `VaultControllerStrategy::withdraw` function permits for a vault to be depleted of its deposits even below the minimum amount of deposits the vault is supposed to hold.
## Impact
Because the `VaultControllerStrategy::withdraw` function permits for a vault balance below the minimum amount of deposits permitted for a vault, it breaks one of the protocol's invariants, affecting the protocol's functionality.
## Tools Used
Manual Review
## Recommendations
Consider adjusting the `VaultControllerStrategy::withdraw` function so that before any withdrawal is made from a vault, the protocol checks to ensure that the vault balance does not fall below the minimum deposits the vault should hold.
```diff
function withdraw(uint256 _amount, bytes calldata _data) external {
if (!fundFlowController.claimPeriodActive() || _amount > totalUnbonded)
revert InsufficientTokensUnbonded();
GlobalVaultState memory globalState = globalVaultState;
uint64[] memory vaultIds = abi.decode(_data, (uint64[]));
VaultGroup memory group = vaultGroups[globalState.curUnbondedVaultGroup];
if (vaultIds[0] != group.withdrawalIndex) revert InvalidVaultIds();
uint256 toWithdraw = _amount;
uint256 unbondedRemaining = totalUnbonded;
(uint256 minDeposits, ) = getVaultDepositLimits();
for (uint256 i = 0; i < vaultIds.length; ++i) {
if (vaultIds[i] % globalState.numVaultGroups != globalState.curUnbondedVaultGroup)
revert InvalidVaultIds();
group.withdrawalIndex = uint64(vaultIds[i]);
IVault vault = vaults[vaultIds[i]];
uint256 deposits = vault.getPrincipalDeposits();
if (deposits != 0 && vault.claimPeriodActive() && !vault.isRemoved()) {
- if (toWithdraw > deposits) {
- vault.withdraw(deposits);
- unbondedRemaining -= deposits;
- toWithdraw -= deposits;
- } else if (deposits - toWithdraw > 0 && deposits - toWithdraw < minDeposits) {
-
- vault.withdraw(deposits);
- unbondedRemaining -= deposits;
- break;
- } else {
- vault.withdraw(toWithdraw);
- unbondedRemaining -= toWithdraw;
- break;
- }
+ if (toWithdraw > deposits && deposits - minDeposits > 0) {
+ vault.withdraw(deposits - minDeposits);
+ unbondedRemaining -= deposits - minDeposits;
+ toWithdraw -= deposits - minDeposits;
+ }
+ else {
+ if(toWithdraw - minDeposits > 0){
+ vault.withdraw(toWithdraw - minDeposits);
+ unbondedRemaining -= toWithdraw - minDeposits;
+ break;
+ }
+ }
}
}
uint256 totalWithdrawn = totalUnbonded - unbondedRemaining;
token.safeTransfer(msg.sender, totalWithdrawn);
totalDeposits -= totalWithdrawn;
totalPrincipalDeposits -= totalWithdrawn;
totalUnbonded = unbondedRemaining;
group.totalDepositRoom += uint128(totalWithdrawn);
vaultGroups[globalVaultState.curUnbondedVaultGroup] = group;
}
Thus when withdrawals are made from any vault, the minimum deposit amount that should be held by the vault is preserved.
