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

Reentrancy in `StakingPool.sol` Contract

chainsentryaudits

Summary

A reentrancy vulnerability exists within the deposit and withdraw functions of the StakingPool.sol contract. These functions perform external calls to untrusted strategy contracts without adequate reentrancy protection, potentially allowing attackers to manipulate balances and withdraw more funds than intended.

Vulnerability Details

Vulnerable deposit Function

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);
_depositLiquidity(_data);
_mint(_account, _amount);
totalStaked += _amount;
} else {
_depositLiquidity(_data);
}
​
uint256 endingBalance = token.balanceOf(address(this));
if (endingBalance > startingBalance && endingBalance > unusedDepositLimit)
revert InvalidDeposit();
}

Explanation:

  • The deposit function transfers tokens and interacts with external strategy contracts (_depositLiquidity) before updating the totalStaked state variable.

  • This order allows external contracts to call back into the deposit or withdraw functions before totalStaked is updated, enabling potential reentrancy attacks.

Vulnerable withdraw Function

function withdraw(
address _account,
address _receiver,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool {
uint256 toWithdraw = _amount;
if (_amount == type(uint256).max) {
toWithdraw = balanceOf(_account);
}
​
uint256 balance = token.balanceOf(address(this));
if (toWithdraw > balance) {
_withdrawLiquidity(toWithdraw - balance, _data);
}
require(
token.balanceOf(address(this)) >= toWithdraw,
"Not enough liquidity available to withdraw"
);
​
_burn(_account, toWithdraw);
totalStaked -= toWithdraw;
token.safeTransfer(_receiver, toWithdraw);
}

Explanation:

  • Similar to deposit, the withdraw function performs external calls (_withdrawLiquidity) before updating the totalStaked variable.

  • This arrangement permits malicious strategies to re-enter the withdraw function, potentially draining more funds than allowed.

Malicious Strategy Contract

contract ReentrantStrategy is IStrategy {
StakingPool public stakingPool;
​
constructor(address _stakingPool) {
stakingPool = StakingPool(_stakingPool);
}
​
function deposit(uint256 _amount, bytes calldata) external override {
// Re-enter the staking pool's deposit function
stakingPool.deposit(msg.sender, _amount, new bytes[](0));
}
​
// Other required functions...
}

Explanation:

  • The ReentrantStrategy contract implements the IStrategy interface.

  • Its deposit function maliciously re-enters the StakingPool's deposit method during its execution.

  • This re-entrance occurs before the totalStaked variable is updated, allowing the attacker to manipulate contract state and potentially withdraw excessive funds.

Impact

  • Financial Losses: Attackers can exploit the reentrancy vulnerability to withdraw more tokens than intended, leading to significant financial losses for users and the platform.

  • State Manipulation: Unauthorized manipulation of state variables like totalStaked can disrupt the integrity of the staking mechanism, causing inconsistencies and potential overflows.

  • Trust Erosion: Successful exploitation can undermine user trust in the platform's security, potentially affecting its reputation and user base.

Tools Used

Manual Review

Recommendations

1. Implement Reentrancy Guards

Action:

Integrate OpenZeppelin's ReentrancyGuard and apply the nonReentrant modifier to critical functions.

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract StakingPool is StakingRewardsPool, ReentrancyGuard {
// ...
​
function deposit(
address _account,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool nonReentrant {
// Function body
}
​
function withdraw(
address _account,
address _receiver,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool nonReentrant {
// Function body
}
​
// Similarly, apply to internal functions if they are exposed to external calls
}

Explanation:

  • The nonReentrant modifier prevents functions from being called recursively, effectively mitigating reentrancy attacks.

2. Adhere to Checks-Effects-Interactions Pattern

Action:

Rearrange the order of operations to update state variables before making external calls.

function deposit(
address _account,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool nonReentrant {
require(strategies.length > 0, "Must be > 0 strategies to stake");
​
if (_amount > 0) {
_mint(_account, _amount);
totalStaked += _amount;
​
token.safeTransferFrom(msg.sender, address(this), _amount);
_depositLiquidity(_data);
} else {
_depositLiquidity(_data);
}
​
uint256 endingBalance = token.balanceOf(address(this));
if (endingBalance > startingBalance && endingBalance > unusedDepositLimit)
revert InvalidDeposit();
​
emit Deposit(_account, _amount - toDeposit, _shouldQueue ? toDeposit : 0);
}

Explanation:

  • By updating totalStaked before external interactions, the contract minimizes the risk window where reentrancy could be exploited.

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.