Summary
Vaults can still have access to controlled functions even after removal via OperatorVCS::queueVaultRemoval and OperatorVCS::removeVault
Vulnerability Details
The OperatorVCS::queueVaultRemoval and OperatorVCS::removeVault are used to remove a vault so long its Operator has been removed.
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVCS.sol#L276-L330
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);
}
}
}
* @notice Removes a vault that has been queued for removal
* @param _queueIndex index of vault in removal queue
*/
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)));
}
However they fail to remove a core access privilege in OperatorVCS::vaultMapping. For context here's the OperatorVCS::addVault that shows when OperatorVCS::vaultMapping is set true.
function addVault(
address _operator,
address _rewardsReceiver,
address _pfAlertsController
) external onlyOwner {
bytes memory data = abi.encodeWithSignature(
"initialize(address,address,address,address,address,address,address)",
address(token),
address(this),
address(stakeController),
stakeController.getRewardVault(),
_pfAlertsController,
_operator,
_rewardsReceiver
);
_deployVault(data);
vaultMapping[address(vaults[vaults.length - 1])] = true; <@audit
emit VaultAdded(_operator);
}
Since that very line indicated above is not set to false during the vault removal process, the vault address is still privy to bypass some access control checks, like in OperatorVCS::withdrawOperatorRewards
function withdrawOperatorRewards(
address _receiver,
uint256 _amount
) external returns (uint256) {
if (!vaultMapping[msg.sender]) revert SenderNotAuthorized(); <@audit
IERC20Upgradeable lsdToken = IERC20Upgradeable(address(stakingPool));
uint256 withdrawableRewards = lsdToken.balanceOf(address(this));
uint256 amountToWithdraw = _amount > withdrawableRewards ? withdrawableRewards : _amount;
unclaimedOperatorRewards -= amountToWithdraw;
lsdToken.safeTransfer(_receiver, amountToWithdraw);
return amountToWithdraw;
}
The impact of this is that the vault address can still withdraw operator funds. And worse, they can use any arbitrary _receiver address, including theirs. And this will go probably unnoticed since it emits no events too.
Impact
The impact of this is that the vault address can still withdraw operator funds.
Tools Used
Manual Review.
Recommendations
Set vaultMapping[vault] for removed vaults to false during vault removal.
