Summary
Operator Vault Controller Strategy contract is able to queue a vault for removal when the operator has been removed from the Chainlink staking contract using the queueVaultRemoval function. The vault can only be removed when the claim period is active for the vault contract in the Chainlink staking contract. However, since removeVault function is public and has no control access, anyone can remove a queued vault even when the required conditions are not met, resulting in unexpected behavior of the strategy system.
Vulnerability Details
When a vault operator has been removed from the Chainlink staking contract, the vault is queued for removal in the OperatorVCS contract via the queueVaultRemoval function. The vault is removed by calling the removeVault function only if the claim period is active in the Chainlink staking contract.
function queueVaultRemoval(uint256 _index) external {
address vault = address(vaults[_index]);
if (!IVault(vault).isRemoved()) revert OperatorNotRemoved();
for (uint256 i = 0; i < vaultsToRemove.length; ++i) {
if (vaultsToRemove[i] == vault) revert VaultRemovalAlreadyQueued();
}
vaultsToRemove.push(address(vaults[_index]));
if (_index < globalVaultState.depositIndex) {
uint256 group = _index % globalVaultState.numVaultGroups;
uint256[] memory groups = new uint256[]();
groups[0] = group;
fundFlowController.updateOperatorVaultGroupAccounting(groups);
@> if (vaults[_index].claimPeriodActive()) {
removeVault(vaultsToRemove.length - 1);
}
}
}
However, the removeVault function is public and has no control access, allowing any user to remove a vault that has been queued even if the claim period is not yet active.
@> function removeVault(uint256 _queueIndex) public {
address vault = vaultsToRemove[_queueIndex];
vaultsToRemove[_queueIndex] = vaultsToRemove[vaultsToRemove.length - 1];
vaultsToRemove.pop();
_updateStrategyRewards();
(uint256 principalWithdrawn, uint256 rewardsWithdrawn) = IOperatorVault(vault).exitVault();
totalDeposits -= principalWithdrawn + rewardsWithdrawn;
totalPrincipalDeposits -= principalWithdrawn;
uint256 numVaults = vaults.length;
uint256 index;
for (uint256 i = 0; i < numVaults; ++i) {
if (address(vaults[i]) == vault) {
index = i;
break;
}
}
for (uint256 i = index; i < numVaults - 1; ++i) {
vaults[i] = vaults[i + 1];
}
vaults.pop();
token.safeTransfer(address(stakingPool), token.balanceOf(address(this)));
}
Impact
Impact: High
A vault should only be removed when the required conditions are met.
Likelihood: Medium
Tools Used
Manual Review
Recommendations
Adding control access to the removeVault function will prevent users from remove queued vaults when the required conditions are not met.
- function removeVault(uint256 _queueIndex) public {
+ function removeVault(uint256 _queueIndex) public onlyOwner{
address vault = vaultsToRemove[_queueIndex];
vaultsToRemove[_queueIndex] = vaultsToRemove[vaultsToRemove.length - 1];
vaultsToRemove.pop();
_updateStrategyRewards();
(uint256 principalWithdrawn, uint256 rewardsWithdrawn) = IOperatorVault(vault).exitVault();
totalDeposits -= principalWithdrawn + rewardsWithdrawn;
totalPrincipalDeposits -= principalWithdrawn;
uint256 numVaults = vaults.length;
uint256 index;
for (uint256 i = 0; i < numVaults; ++i) {
if (address(vaults[i]) == vault) {
index = i;
break;
}
}
for (uint256 i = index; i < numVaults - 1; ++i) {
vaults[i] = vaults[i + 1];
}
vaults.pop();
token.safeTransfer(address(stakingPool), token.balanceOf(address(this)));
}
