[H-1] Reentrancy Vulnerability in PriorityPool._deposit and StakingPool.deposit Functions

Github

• https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L627

• https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L124

Summary

There is a potential reentrancy vulnerability in the PriorityPool._deposit and StakingPool.deposit functions of the staking pool system. This vulnerability arises due to delayed state updates (toDeposit in PriorityPool and totalStaked in StakingPool) after external contract calls, specifically the call to stakingPool.deposit and _mint, respectively. If exploited, this vulnerability could lead to multiple deposits or minting of liquid staking tokens for the same tokens, causing inconsistencies in the contract's state and token balances.

Vulnerability Details

PriorityPool _deposit Function

• The function makes an external call to stakingPool.deposit to deposit tokens into the staking pool. However, the toDeposit variable (which tracks the amount to be deposited) is updated after the call to stakingPool.deposit.

• This external call opens up the possibility of reentrancy, where the stakingPool.deposit function (or any contract it interacts with, such as strategies) could call back into the PriorityPool contract before the toDeposit variable is updated.

StakingPool deposit Function:

• In the StakingPool.deposit function, after tokens are transferred and liquidity is distributed to strategies via the _depositLiquidity function, the _mint function is called to mint liquid staking tokens to the user. The totalStaked variable, which tracks the total number of staked tokens, is only updated after the _mint function.

• This opens the possibility for reentrancy via the _mint function or any external contracts involved, potentially allowing multiple token minting or incorrect state updates before totalStaked is modified.

In both functions, the critical state variables (toDeposit and totalStaked) are updated after making external calls to contracts like stakingPool and _mint. And if the external contracts (like strategies) or fallback functions in these contracts reenter into the contract (either PriorityPool or StakingPool), they could trigger another deposit or mint operation before the state is correctly updated.

Impact

• The attacker could deposit the same tokens multiple times before the toDeposit variable is correctly updated. This would lead to inflated staking amounts and incorrect accounting of tokens in the staking pool.

• In the StakingPool.deposit function, the attacker could exploit the delayed update of totalStaked to mint more staking tokens than they are entitled to, leading to an inflation of staking tokens and incorrect distribution of rewards.

• The reentrancy could result in inconsistencies in key state variables such as totalQueued in the PriorityPool and totalStaked in the StakingPool. This could destabilize the staking system, affecting reward calculations and leading to the misallocation of tokens.

• The ability to mint or deposit more tokens than actually exist would lead to economic loss for other users or the protocol itself, as the attacker would have an unfair advantage in the staking system.

Tools Used

Manual Review

Recommendations

• Apply the nonReentrant modifier (from OpenZeppelin’s ReentrancyGuard) to both the PriorityPool._deposit and StakingPool.deposit functions to ensure that reentrancy is prevented. This will block any recursive calls that could exploit delayed state updates.

• In the PriorityPool._deposit function, update the toDeposit variable before making the external call to stakingPool.deposit.

• In the StakingPool.deposit function, update the totalStaked variable before calling the _mint function. This will ensure that the contract's state is accurate before any external interactions occur.