Summary
on OparetorVault.sol the function exitVault() says that @dev updateDeposits must be called before calling this function
on OparetorVSC.sol the removeVault function is calling exitVault() but not calling updateDeposits function.
This led to loss of funds.
Vulnerability Details
on OparetorVault.sol the function exitVault() says that @dev updateDeposits must be called before calling this function
I pointed out that comment.
* @notice Withdraws tokens from the Chainlink staking contract and sends them to the vault controller
👉 * @dev updateDeposits must be called before calling this function
* @dev used to withdraw remaining principal and rewards after operator has been removed
* @dev will also send any unclaimed operator rewards to rewards receiver
* @return total principal withdrawn
* @return total rewards withdrawn
*/
function exitVault() external onlyVaultController returns (uint256, uint256) {
if (!isRemoved()) revert OperatorNotRemoved();
uint256 opRewards = getUnclaimedRewards();
if (opRewards != 0) _withdrawRewards();
uint256 rewards = getRewards();
if (rewards != 0) rewardsController.claimReward();
uint256 principal = getPrincipalDeposits();
stakeController.unstakeRemovedPrincipal();
uint256 balance = token.balanceOf(address(this));
token.safeTransfer(vaultController, balance);
return (principal, rewards);
}
on OparetorVSC.sol the removeVault function is calling exitVault() but not calling updateDeposits function.
on the pointing line, it is calling the exitVault function. But before calling this function it must call updateDeposits function.
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)));
}
If we call the updateDeposits function with _minRewards of 1, then the test/linkStaking/operator-vcs.test.ts test file fails.
function removeVault(uint256 _queueIndex) public {
address vault = vaultsToRemove[_queueIndex];
vaultsToRemove[_queueIndex] = vaultsToRemove[vaultsToRemove.length - 1];
vaultsToRemove.pop();
_updateStrategyRewards();
👉✅ IOperatorVault(vault).updateDeposits(1, address(stakingPool));
(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)));
}
The failed message of (test\linkStaking\operator-vcs.test.ts:445:12)
❗❗❗❗❗
OperatorVCS
removeVault should work correctly:
AssertionError: expected 950 to equal 900
+ expected - actual
-950
+900
But if you set the updateDeposits _minRewards to 0 then the test pass. which means the stacking pool does not want any rewards that's why it is passing.
Impact
we must call the updateDeposits function otherwise the strategy loses funds.
Tools Used
Manually Reviewed
Recommendations
Call the updateDeposits function as mentioned on the impact.
Also, update the test file of (test\linkStaking\operator-vcs.test.ts:445:12)
--- assert.equal(fromEther(await strategy.getTotalDeposits()), 900)
+++ assert.equal(fromEther(await strategy.getTotalDeposits()), 950)
