Summary
When a vault is added to the OperatorVCS the function addVault is used:
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;
emit VaultAdded(_operator);
}
We can see that the vaultMapping for the concrete vault is set to true.
When removing a vault the function queueVaultRemoval is used and then the function removeVault:
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);
}
}
}
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, we can see that in both function this vaultMapping that was set to true is never set to false. The mapping is used in withdrawOperatorRewards:
function withdrawOperatorRewards(
address _receiver,
uint256 _amount
) external returns (uint256) {
if (!vaultMapping[msg.sender]) revert SenderNotAuthorized();
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;
}
This means that even if a vault is "removed" it will still be able to withdraw operator rewards.
Vulnerability Details
Let's have the following scenario:
Vault A is added via addVault.
Vault A is removed via queueVaultRemoval -> removeVault.
However, Vault A is able to call withdrawOperatorRewards setting _receiver and _amount and withdrawing the rewards even though it should not be able to.
Impact
Such an issue has a critical impact to the security of the OperatorVCS contract as no one is able to stop a vault from operating even after removing it.
Tools Used
Manual Review
Recommendations
Update the vaultMapping to reflect that a given vault was removed:
function removeVault(uint256 _queueIndex) public {
address vault = vaultsToRemove[_queueIndex];
+ vaultMapping[vault] = false;
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)));
}
