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

Deposits with constantly revert in periods of often usage

Dup1337

Summary

Deposits require that _vaultIds[0] != globalState.groupDepositIndex, where _vaultIds is an array decoded from generic bytse calldata. However, because multiple users may interact with the contract during similar time, and globalState.groupDepositIndex updates with each iteration over _vaultIds in VaultControllerStrategy#_depositToVaults, during execution many of the requests will revert with globalState.groupDepositIndex mismatch.

Vulnerability Details

When calling deposit, the code finally lands in VaultControllerStrategy (delegatecall from VCS). Then the calldata that the user sent is decoded into which vaults to deposit into:

function _depositToVaults(
uint256 _toDeposit,
uint256 _minDeposits,
uint256 _maxDeposits,
@> uint64[] memory _vaultIds
) private returns (uint256) {
// [...]
​
// 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) {
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;
​
// @audit it increases with every user passed _vaultIds
@> globalState.groupDepositIndex = uint64(vaultIndex);
// [...]
@> globalVaultState = globalState;

It's enforced that the vault id of the array passed in calldata starts with the vault they left off at during the previous call - this will never work if it's used frequently. That's because of globalVaultState.groupDepositIndex will already be overriden between user posting a transaction, and the transaction being executed during high demand.

A malicious user can even go ahead and soft DoS it for prolonged period of time by creating muyltiple spaced deposits to a single vault. Then other people requests won't start with the id that the code expects.

Impact

DoS of the deposits in times of high demand/by malicious actor.

Tools Used

Manual review

Recommendations

Consider changing the code to automatically put the funds into appropriate vaults, and not rely on user calldata.

Updates

Lead Judging Commences

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

Support

FAQs

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