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

The withdrawal index can be set to an index outside of the group, resulting in incorrect totalDepositRoom accounting

rajko_z

Summary

Vaults are distributed into vault groups. Each vault group has a withdrawal index that represents the next vault in the group from which a withdrawal operation will be performed. Additionally, every group has a totalDepositRoom, representing how much can be deposited across all the vaults in the group.

struct VaultGroup {
// index of next vault in the group to be withdrawn from
uint64 withdrawalIndex;
// total deposit room across all vaults in the group
uint128 totalDepositRoom;
}

Besides vault groups, there are vaults that do not need to be part of a group. The next such vault that accepts deposits is represented by the depositIndex inside the GlobalVaultState.

struct GlobalVaultState {
// total number of groups
uint64 numVaultGroups;
// index of the current unbonded group
uint64 curUnbondedVaultGroup;
// index of next vault to receive deposits across all groups
uint64 groupDepositIndex;
// index of next non-group vault to receive deposits
@> uint64 depositIndex;
}

When there is a new deposit, the _depositToVaults function will be called on the strategy, specifically the VaultControllerStrategy, which is inherited by both OperatorVCS and CommunityVCS. The new deposit is performed by passing a list of vaultIds. If one of the vaults is a withdrawal vault in its group and does not have any deposits, the group will be updated so that the withdrawalIndex points to the next vault in the group.

/**
* @notice Deposits tokens into vaults
* @param _toDeposit amount to deposit
* @param _minDeposits minimum amount of deposits that a vault can hold
* @param _maxDeposits minimum amount of deposits that a vault can hold
* @param _vaultIds list of vaults to deposit into
*/
function _depositToVaults(
uint256 _toDeposit,
uint256 _minDeposits,
uint256 _maxDeposits,
uint64[] memory _vaultIds
) private returns (uint256) {
...
​
// deposit into vaults in the order specified in _vaultIds
for (uint256 i = 0; i < _vaultIds.length; ++i) {
uint256 vaultIndex = _vaultIds[i];
// vault must be a member of a group
if (vaultIndex >= globalState.depositIndex) revert InvalidVaultIds();
​
IVault vault = vaults[vaultIndex];
uint256 groupIndex = vaultIndex % globalState.numVaultGroups;
VaultGroup memory group = groups[groupIndex];
uint256 deposits = vault.getPrincipalDeposits();
uint256 canDeposit = _maxDeposits - deposits;
​
globalState.groupDepositIndex = uint64(vaultIndex);
​
// if vault is empty and equal to withdrawal index, increment withdrawal index to the next vault in the group
@> if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
@> group.withdrawalIndex += uint64(globalState.numVaultGroups);
@> if (group.withdrawalIndex > globalState.depositIndex) {
@> group.withdrawalIndex = uint64(groupIndex);
}
}
...

However, it is possible for the withdrawalIndex to become the depositIndex, meaning the next withdrawalIndex in the group could point to a vault that is not part of that group. This occurs because the withdrawalIndex only checks if the next index is greater than the depositIndex.

// if vault is empty and equal to withdrawal index, increment withdrawal index to the next vault in the group
if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
group.withdrawalIndex += uint64(globalState.numVaultGroups);
@> if (group.withdrawalIndex > globalState.depositIndex) {
group.withdrawalIndex = uint64(groupIndex);
}
}

With the group’s withdrawalIndex set to the groupIndex, it is possible for this group to become the curUnboundedVaultGroup, meaning withdrawals will be possible from this group.

function updateVaultGroups(
uint256[] calldata _curGroupVaultsToUnbond,
uint256 _curGroupTotalDepositRoom,
uint256 _nextGroup,
uint256 _nextGroupTotalUnbonded
) external onlyFundFlowController {
for (uint256 i = 0; i < _curGroupVaultsToUnbond.length; ++i) {
vaults[_curGroupVaultsToUnbond[i]].unbond();
}
​
vaultGroups[globalVaultState.curUnbondedVaultGroup].totalDepositRoom = uint128(
_curGroupTotalDepositRoom
);
@> globalVaultState.curUnbondedVaultGroup = uint64(_nextGroup);
totalUnbonded = _nextGroupTotalUnbonded;
}

With this group as the unbounded group, when a withdrawal is called inside VaultControllerStrategy, the depositIndex can be passed as the first vaultId. This index will be valid since it was previously set as the withdrawal index during a deposit.

/**
* @notice Withdraws tokens from vaults and sends them to staking pool
* @dev called by VaultControllerStrategy using delegatecall
* @param _amount amount to withdraw
* @param _data encoded vault withdrawal order
*/
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];
​
// withdrawals must continue with the vault they left off at during the previous call
@> if (vaultIds[0] != group.withdrawalIndex) revert InvalidVaultIds();

If the vault with the depositIndex has some deposits and an active claim period for withdrawal, the withdrawal will go through this vault.

for (uint256 i = 0; i < vaultIds.length; ++i) {
// vault must be a member of the current unbonded group
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) {
// cannot leave a vault with less than minimum deposits
vault.withdraw(deposits);
unbondedRemaining -= deposits;
break;
} else {
vault.withdraw(toWithdraw);
unbondedRemaining -= toWithdraw;
break;
}
}
}

The end result is that the totalDepositRoom will be increased by the withdrawn amount, even though part or all of that amount was withdrawn from a vault that is not part of that group. This will lead to inaccurate accounting, as the totalDepositRoom for this group will actually be lower than reported.

totalDeposits -= totalWithdrawn;
totalPrincipalDeposits -= totalWithdrawn;
totalUnbonded = unbondedRemaining;
​
@> group.totalDepositRoom += uint128(totalWithdrawn);
vaultGroups[globalVaultState.curUnbondedVaultGroup] = group;

Vulnerability Details

Vulnerable code: https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/linkStaking/base/VaultControllerStrategy.sol#L204

Consider having 17 vaults:

__. __. __. __. __. __. __. __. __. __. __. __. __. __. __. __. __
|__||__||__||__||__||__||__||__||__||__||__||__||__||__||__||__||__|
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16

Where first 16 vaults are parth of the 4 groups like this, and last vault is not part of any group.

_________ _________ _________ _________
| 0 | 4 | | 1 | 5 | | 2 | 6 | | 3 | 7 |
|____|____| |____|____| |____|____| |____|____|
| 8 | 12 | | 9 | 13 | |10 | 14 | |11 | 15 |
|____|____| |____|____| |____|____| |____|____| 16

Here, the 16th vault is the globalState.depositIndex.
During a deposit, if the 12th vault is part of the vaultIds and it is the withdrawalIndex for group 0, the withdrawal index will be set to index 16. This is a valid index for group 0 because 16 % numOfGroups = 0, but that vault is not part of the group.

if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
@> group.withdrawalIndex += uint64(globalState.numVaultGroups);
if (group.withdrawalIndex > globalState.depositIndex) {
group.withdrawalIndex = uint64(groupIndex);
}
}

Later during withdraw phase, if group 0 becomes curUnboundedVaultGroup this check will pass:

if (vaultIds[i] % globalState.numVaultGroups != globalState.curUnbondedVaultGroup)
revert InvalidVaultIds();

This means the withdrawal will be executed from the 16th vault if possible, and an incorrect amount will be added to the depositRoom of group 0.


Impact

Likelihood: Low

Several conditions needs to be met for this to happen, making this low likelihood:

  1. The vault with the withdrawal index needs to be empty during the deposit.

  2. There must be a valid groupIndex that satisfies the condition (withdrawalIndex + vault group size) = groupIndex.

  3. The group must become the curUnboundedVaultGroup.

  4. The groupIndex vault must not be empty and should be in an active claim period.

Impact: Medium

The total depositRoom will be inaccurately updated for the group, which will lead to false assumptions and break functionality if the withdrawal amount is greater than the actual depositRoom for that group.

Tools Used

Manual review.

Recommendations

When checking for withdrawal index, use >= instead of > . This will make sure that withdrawalIndex is alway part of the group.

// if vault is empty and equal to withdrawal index, increment withdrawal index to the next vault in the group
if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
group.withdrawalIndex += uint64(globalState.numVaultGroups);
- if (group.withdrawalIndex > globalState.depositIndex) {
+ if (group.withdrawalIndex >= globalState.depositIndex) {
group.withdrawalIndex = uint64(groupIndex);
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

The withdrawal index can be set to an index outside of the group, resulting in incorrect totalDepositRoom accounting

Support

FAQs

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