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

`_depositLiquidity` is called without reentrancy protection

0xbrett8571

Summary

The StakingPool contract is susceptible to a reentrancy attack due to the lack of reentrancy guards in the _depositLiquidity function. A malicious strategy contract can exploit this vulnerability to make reentrant calls back into the StakingPool.deposit function, potentially manipulating the contract's state, stealing funds, or causing unexpected behavior.

Vulnerability Details

This function is called by the deposit function, which is a critical function that allows users to stake their tokens and mint liquid staking tokens.

The vulnerability arises because _depositLiquidity directly calls strategy.deposit without any reentrancy guards. If a malicious strategy contract is added to the StakingPool, it can make a reentrant call back into the StakingPool.deposit function during the execution of _depositLiquidity. This reentrant call can potentially manipulate the contract's state, steal funds, or cause the original deposit function to revert unexpectedly.

  1. https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L111-L132

function deposit(
address _account,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool {
require(strategies.length > 0, "Must be > 0 strategies to stake");
​
uint256 startingBalance = token.balanceOf(address(this));
​
if (_amount > 0) {
token.safeTransferFrom(msg.sender, address(this), _amount);
// @audit: _depositLiquidity is called without reentrancy protection
_depositLiquidity(_data);
_mint(_account, _amount);
totalStaked += _amount;
} else {
// @audit: _depositLiquidity is called without reentrancy protection
_depositLiquidity(_data);
}
​
uint256 endingBalance = token.balanceOf(address(this));
if (endingBalance > startingBalance && endingBalance > unusedDepositLimit)
revert InvalidDeposit();
}

  1. https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L477-L492

function _depositLiquidity(bytes[] calldata _data) private {
uint256 toDeposit = token.balanceOf(address(this));
if (toDeposit > 0) {
for (uint256 i = 0; i < strategies.length; i++) {
IStrategy strategy = IStrategy(strategies[i]);
uint256 strategyCanDeposit = strategy.canDeposit();
if (strategyCanDeposit >= toDeposit) {
// @audit: strategy.deposit is called without reentrancy protection
strategy.deposit(toDeposit, _data[i]);
break;
} else if (strategyCanDeposit > 0) {
// @audit: strategy.deposit is called without reentrancy protection
strategy.deposit(strategyCanDeposit, _data[i]);
toDeposit -= strategyCanDeposit;
}
}
}
}

In both the deposit function and the _depositLiquidity function, where the calls to _depositLiquidity and strategy.deposit are made without reentrancy protection. These unprotected calls can be exploited by a malicious strategy contract to perform reentrant calls.

Here's how the bug happens:

  1. A user calls the deposit function with _amount = 0 and provides deposit data for a malicious strategy.

  2. Inside deposit, the _depositLiquidity function is called with the provided deposit data.

  3. _depositLiquidity iterates over the strategies and calls the deposit function of each strategy, passing the deposit data.

  4. If a malicious strategy is encountered, its deposit function can make a reentrant call back into the StakingPool.deposit function.

  5. The reentrant call to deposit can execute malicious code, potentially manipulating the contract's state or stealing funds.

  6. The reentrant call can also cause the original deposit function to revert, violating the noReentrancyInCriticalFunctions rule.

Impact

If a malicious strategy is added to the pool, it can exploit the reentrancy vulnerability to steal funds from users or manipulate the contract's state.

Tools Used

Manual Review

Recommendations

Implement reentrancy guards in the StakingPool contract. The deposit function and the other external functions that call _depositLiquidity should be protected with a reentrancy guard, such as the nonReentrant modifier from OpenZeppelin.

+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
-contract StakingPool is StakingRewardsPool {
+contract StakingPool is StakingRewardsPool, ReentrancyGuard {
// ...
​
function deposit(
address _account,
uint256 _amount,
bytes[] calldata _data
- ) external onlyPriorityPool {
+ ) external onlyPriorityPool nonReentrant {
// ...
}
​
// ...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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