Summary
The removeVault function in the provided Solidity code contains broken logic for removing vaults. The function incorrectly handles the removal process, leading to potential loss of valid vaults and operational issues.
Vulnerability Details
https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/linkStaking/OperatorVCS.sol#L304-L330
The remove() function is expected to remove a particular vault address after updating its strategy rewards. The function after updating strategy rewards successfully however fails to remove intended vault correctly.
The function end ups doubling the vault address for (Index + 1) and removes the the last vault which is not supposed to be removed from storage
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
High
Tools Used
manual review
Recommendations
Adjust the function to correctly handle the removal of vaults without affecting valid vaults.
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; ++i) {
++ vaults[index] = vaults[numVaults - 1];
}
vaults.pop();
token.safeTransfer(address(stakingPool), token.balanceOf(address(this)));
}
}
