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

Reentrancy Allows Manipulation of Accounting and Potential Theft of Funds

foxb868

Summary

The bug occurs due to a reentrancy vulnerability in the StakingPool contract's deposit() function.

It's happening because.

  • The deposit() function calls _depositLiquidity() which loops through a list of strategy contracts and calls their deposit() function with user-supplied calldata.

  • If one of these strategy contracts is malicious, it can reenter the StakingPool contract before the original deposit() call finishes executing.

  • The malicious strategy can call donateTokens() or other state-changing functions in StakingPool, modifying accounting variables like totalStaked.

  • When the original deposit() resumes, the _mint() call reverts because totalStaked is now out of sync with the actual token balances.

If exploited, users' balances could be manipulated, funds could be drained from the pool, or the contract could be put in an inconsistent state.

Vulnerability Details

The deposit() function calls the _depositLiquidity() internal function which loops through a list of strategy contracts and invokes their deposit() function with user-supplied calldata.

If one of these strategy contracts is malicious, it can reenter the StakingPool contract before the original deposit() call completes execution. The malicious strategy can then call donateTokens() or other state-changing functions in StakingPool, modifying accounting variables like totalStaked.

When the original deposit() call resumes, the _mint() call reverts because totalStaked is now inconsistent with the actual token balances due to the reentrancy.

Here's how a malicious user could exploit:

  1. Deploy a malicious strategy contract that reenters StakingPool when its deposit() function is called.

  2. Call StakingPool.donateTokens(0) to initialize totalStaked with a non-zero value.

  3. Call StakingPool.deposit(attacker, 0, maliciousData) where maliciousData contains calldata for invoking the malicious strategy.

  4. StakingPool._depositLiquidity() is invoked which calls deposit() on the malicious strategy.

  5. The malicious strategy reenters StakingPool, calling donateTokens(1) to increase totalStaked.

  6. The original deposit() call resumes but the _mint() call reverts because totalStaked is now out of sync.

StakingPool.sol:111-132: 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) {
// Calls into untrusted strategy contracts
token.safeTransferFrom(msg.sender, address(this), _amount);
_depositLiquidity(_data); // <-- @audit Vulnerable call to _depositLiquidity
_mint(_account, _amount); // Reverts due to reentrancy
totalStaked += _amount;
} else {
_depositLiquidity(_data); // <-- @audit Vulnerable call to _depositLiquidity
// Also vulnerable even if _amount is 0
}
​
uint256 endingBalance = token.balanceOf(address(this));
if (endingBalance > startingBalance && endingBalance > unusedDepositLimit)
revert InvalidDeposit();
}

StakingPool.sol:477-492: 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) {
// Untrusted external call
strategy.deposit(toDeposit, _data[i]); // <-- @audit Vulnerable external call
break;
} else if (strategyCanDeposit > 0) {
strategy.deposit(strategyCanDeposit, _data[i]); // <-- @audit Vulnerable external call
toDeposit -= strategyCanDeposit;
}
}
}
}

Impact

deposit() calls into untrusted strategy contracts before updating accounting variables like totalStaked

Tools Used

Vs

Recommendations

diff --git a/contracts/core/StakingPool.sol b/contracts/core/StakingPool.sol
index ...
--- a/contracts/core/StakingPool.sol
+++ b/contracts/core/StakingPool.sol
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.15;
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+
import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
import "./base/StakingRewardsPool.sol";
@@ -11,7 +13,7 @@ import "./interfaces/IStrategy.sol";
* @notice Allows users to stake asset tokens and receive liquid staking tokens 1:1, then deposits staked
* asset tokens into strategy contracts
*/
-contract StakingPool is StakingRewardsPool {
+contract StakingPool is StakingRewardsPool, ReentrancyGuard {
using SafeERC20Upgradeable for IERC20Upgradeable;
struct Fee {
@@ -119,7 +121,7 @@ contract StakingPool is StakingRewardsPool {
address _account,
uint256 _amount,
bytes[] calldata _data
- ) external onlyPriorityPool {
+ ) external onlyPriorityPool nonReentrant {
require(strategies.length > 0, "Must be > 0 strategies to stake");
uint256 startingBalance = token.balanceOf(address(this));
@@ -127,6 +129,7 @@ contract StakingPool is StakingRewardsPool {
if (_amount > 0) {
token.safeTransferFrom(msg.sender, address(this), _amount);
_depositLiquidity(_data);
+ totalStaked += _amount;
_mint(_account, _amount);
totalStaked += _amount;
} else {
​
Updates

Lead Judging Commences

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.