Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Missing `updateDeposits` call in `OperatorVCS::removeVault` leads to incorrect reward distribution to the Vault

kiteweb3

Summary

The OperatorVCS::removeVault contains the logic for removing a vault that has been previously queued. This function can be called by anyone without restriction. The actual removal is done by calling the OperatorVault::exitVault function on the vault being removed.

According to the Natspec (https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVault.sol#L219) comment in the OperatorVault::exitVault function, updateDeposits must be called before exitVault.This ensures that the vault's state is up-to-date before it's removed. However, the OperatorVCS::removeVault function is missing to call updateDeposits before calling exitVault. This means the vault's state isn't current when it's removed leading to incorrect accounting of rewards and deposits when removing a vault.

Link: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVCS.sol#L311

Vulnerability Details

@> 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

Add this test in operator-vault.test.ts and run yarn test

it.only('removeVault should call updateDeposit before exitVault for working correctly', async () => {
const { accounts, adrs, strategy, token, vault, rewardsController, stakingController } =
await loadFixture(deployFixture)
​
await strategy.deposit(toEther(100))
assert.equal(fromEther(await token.balanceOf(adrs.stakingController)), 100)
assert.equal(fromEther(await stakingController.getStakerPrincipal(adrs.vault)), 100)
assert.equal(fromEther(await vault.getTotalDeposits()), 100)
assert.equal(fromEther(await vault.getUnclaimedRewards()), 0)
assert.equal(fromEther(await vault.trackedTotalDeposits()), 100)
​
await rewardsController.setReward(adrs.vault, toEther(10))
​
assert.equal(fromEther(await vault.getTotalDeposits()), 110)
assert.equal(fromEther(await vault.getUnclaimedRewards()), 0) //Should be 1
assert.equal(fromEther(await vault.trackedTotalDeposits()), 100) //Should be 110
assert.equal(fromEther(await vault.getPendingRewards()), 1) // Should be 0
​
await stakingController.removeOperator(adrs.vault)
assert.deepEqual(
(await strategy.removeVault.staticCall()).map((v) => fromEther(v)),
[100, 10]
)
await strategy.removeVault()
​
assert.equal(fromEther(await vault.getTotalDeposits()), 0)
assert.equal(fromEther(await vault.getUnclaimedRewards()), 0) //Should be 0.5
assert.equal(fromEther(await vault.trackedTotalDeposits()), 100) //Should be 110
assert.equal(fromEther(await vault.getPendingRewards()), 0)
})
​
Logs:
OperatorVault
✔ removeVault should call updateDeposit before exitVault for working correctly (2396ms)
​
​
1 passing (2s)

Calling updateDeposits before calling exitVault is necessary to update the vault's accounting for rewards earned since the last update. This ensures that all earned rewards are credited to the vault before the vault is exited. Calling _updateStrategyRewards alone will only credit the strategy and not the individual vaults. The OperatorVCS::removeVault function does not call updateDeposits before calling exitVault. This leads to incorrect accounting of rewards and deposits when removing a vault.

Tools Used

Manual review

Recommendations

Call the updateDeposit before the IOperatorVault(vault).exitVault() call in the OperatorVCS::removeVault.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

[INVALID] `exitVault` doesn't call `updateDeposits` before calling `_withdrawRewards` in the vault removal process

Appeal created

inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[INVALID] `exitVault` doesn't call `updateDeposits` before calling `_withdrawRewards` in the vault removal process

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.