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

Miscalculation of totalUnbonded prevents VaultDepositController::withdraw from being called, locking funds in the vault

strapontin

Summary

A miscalculation in the logic that determines the value for totalUnbonded prevents it from ever being different than 0.

This prevents successful withdrawals from VaultDepositController::withdraw, and causes VaultControllerStrategy::getMinDeposits to always return totalDeposits, ultimately preventing WithdrawalPool::checkUpkeep to ever return true.

Vulnerability Details

There are only two places where VaultControllerStrategy::totalUnbonded is updated : VaultDepositController::withdraw and VaultControllerStrategy::updateVaultGroups. As this report will show, both of those cannot update the value of totalUnbonded to another value than 0.

Issue in withdraw function

The function VaultDepositController::withdraw begins by checking whether the withdrawal amount is greater than totalUnbonded.

Since totalUnbonded is always 0 due to a miscalculation, the function reverts whenever it's called, preventing any withdrawal from happening. This also prevents the assignation of totalUnbonded to another value in this function.

Issue in updateVaultGroups function

The function VaultControllerStrategy::updateVaultGroups is called only by FundFlowController, for both operatorVCS and communityVCS. The function takes the parameter _nextGroupTotalUnbonded, which is the value we are setting to totalUnbonded.

For both operatorVCS and communityVCS, the variable passed as _nextGroupTotalUnbonded is calculated by the function FundFlowController::_getVaultUpdateData, which sets the variable nextGroupTotalUnbonded from the result of FundFlowController::_getTotalUnbonded.

The result variable in this function, totalUnbonded, is computed by adding the results of vault.getPrincipalDeposits() for each vault passed in parameter (each vault of the group to unbond) that is not removed and for which the claim period is active. However, since we are trying to get the totalUnbonded value of all vaults that we are about to unbond, none of these vault can have claimPeriodActive() return true, so the returned value of totalUnbonded will always be 0, which will later be set to VaultControllerStrategy::updateVaultGroups from the FundFlowController.

A straightforward summary for the issue in updateVaultGroups :

Impact

The miscalculation of totalUnbonded results in funds being permanently locked within vaults, as VaultDepositController::withdraw can never be executed successfully.

Additionally, StakingPool::canWithdraw will always return 0. Since WithdrawalPool::performUpkeep relies on this value being different to 0 to execute, the execution of the WithdrawalPool::performUpkeep function to always fail. While the WithdrawalPool can still execute withdrawals with deposits from the PriorityPool, the inability to run performUpkeep leads to the protocol not being able to finalize the withdrawals for users from the tokens of the staking pool.

PoC

The following PoC can be copy-pasted in test/core/priorityPool/, and run with :

npx hardhat test --grep "totalUnbonded is always 0"

The PoC proves the following points :

  • VaultControllerStrategy::totalUnbonded is always 0.

  • VaultDepositController::withdraw always revert.

  • WithdrawalPool::checkUpkeep returns false due to PriorityPool::canWithdraw consistently returning 0.

import { loadFixture, time } from '@nomicfoundation/hardhat-network-helpers'
import { assert, expect } from 'chai'
import { ethers } from 'hardhat'
import {
CommunityVCS,
ERC677,
FundFlowController,
OperatorVCS,
PriorityPool,
SDLPoolMock,
StakingMock,
StakingPool,
StakingRewardsMock,
WithdrawalPool,
} from '../../../typechain-types'
import {
deploy,
deployImplementation,
deployUpgradeable,
fromEther,
getAccounts,
setupToken,
toEther,
} from '../../utils/helpers'
​
describe('Workflow', () => {
async function deployFixture() {
const { signers, accounts } = await getAccounts()
const adrs: any = {}
​
const linkToken = (await deploy('contracts/core/tokens/base/ERC677.sol:ERC677', [
'Chainlink',
'LINK',
1000000000,
])) as ERC677
adrs.linkToken = await linkToken.getAddress()
await setupToken(linkToken, accounts, true)
​
// The following 2 mocks are the only mock contracts because
// we don't have access to the official Chainlink contracts in the codebase
const rewardsController = (await deploy('StakingRewardsMock', [
adrs.linkToken,
])) as StakingRewardsMock
adrs.rewardsController = await rewardsController.getAddress()
​
const unbondingPeriod = 28 * 86400
const claimPeriod = 7 * 86400
​
const stakingController = (await deploy('StakingMock', [
adrs.linkToken,
adrs.rewardsController,
toEther(10),
toEther(100),
toEther(10000),
unbondingPeriod,
claimPeriod,
])) as StakingMock
adrs.stakingController = await stakingController.getAddress()
​
const stakingPool = (await deployUpgradeable('StakingPool', [
adrs.linkToken,
'Staked LINK',
'stLINK',
[],
toEther(10000),
])) as StakingPool
adrs.stakingPool = await stakingPool.getAddress()
​
let communityVault = await deployImplementation('CommunityVault')
​
const vaultDepositController = await deploy('VaultDepositController')
​
const communityVCS = (await deployUpgradeable(
'CommunityVCS',
[
adrs.linkToken,
adrs.stakingPool,
adrs.stakingController,
communityVault,
[],
9000,
toEther(100),
10,
20,
vaultDepositController.target,
],
{ unsafeAllow: ['delegatecall'] }
)) as CommunityVCS
adrs.communityVCS = await communityVCS.getAddress()
​
let operatorVault = await deployImplementation('OperatorVault')
const operatorVCS = (await deployUpgradeable(
'OperatorVCS',
[
adrs.linkToken,
adrs.stakingPool,
adrs.stakingController,
operatorVault,
[],
9000,
toEther(100),
1000,
vaultDepositController.target,
],
{ unsafeAllow: ['delegatecall'] }
)) as OperatorVCS
adrs.operatorVCS = await operatorVCS.getAddress()
​
const fundFlowController = (await deployUpgradeable('FundFlowController', [
operatorVCS.target,
communityVCS.target,
unbondingPeriod,
claimPeriod,
5,
])) as FundFlowController
​
await communityVCS.setFundFlowController(fundFlowController.target)
await operatorVCS.setFundFlowController(fundFlowController.target)
​
const sdlPool = (await deploy('SDLPoolMock')) as SDLPoolMock
adrs.sdlPool = await sdlPool.getAddress()
​
const pp = (await deployUpgradeable('PriorityPool', [
adrs.linkToken,
adrs.stakingPool,
adrs.sdlPool,
toEther(100),
toEther(1000),
])) as PriorityPool
adrs.pp = await pp.getAddress()
​
const withdrawalPool = (await deployUpgradeable('WithdrawalPool', [
adrs.linkToken,
adrs.stakingPool,
adrs.pp,
toEther(10),
86400,
])) as WithdrawalPool
adrs.withdrawalPool = await withdrawalPool.getAddress()
​
await linkToken.approve(adrs.communityVCS, ethers.MaxUint256)
await linkToken.approve(adrs.operatorVCS, ethers.MaxUint256)
await linkToken.transfer(adrs.rewardsController, toEther(10000))
​
await stakingPool.addStrategy(adrs.communityVCS)
await stakingPool.addStrategy(adrs.operatorVCS)
await stakingPool.setPriorityPool(adrs.pp)
await stakingPool.setRebaseController(accounts[0])
await pp.setDistributionOracle(accounts[0])
await pp.setWithdrawalPool(adrs.withdrawalPool)
​
for (let i = 0; i < signers.length; i++) {
await linkToken.connect(signers[i]).approve(adrs.pp, ethers.MaxUint256)
await linkToken.connect(signers[i]).approve(adrs.stakingPool, ethers.MaxUint256)
await stakingPool.connect(signers[i]).approve(adrs.pp, ethers.MaxUint256)
}
​
return {
signers,
adrs,
pp,
stakingPool,
communityVCS,
fundFlowController,
}
}
​
it('totalUnbonded is always 0', async () => {
const { signers, adrs, pp, stakingPool, communityVCS, fundFlowController } = await loadFixture(
deployFixture
)
​
/**
* Function to test
* - 1. totalUnbonded == 0 is always true
* - 2. VaultDepositController::withdraw always reverts
* - 3. WithdrawalPool::checkUpkeep will always return false because :
* - PriorityPool::canWithdraw will always return 0
*/
async function testInvariants() {
// 1.`totalUnbonded` is always 0
assert.isTrue(fromEther(await communityVCS.totalUnbonded()) == 0)
​
// 2. `StakingPool::strategyWithdraw` directly calls `withdraw` in the strategy
// Since `totalUnbonded` is always 0, this will always fail
await expect(stakingPool.strategyWithdraw(0, 1, '0x')).to.be.revertedWithCustomError(
communityVCS,
'WithdrawalFailed'
)
​
// 3. `PriorityPool::canWithdraw` always returns 0 for the withdrawalPool, preventing `performUpkeep` to run
assert.isTrue(fromEther(await pp.canWithdraw(adrs.withdrawalPool, 0)) == 0)
}
​
/**
* Start of simulation
*/
​
// Users are staking
const emptyVaultId = ethers.AbiCoder.defaultAbiCoder().encode(['uint64[]'], [[]])
await pp.connect(signers[0]).deposit(toEther(1000), true, [emptyVaultId])
await pp.connect(signers[1]).deposit(toEther(1000), true, [emptyVaultId])
​
// 20 should be significant enough, but you're free to increase it as you wish
const numberOfTimesToTry = 20
for (let i = 0; i < numberOfTimesToTry; i++) {
await fundFlowController.updateVaultGroups()
​
// Add this function before and after the call to updateVaultGroups, to prove
// that it fails all the time
await testInvariants()
​
await time.increase(
(await fundFlowController.unbondingPeriod()) + (await fundFlowController.claimPeriod())
)
await testInvariants()
}
​
// This PoC proves that `totalUnbonded` is always 0 by being unable to update it to another value
// everywhere the variable is assigned
})
})

Tools Used

Manual review.

Recommendations

If FundFlowController::_getTotalUnbonded is edited to not ignore vaults that aren't in the claim period, this would fix the value of totalUnbonded for this vulnerability, but it may impact other parts of the protocol, such as FundFlowController::updateOperatorVaultGroupAccounting as it loops through all _vaultGroups and not only the next one to unbound.

To mitigate this, consider creating a separate function, _getTotalUnbondedForNextVaultGroup, to handle the unbonding process specifically.

The following code diff illustrates the recommended solution :

// Do not edit `_getTotalUnbonded`.
// Instead, create a new function `_getTotalUnbondedForNextVaultGroup`
// The following code highlights how the 2 functions will be differents
- function _getTotalUnbonded(
+ function _getTotalUnbondedForNextVaultGroup(
address[] memory _vaults,
uint256 _numVaultGroups,
uint256 _vaultGroup
) internal view returns (uint256) {
uint256 totalUnbonded;
​
for (uint256 i = _vaultGroup; i < _vaults.length; i += _numVaultGroups) {
- if (!IVault(_vaults[i]).claimPeriodActive() || IVault(_vaults[i]).isRemoved()) continue;
+ if (IVault(_vaults[i]).isRemoved()) continue;
​
totalUnbonded += IVault(_vaults[i]).getPrincipalDeposits();
}
​
return totalUnbonded;
}
​
​
...
...
...
​
​
// This part is in the _getVaultUpdateData function
- uint256 nextGroupTotalUnbonded = _getTotalUnbonded(
+ uint256 nextGroupTotalUnbonded = _getTotalUnbondedForNextVaultGroup(
vaults,
numVaultGroups,
_nextUnbondedVaultGroup
);
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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