Summary
VaultDepositController::_depositToVaults() doesn't update the vaultGroups storage variable.
Vulnerability Details
VaultDepositController::_depositToVaults() is the function that is ultimately used to deposit asset tokens into vaults. It changes the global state of the vault, and so the vaultGroups storage variable since the vault is being modifying by the function. the globalVaultState storage variable is correctly updated, but vaultGroups doesn't change.
File: contracts/linkStaking/base/VaultControllerStrategy.sol#L172-L292
function _depositToVaults(
uint256 _toDeposit,
uint256 _minDeposits,
uint256 _maxDeposits,
uint64[] memory _vaultIds
) private returns (uint256) {
uint256 toDeposit = _toDeposit;
uint256 totalRebonded;
GlobalVaultState memory globalState = globalVaultState;
>> VaultGroup[] memory groups = vaultGroups;
if (_vaultIds.length != 0 && _vaultIds[0] != globalState.groupDepositIndex)
revert InvalidVaultIds();
for (uint256 i = 0; i < _vaultIds.length; ++i) {
>> VaultGroup memory group = groups[groupIndex];
uint256 deposits = vault.getPrincipalDeposits();
uint256 canDeposit = _maxDeposits - deposits;
globalState.groupDepositIndex = uint64(vaultIndex);
if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
>> group.withdrawalIndex += uint64(globalState.numVaultGroups);
if (group.withdrawalIndex > globalState.depositIndex) {
>> group.withdrawalIndex = uint64(groupIndex);
}
}
if (canDeposit != 0 && vaultIndex != group.withdrawalIndex && !vault.isRemoved()) {
if (toDeposit > canDeposit) {
vault.deposit(canDeposit);
toDeposit -= canDeposit;
>> group.totalDepositRoom -= uint128(canDeposit);
} else {
vault.deposit(toDeposit);
>> group.totalDepositRoom -= uint128(toDeposit);
toDeposit = 0;
break;
}
}
}
globalVaultState = globalState;
}
What is being updated is the loop's memory variable used in the function but the final state of the vault groups is not updated after. Therefore, the changes made to the groups will not remain beyond the loop.
Impact
The vault groups updates group will be lost after the loop since the changes were made in memory variables and not in the contract's persistent storage.
Tools Used
Manual review.
Recommendations
Consider the following changes.
function _depositToVaults(
uint256 _toDeposit,
uint256 _minDeposits,
uint256 _maxDeposits,
uint64[] memory _vaultIds
) private returns (uint256) {
uint256 toDeposit = _toDeposit;
uint256 totalRebonded;
GlobalVaultState memory globalState = globalVaultState;
VaultGroup[] memory groups = vaultGroups;
// deposits must continue with the vault they left off at during the previous call
if (_vaultIds.length != 0 && _vaultIds[0] != globalState.groupDepositIndex)
revert InvalidVaultIds();
// deposit into vaults in the order specified in _vaultIds
for (uint256 i = 0; i < _vaultIds.length; ++i) {
/** Lines of code **/
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);
}
}
if (canDeposit != 0 && vaultIndex != group.withdrawalIndex && !vault.isRemoved()) {
/** Lines of code **/
if (toDeposit > canDeposit) {
vault.deposit(canDeposit);
toDeposit -= canDeposit;
group.totalDepositRoom -= uint128(canDeposit);
} else {
vault.deposit(toDeposit);
group.totalDepositRoom -= uint128(toDeposit);
toDeposit = 0;
break;
}
}
++ groups[groupIndex] = group;
}
globalVaultState = globalState;
++ vaultGroups = groups;
/** Rest of the function **/
}
