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

`performUpkeep` Does Not Always Update State Correctly Leading to Unexpected `checkUpkeep` Behavior

lin0x9a7a

Summary

The PriorityPool contract has a vulnerability where the performUpkeep function does not always update the contract state as expected. Due to how the _depositQueuedTokens function is implemented, there are cases where performUpkeep will revert without making any changes. This leads to a situation where checkUpkeep incorrectly returns true even after performUpkeep is called.

Vulnerability Details

In the PriorityPool contract, there is an issue with how the performUpkeep function interacts with the checkUpkeep function. The performUpkeep function is expected to make state changes such that immediately calling checkUpkeep afterwards should return false. However, there are certain conditions where performUpkeep may not actually update the state as expected.

The root cause lies in the _depositQueuedTokens internal function which performUpkeep relies on: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L693-L729

function _depositQueuedTokens(
uint256 _depositMin,
uint256 _depositMax,
bytes[] memory _data
) internal {
if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
​
uint256 strategyDepositRoom = stakingPool.getStrategyDepositRoom();
// @Vuln: This condition can cause the function to revert without making any state changes
if (strategyDepositRoom == 0 || strategyDepositRoom < _depositMin)
revert InsufficientDepositRoom();
​
uint256 _totalQueued = totalQueued;
uint256 unusedDeposits = stakingPool.getUnusedDeposits();
uint256 canDeposit = _totalQueued + unusedDeposits;
// @Vuln: This condition can also cause the function to revert without making any state changes
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);
}

If either the InsufficientDepositRoom or InsufficientQueuedTokens revert conditions are met, the function will exit without updating the totalQueued state variable or making any actual token deposits.

As a result, if performUpkeep is called when these conditions are true, it will not change the contract state in a way that guarantees checkUpkeep will return false immediately after. The checkUpkeep function may continue to return true unexpectedly.

// Assume contract is set up with proper initial state
​
// Call checkUpkeep, expect it to return true
bytes memory data;
(bool upkeepNeeded, bytes memory performData) = priorityPool.checkUpkeep(data);
assert(upkeepNeeded == true);
​
// Call performUpkeep
priorityPool.performUpkeep(performData);
​
// Call checkUpkeep again, expect it to return false, but it will still return true
(bool upkeepNeededAfter, ) = priorityPool.checkUpkeep(data);
assert(upkeepNeededAfter == false); // This assert will fail

Would need to ensure:

  1. strategyDepositRoom is less than queueDepositMin so the first revert condition is hit OR

  2. totalQueued + unusedDeposits is less than queueDepositMin so the second revert condition is hit

Under these conditions, performUpkeep will revert without making any state changes, so checkUpkeep will continue to return true erroneously.

Impact

If performUpkeep is called but does not perform any updates, it may waste gas fees for users. It could also stall expected token deposits from occurring if checkUpkeep continues to return true but performUpkeep keeps reverting.

Tools Used

Vs Code

Recommendations

Instead of immediately reverting, it could return a boolean indicating whether the deposit was successful or not. The performUpkeep function can then check this return value and only update the totalQueued state if the deposit actually occurred. All code paths need to result in checkUpkeep returning false if called again right after performUpkeep.

diff --git a/contracts/core/priorityPool/PriorityPool.sol b/contracts/core/priorityPool/PriorityPool.sol
index 1234567..8901234 100644
--- a/contracts/core/priorityPool/PriorityPool.sol
+++ b/contracts/core/priorityPool/PriorityPool.sol
@@ -500,7 +500,7 @@ contract PriorityPool is UUPSUpgradeable, OwnableUpgradeable, PausableUpgradeabl
* @param _depositMax max amount of tokens that can be deposited into at once
* @param _data deposit data passed to staking pool strategies
**/
- function _depositQueuedTokens(
+ function _depositQueuedTokens(
uint256 _depositMin,
uint256 _depositMax,
bytes[] memory _data
@@ -508,10 +508,10 @@ contract PriorityPool is UUPSUpgradeable, OwnableUpgradeable, PausableUpgradeabl
if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
​
uint256 strategyDepositRoom = stakingPool.getStrategyDepositRoom();
- if (strategyDepositRoom == 0 || strategyDepositRoom < _depositMin)
- revert InsufficientDepositRoom();
+ if (strategyDepositRoom == 0 || strategyDepositRoom < _depositMin)
+ return false;
​
uint256 _totalQueued = totalQueued;
uint256 unusedDeposits = stakingPool.getUnusedDeposits();
@@ -519,7 +519,7 @@ contract PriorityPool is UUPSUpgradeable, OwnableUpgradeable, PausableUpgradeabl
if (canDeposit == 0 || canDeposit < _depositMin) revert InsufficientQueuedTokens();
​
uint256 toDepositFromStakingPool = MathUpgradeable.min(
MathUpgradeable.min(unus
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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