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

OperatorVCS::initialize - wrong initialization for GlobalVaultState.depositIndex

0xrs

Summary

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

The value for GlobalVaultState.depositIndex is initialized with the wrong value, which causes several function, like: queueVaultRemoval and _depositToVaults to behave in a wrong way.

Vulnerability Details

When the OperatorVCS contract is deployed, the initialize function will run automatically and the contract will be initialized to version 3 (because of the presence of the reinitializer(3) modifier). A version of the VaultControllerStrategy contract will already have been deployed previously, so, at this point, the token state variable will have a different value than address(0) and the else block in the code will be executed:

else {
globalVaultState = GlobalVaultState(5, 0, 0, uint64(maxDepositSizeBP + 1));
maxDepositSizeBP = _maxDepositSizeBP;
delete fundFlowController;
vaultMaxDeposits = _vaultMaxDeposits;
}

The problem here is that the globalVaultState.depositIndex will be set to the wrong value: uint64(maxDepositSizeBP + 1). maxDepositSizeBP will be a high value (something around 9000) and the number of vaults will be much lower than this value.

Impact

The globalVaultState.depositIndex is used in the queueVaultRemoval function:

function queueVaultRemoval(uint256 _index) external {
...
if (_index < globalVaultState.depositIndex) {
uint256 group = _index % globalVaultState.numVaultGroups;
uint256[] memory groups = new uint256[]();
groups[0] = group;
fundFlowController.updateOperatorVaultGroupAccounting(groups);
​
// if possiible, remove vault right away
if (vaults[_index].claimPeriodActive()) {
removeVault(vaultsToRemove.length - 1);
}
}

Because of the wrong initialization of the depositIndex, the specified _index will always be lower than the depositIndex, even for vaults that are not part of a vault group. This means, the fundFlowController will potentially update the internal accounting of a vault group that should not be updated. Also, because the specified vault _index may not be part of a group, a vault that should not be removed may be removed.

The globalVaultState.depositIndex is also used in the _depositToVaults function:

In the following code section, vaultIndex will never be bigger than the globalState.depositIndex, so, this will never revert, even if an invalid _vaultId is provided:

function _depositToVaults(uint256 _toDeposit, uint256 _minDeposits, uint256 _maxDeposits, uint64[] memory _vaultIds) {
...
for (uint256 i = 0; i < _vaultIds.length; ++i) {
uint256 vaultIndex = _vaultIds[i];
//this will not revert, even for invalid vaultIndex
if (vaultIndex >= globalState.depositIndex) revert InvalidVaultIds();
​

In the following code section, the group.withdrawalIndex will never be bigger than the globalState.depositIndex, which means, the group.withdrawalIndex may be set to the wrong value:

...
if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
group.withdrawalIndex += uint64(globalState.numVaultGroups);
if (group.withdrawalIndex > globalState.depositIndex) {
//this code will never be executed
group.withdrawalIndex = uint64(groupIndex);
}
}

In the following code section, the variable i will be set to an index thats far greater than the current number vaults. This means, the while loop will never be executed, it is not possible to deposit into the vault and the correct depositindex cannot be set

...
uint256 i = globalState.depositIndex;
​
//loop will never be executed
while (i < numVaults) {
IVault vault = vaults[i];
uint256 deposits = vault.getPrincipalDeposits();
uint256 canDeposit = _maxDeposits - deposits;
...
//deposit cannot be done
if (toDeposit > canDeposit) {
vault.deposit(canDeposit);
...
++i;
}
​
//correct depositIndex cannot be set
globalVaultState.depositIndex = uint64(i);
​

Tools Used

Manual Review

Recommendations

Replace the else block of the initialize function with the following code:

else {
+ uint64 currentDepositIndex = globalVaultState.depositIndex;
+ globalVaultState = GlobalVaultState(5, 0, 0, currentDepositIndex);
- globalVaultState = GlobalVaultState(5, 0, 0, uint64(maxDepositSizeBP + 1));
maxDepositSizeBP = _maxDepositSizeBP;
delete fundFlowController;
vaultMaxDeposits = _vaultMaxDeposits;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
11 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

[INVALID] Incorrect Initialization of `GlobalVaultState` in `CommunityVCS` Contract

Support

FAQs

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