Summary
An issue in the VaultDepositController::withdraw function allows for rewards to be left untransferred if the StakingPool::updateStrategyRewards function has not been called prior to withdrawal. This can result in users receiving less than they are entitled to, leading to potential financial loss and missed rewards.
Vulnerability Details
When VaultDepositController::withdraw is triggered, tokens are withdrawn from the vault. However, there is no guarantee that the StakingPool::updateStrategyRewards function has been called prior to this withdrawal. As a result, rewards that have accrued but not yet been transferred may remain unaccounted for, leading to discrepancies in the vault balance.
This discrepancy occurs because Vault::getPrincipalDeposits is used instead of Vault::getTotalDeposits, which could cause the vault's balance to appear smaller than it actually is.
function withdraw(uint256 _amount, bytes calldata _data) external {
if (!fundFlowController.claimPeriodActive() || _amount > totalUnbonded)
revert InsufficientTokensUnbonded();
GlobalVaultState memory globalState = globalVaultState;
uint64[] memory vaultIds = abi.decode(_data, (uint64[]));
VaultGroup memory group = vaultGroups[globalState.curUnbondedVaultGroup];
if (vaultIds[0] != group.withdrawalIndex) revert InvalidVaultIds();
uint256 toWithdraw = _amount;
uint256 unbondedRemaining = totalUnbonded;
(uint256 minDeposits, ) = getVaultDepositLimits();
for (uint256 i = 0; i < vaultIds.length; ++i) {
if (vaultIds[i] % globalState.numVaultGroups != globalState.curUnbondedVaultGroup)
revert InvalidVaultIds();
group.withdrawalIndex = uint64(vaultIds[i]);
IVault vault = vaults[vaultIds[i]];
@> uint256 deposits = vault.getPrincipalDeposits();
if (deposits != 0 && vault.claimPeriodActive() && !vault.isRemoved()) {
if (toWithdraw > deposits) {
vault.withdraw(deposits);
unbondedRemaining -= deposits;
toWithdraw -= deposits;
} else if (deposits - toWithdraw > 0 && deposits - toWithdraw < minDeposits) {
vault.withdraw(deposits);
unbondedRemaining -= deposits;
break;
} else {
vault.withdraw(toWithdraw);
unbondedRemaining -= toWithdraw;
break;
}
}
}
uint256 totalWithdrawn = totalUnbonded - unbondedRemaining;
token.safeTransfer(msg.sender, totalWithdrawn);
totalDeposits -= totalWithdrawn;
totalPrincipalDeposits -= totalWithdrawn;
totalUnbonded = unbondedRemaining;
group.totalDepositRoom += uint128(totalWithdrawn);
vaultGroups[globalVaultState.curUnbondedVaultGroup] = group;
}
Consequently, the system may transfer fewer tokens than intended, resulting in potential losses for users due to missed profits.
# PoC
In vault-controller-strategy.test.ts add the following test case
it.only('withdraw inaccurate amount', async () => {
const { adrs, strategy, token, stakingController, vaults, fundFlowController, rewardsController, vaultContracts } =
await loadFixture(deployFixture)
await strategy.deposit(toEther(1200), encodeVaults([]))
await fundFlowController.updateVaultGroups()
await time.increase(claimPeriod)
await fundFlowController.updateVaultGroups()
await time.increase(claimPeriod)
await fundFlowController.updateVaultGroups()
await time.increase(claimPeriod)
await fundFlowController.updateVaultGroups()
await time.increase(claimPeriod)
await fundFlowController.updateVaultGroups()
await expect(
strategy.withdraw(toEther(150), encodeVaults([5, 10]))
).to.be.revertedWithCustomError(strategy, 'WithdrawalFailed()')
await expect(
strategy.withdraw(toEther(150), encodeVaults([0, 1]))
).to.be.revertedWithCustomError(strategy, 'WithdrawalFailed()')
console.log(fromEther(await vaultContracts[5].getTotalDeposits()), " vault contract total deposit - INITIAL")
await rewardsController.setReward(vaults[5], toEther(20))
console.log(fromEther(await vaultContracts[5].getTotalDeposits()), " vault contract total deposit - UPDATED WITH REWARD")
await strategy.withdraw(toEther(300), encodeVaults([0, 5]))
assert.equal(fromEther(await token.balanceOf(adrs.stakingController)), 1000)
assert.equal(fromEther(await stakingController.getStakerPrincipal(vaults[0])), 0)
assert.equal(fromEther(await stakingController.getStakerPrincipal(vaults[5])), 0)
console.log(fromEther(await vaultContracts[5].getTotalDeposits()), " vault contract total deposit - AFTER WITHDRAWAL")
})
Impact
Exploiting this vulnerability can lead to financial losses and missed rewards for users, resulting in potential lost profits and reduced trust in the protocol's operations.
Tools Used
Manual Review
Recommendations
Use Vault::getTotalDeposits instead of Vault::getPrincipalDeposits
