Summary
function setQueueDepositParams(
uint128 _queueDepositMin,
uint128 _queueDepositMax
) external onlyOwner {
queueDepositMin = _queueDepositMin;
queueDepositMax = _queueDepositMax;
emit SetQueueDepositParams(_queueDepositMin, _queueDepositMax);
}
_queueDepositMin and _queueDepositMax are parameters that should only be modifiable by the owner.
* @notice Deposits queued and/or unused tokens into staking pool strategies
* @dev will revert if less than queueDepositMin tokens can be deposited
* @param _performData encoded list of deposit data to be passed to staking pool strategies (bytes[])
*/
function performUpkeep(bytes calldata _performData) external {
bytes[] memory depositData = abi.decode(_performData, (bytes[]));
_depositQueuedTokens(queueDepositMin, queueDepositMax, depositData);
}
_queueDepositMin and _queueDepositMax are parameters that should only be modifiable by the owner. They are used in the _depositQueuedTokens function as conditions for boundary checks.
function depositQueuedTokens(
uint256 _queueDepositMin,
uint256 _queueDepositMax,
bytes[] calldata _data
) external {
_depositQueuedTokens(_queueDepositMin, _queueDepositMax, _data);
}
However, depositQueuedTokens breaks this security assumption. This function allows users to call it with arbitrary _queueDepositMin and _queueDepositMax values.
Vulnerability Details
function _depositQueuedTokens(
uint256 _depositMin,
uint256 _depositMax,
bytes[] memory _data
) internal {
if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
uint256 strategyDepositRoom = stakingPool.getStrategyDepositRoom();
if (strategyDepositRoom == 0 || strategyDepositRoom < _depositMin)
revert InsufficientDepositRoom();
uint256 _totalQueued = totalQueued;
uint256 unusedDeposits = stakingPool.getUnusedDeposits();
uint256 canDeposit = _totalQueued + unusedDeposits;
if (canDeposit == 0 || canDeposit < _depositMin) revert InsufficientQueuedTokens();
uint256 toDepositFromStakingPool = MathUpgradeable.min(
MathUpgradeable.min(unusedDeposits, strategyDepositRoom),
_depositMax
);
uint256 toDepositFromQueue = MathUpgradeable.min(
MathUpgradeable.min(_totalQueued, strategyDepositRoom - toDepositFromStakingPool),
_depositMax - toDepositFromStakingPool
);
stakingPool.deposit(address(this), toDepositFromQueue, _data);
_totalQueued -= toDepositFromQueue;
if (_totalQueued != totalQueued) {
uint256 diff = totalQueued - _totalQueued;
depositsSinceLastUpdate += diff;
sharesSinceLastUpdate += stakingPool.getSharesByStake(diff);
totalQueued = _totalQueued;
}
emit DepositTokens(toDepositFromStakingPool, toDepositFromQueue);
}
Secondly, when _queueDepositMin is set to 1 or lower, an appropriately chosen _queueDepositMax can ultimately result in a diff value of 1. This causes getSharesByStake(1) to return 0, leading to a scenario where sharesSinceLastUpdate remains unchanged while depositsSinceLastUpdate increases by 1.
This mismatch between depositsSinceLastUpdate and sharesSinceLastUpdate results in an incorrect internal state of the contract.
Impact
Bypasses the security assumption
Contract records an incorrect state between depositsSinceLastUpdate and sharesSinceLastUpdate
Recommendations
If this interface is mandatory, basic checks on the parameters are necessary.
