Summary
VaultControllerStrategy's deposit function does not properly validate the provided vault IDs, allowing users to manipulate the vault IDs to deposit repeatedly into the same vault.
This can result in a single vault receiving more than its intended deposit value, disrupting the intended distribution of deposits across vaults, causing reward dilution, impacting the unbonding process, and ultimately breaking the intended composition and operation of the vault system.
Vulnerability Details
The issue arises because the deposit logic in the VaultControllerStrategy contract does not adequately validate the vault IDs provided by the user.
Specifically, it does not check for duplicate vault IDs. As a result, a user can craft input data to deposit repeatedly into the same vault causing an imbalance in the vault distribution.
The user calls depositQueuedTokens, providing _data, which contains a list of vault IDs. This function is intended to deposit queued tokens into the vaults:
Contract: PriorityPool.sol
390:
391: * @notice Deposits queued tokens and/or unused tokens sitting in staking pool
392: * @dev allows bypassing of the stored deposit limits
393: * @param _queueDepositMin min amount of tokens required for deposit into staking pool strategies
394: * @param _queueDepositMax max amount of tokens that can be deposited into staking pool strategies at once
395: * @param _data list of deposit data passed to staking pool strategies
396: */
397: function depositQueuedTokens(
398: uint256 _queueDepositMin,
399: uint256 _queueDepositMax,
400: bytes[] calldata _data
401: ) external {
402: >>> _depositQueuedTokens(_queueDepositMin, _queueDepositMax, _data);
403: }
The _depositQueuedTokens function eventually calls deposit on the stakingPool, passing along the _data containing vault IDs:
Contract: PriorityPool.sol
693: function _depositQueuedTokens(
694: uint256 _depositMin,
695: uint256 _depositMax,
696: bytes[] memory _data
697: ) internal {
698: if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
699:
700: uint256 strategyDepositRoom = stakingPool.getStrategyDepositRoom();
701: if (strategyDepositRoom == 0 || strategyDepositRoom < _depositMin)
702: revert InsufficientDepositRoom();
703:
704: uint256 _totalQueued = totalQueued;
705: uint256 unusedDeposits = stakingPool.getUnusedDeposits();
706: uint256 canDeposit = _totalQueued + unusedDeposits;
707: if (canDeposit == 0 || canDeposit < _depositMin) revert InsufficientQueuedTokens();
708:
709: uint256 toDepositFromStakingPool = MathUpgradeable.min(
710: MathUpgradeable.min(unusedDeposits, strategyDepositRoom),
711: _depositMax
712: );
713: uint256 toDepositFromQueue = MathUpgradeable.min(
714: MathUpgradeable.min(_totalQueued, strategyDepositRoom - toDepositFromStakingPool),
715: _depositMax - toDepositFromStakingPool
716: );
717:
718: >>> stakingPool.deposit(address(this), toDepositFromQueue, _data);
719: _totalQueued -= toDepositFromQueue;
720:
721: if (_totalQueued != totalQueued) {
722: uint256 diff = totalQueued - _totalQueued;
723: depositsSinceLastUpdate += diff;
724: sharesSinceLastUpdate += stakingPool.getSharesByStake(diff);
725: totalQueued = _totalQueued;
726: }
727:
728: emit DepositTokens(toDepositFromStakingPool, toDepositFromQueue);
729: }
The deposit function in StakingPool calls _depositLiquidity, passing the _data (vault IDs):
Contract: StakingPool.sol
111: function deposit(
112: address _account,
113: uint256 _amount,
114: bytes[] calldata _data
115: ) external onlyPriorityPool {
116: require(strategies.length > 0, "Must be > 0 strategies to stake");
117:
118: uint256 startingBalance = token.balanceOf(address(this));
119:
120: if (_amount > 0) {
121: token.safeTransferFrom(msg.sender, address(this), _amount);
122: >>> _depositLiquidity(_data);
123: _mint(_account, _amount);
124: totalStaked += _amount;
125: } else {
126: >>> _depositLiquidity(_data);
127: }
128:
129: uint256 endingBalance = token.balanceOf(address(this));
130: if (endingBalance > startingBalance && endingBalance > unusedDepositLimit)
131: revert InvalidDeposit();
132: }
The _depositLiquidity function loops over the strategies and calls deposit on each, passing the corresponding _data[i];
Contract: StakingPool.sol
477: function _depositLiquidity(bytes[] calldata _data) private {
478: uint256 toDeposit = token.balanceOf(address(this));
479: if (toDeposit > 0) {
480: for (uint256 i = 0; i < strategies.length; i++) {
481: IStrategy strategy = IStrategy(strategies[i]);
482: uint256 strategyCanDeposit = strategy.canDeposit();
483: if (strategyCanDeposit >= toDeposit) {
484: >>> strategy.deposit(toDeposit, _data[i]);
485: break;
486: } else if (strategyCanDeposit > 0) {
487: >>> strategy.deposit(strategyCanDeposit, _data[i]);
488: toDeposit -= strategyCanDeposit;
489: }
490: }
491: }
492: }
The deposit function in VaultControllerStrategy delegates the call to vaultDepositController, which contains the actual deposit logic:
Contract: VaultControllerStrategy.sol
437: function deposit(uint256 _amount, bytes calldata _data) external virtual onlyStakingPool {
438: if (vaultDepositController == address(0)) revert VaultDepositControllerNotSet();
439:
440: (bool success, ) = vaultDepositController.delegatecall(
441: abi.encodeWithSelector(VaultDepositController.deposit.selector, _amount, _data)
442: );
443:
444: if (!success) revert DepositFailed();
445: }
The deposit function decodes the _data to get the vaultIds and calls _depositToVaults:
Contract: VaultControllerStrategy.sol
87: function deposit(uint256 _amount, bytes calldata _data) external {
88: token.safeTransferFrom(msg.sender, address(this), _amount);
89:
90: (uint256 minDeposits, uint256 maxDeposits) = getVaultDepositLimits();
91:
92: uint256 toDeposit = token.balanceOf(address(this));
93: uint64[] memory vaultIds = abi.decode(_data, (uint64[]));
94:
95: uint256 deposited = _depositToVaults(toDeposit, minDeposits, maxDeposits, vaultIds);
96:
97: totalDeposits += deposited;
98: totalPrincipalDeposits += deposited;
99:
100: if (deposited < toDeposit) {
101: token.safeTransfer(address(stakingPool), toDeposit - deposited);
102: }
103: }
The _depositToVaults function iterates over the provided vaultIds and deposits tokens into each vault. However, it does not adequately validate the vault IDs beyond checking that they are within the depositIndex range:
Contract: VaultControllerStrategy.sol
172: function _depositToVaults(
173: uint256 _toDeposit,
174: uint256 _minDeposits,
175: uint256 _maxDeposits,
176: uint64[] memory _vaultIds
177: ) private returns (uint256) {
178: uint256 toDeposit = _toDeposit;
179: uint256 totalRebonded;
180: GlobalVaultState memory globalState = globalVaultState;
181: VaultGroup[] memory groups = vaultGroups;
182:
183:
184: if (_vaultIds.length != 0 && _vaultIds[0] != globalState.groupDepositIndex)
185: revert InvalidVaultIds();
186:
187:
188: for (uint256 i = 0; i < _vaultIds.length; ++i) {
189: uint256 vaultIndex = _vaultIds[i];
190:
191: if (vaultIndex >= globalState.depositIndex) revert InvalidVaultIds();
192:
193: IVault vault = vaults[vaultIndex];
194: uint256 groupIndex = vaultIndex % globalState.numVaultGroups;
195: VaultGroup memory group = groups[groupIndex];
196: uint256 deposits = vault.getPrincipalDeposits();
197: uint256 canDeposit = _maxDeposits - deposits;
198:
199: globalState.groupDepositIndex = uint64(vaultIndex);
200:
201:
202: if (deposits == 0 && vaultIndex == group.withdrawalIndex) {
203: group.withdrawalIndex += uint64(globalState.numVaultGroups);
204: if (group.withdrawalIndex > globalState.depositIndex) {
205: group.withdrawalIndex = uint64(groupIndex);
206: }
207: }
208:
209: if (canDeposit != 0 && vaultIndex != group.withdrawalIndex && !vault.isRemoved()) {
210: if (deposits < _minDeposits && toDeposit < (_minDeposits - deposits)) {
211: break;
212: }
213:
214:
215: if (vault.claimPeriodActive()) {
216: totalRebonded += deposits;
217: }
218:
219: if (toDeposit > canDeposit) {
220: vault.deposit(canDeposit);
221: toDeposit -= canDeposit;
222: group.totalDepositRoom -= uint128(canDeposit);
223: } else {
224: vault.deposit(toDeposit);
225: group.totalDepositRoom -= uint128(toDeposit);
226: toDeposit = 0;
227: break;
228: }
229: }
230: }
231:
232: globalVaultState = globalState;
233:
234:
235: for (uint256 i = 0; i < globalState.numVaultGroups; ++i) {
236: VaultGroup memory group = vaultGroups[i];
237: if (
238: group.withdrawalIndex != groups[i].withdrawalIndex ||
239: group.totalDepositRoom != groups[i].totalDepositRoom
240: ) {
241: vaultGroups[i] = groups[i];
242: }
243: }
244:
245: if (totalRebonded != 0) totalUnbonded -= totalRebonded;
246: if (toDeposit == 0 || toDeposit < _minDeposits) return _toDeposit - toDeposit;
247:
248:
249: for (uint256 i = 0; i < globalState.numVaultGroups; ++i) {
250: if (
251: (i != globalState.curUnbondedVaultGroup &&
252: groups[i].totalDepositRoom >= _maxDeposits) ||
253: (i == globalState.curUnbondedVaultGroup &&
254: groups[i].totalDepositRoom >= 2 * _maxDeposits)
255: ) {
256: return _toDeposit - toDeposit;
257: }
258: }
259:
260:
261: uint256 numVaults = vaults.length;
262: uint256 i = globalState.depositIndex;
263:
264: while (i < numVaults) {
265: IVault vault = vaults[i];
266: uint256 deposits = vault.getPrincipalDeposits();
267: uint256 canDeposit = _maxDeposits - deposits;
268:
269:
270: if (deposits < _minDeposits && toDeposit < (_minDeposits - deposits)) {
271: break;
272: }
273:
274: if (toDeposit > canDeposit) {
275: vault.deposit(canDeposit);
276: toDeposit -= canDeposit;
277: } else {
278: vault.deposit(toDeposit);
279: if (toDeposit < canDeposit) {
280: toDeposit = 0;
281: break;
282: }
283: toDeposit = 0;
284: }
285:
286: ++i;
287: }
288:
289: globalVaultState.depositIndex = uint64(i);
290:
291: return _toDeposit - toDeposit;
292: }
As can be seen on the function flow above, before depositing into the vaults, the function does not check for duplicate vault IDs.
This will end with the function depositing into the same vault repeatedly.
This breaks the mechanism of fund distribution whereas the CL Staking rewards to be dilluted for the concerned vault. Subsequent vaults may receive less in rewards (deposit change) because earlier vaults received more than intended.
And, the unbonding mechanism relies on vault groups being unbonded in a sequence. Overloading a single vault will disrupt the unbonding schedule due to lack of funds at the other vaults.
The overall composition and operation of the vault system becomes broken.
Impact
Dillution in rewards, unbonding trouble, breaking the compostion of the operations
Tools Used
Manual Review
Recommendations
Ensure that each vault ID in _data is unique
