Summary
In the SDLPoolSecondary contract the stake deposits to the same lockId are going to be queued using the function SDLPoolSecondary::_queueLockUpdate, then once the L2 chain is updated it is possible to execute the queued operations using the SDLPoolSecondary::_executeQueuedLockUpdates. The problem is that the user lockId updating is not limited, affecting the user because it may necessary to execute a large array of queues.
Vulnerability Details
The user can send tokens to the SDLPoolSecondary contract specifying the lockId, then the _queueLockUpdate function will be executed, the SDLPoolSecondary::_queueLockUpdate function push the update code line 436 to an array:
File: SDLPoolSecondary.sol
428: function _queueLockUpdate(
429: address _owner,
430: uint256 _lockId,
431: uint256 _amount,
432: uint64 _lockingDuration
433: ) internal onlyLockOwner(_lockId, _owner) {
434: Lock memory lock = _getQueuedLockState(_lockId);
435: LockUpdate memory lockUpdate = LockUpdate(updateBatchIndex, _updateLock(lock, _amount, _lockingDuration));
436: queuedLockUpdates[_lockId].push(lockUpdate);
437: queuedRESDLSupplyChange +=
438: int256(lockUpdate.lock.amount + lockUpdate.lock.boostAmount) -
439: int256(lock.amount + lock.boostAmount);
440: if (updateNeeded == 0) updateNeeded = 1;
441:
442: emit QueueUpdateLock(_owner, _lockId, lockUpdate.lock.amount, lockUpdate.lock.boostAmount, lockUpdate.lock.duration);
443: }
Once the data is updated accordly to the data from the primary chain (mainnet), it is possible to execute the queue operations using the SDLPoolSecondary::_executeQueuedLockUpdates function. The problem is that the function iterates over all the user queue updates code line 461:
File: SDLPoolSecondary.sol
451: function _executeQueuedLockUpdates(address _owner, uint256[] memory _lockIds) internal updateRewards(_owner) {
...
...
454: for (uint256 i = 0; i < _lockIds.length; ++i) {
455: uint256 lockId = _lockIds[i];
456: _onlyLockOwner(lockId, _owner);
457: uint256 numUpdates = queuedLockUpdates[lockId].length;
458:
459: Lock memory curLockState = locks[lockId];
460: uint256 j = 0;
461: while (j < numUpdates) {
...
...
499: }
...
...
509: }
510: }
That could be a problem if the user send multiple updates to the same lockId because the _executeQueuedLockUpdates function will consume more gas.
Impact
The SDLPoolSecondary::_executeQueuedLockUpdates execution may cost more gas than expected.
Tools used
Manual review
Recommendations
When the user mints new lockId, it has a validation that verifies code line 368 the user has not exceed a limit:
File: SDLPoolSecondary.sol
363: function _queueNewLock(
364: address _owner,
365: uint256 _amount,
366: uint64 _lockingDuration
367: ) internal {
368: if (newLocksByOwner[_owner].length >= queuedNewLockLimit) revert TooManyQueuedLocks();
...
...
The same validation should be added when the user updates a lockId:
// File: SDLPoolSecondary.sol
function _queueLockUpdate(
address _owner,
uint256 _lockId,
uint256 _amount,
uint64 _lockingDuration
) internal onlyLockOwner(_lockId, _owner) {
++ if (updateLocksByOwner[_owner].length >= queuedUpdatesLockLimit) revert TooManyQueuedLocks();
Lock memory lock = _getQueuedLockState(_lockId);
LockUpdate memory lockUpdate = LockUpdate(updateBatchIndex, _updateLock(lock, _amount, _lockingDuration));
queuedLockUpdates[_lockId].push(lockUpdate);
queuedRESDLSupplyChange +=
int256(lockUpdate.lock.amount + lockUpdate.lock.boostAmount) -
int256(lock.amount + lock.boostAmount);
if (updateNeeded == 0) updateNeeded = 1;
emit QueueUpdateLock(_owner, _lockId, lockUpdate.lock.amount, lockUpdate.lock.boostAmount, lockUpdate.lock.duration);
}