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

Lack of strategy existence checks before withdrawal cause the contract to interact with non-existent or removed strategies, leading to transaction failures and potential DoS scenarios in `StakingPool.sol`

safdie

Summary

The contract has a vulnerability where it fails to check whether a strategy exists before attempting to withdraw liquidity from it. This can cause the contract to interact with non-existent or removed strategies, leading to transaction failures and potential denial of service (DoS) scenarios.

Vulnerability Details

In the _withdrawLiquidity function, the contract iterates over the strategies array in reverse order, attempting to withdraw liquidity from each strategy. However, it doesn't verify whether the strategy at each index still exists or is valid before calling the withdraw() function on it. If a strategy has been removed or is invalid, the contract will try to withdraw from a non-existent strategy, causing the transaction to revert. This vulnerability can lead to a denial of service (DoS), preventing users from withdrawing their liquidity.
Code Snippet from _withdrawLiquidity:

function _withdrawLiquidity(uint256 _amount, bytes[] calldata _data) private {
uint256 toWithdraw = _amount;
​
for (uint256 i = strategies.length; i > 0; i--) {
IStrategy strategy = IStrategy(strategies[i - 1]);
uint256 strategyCanWithdraw = strategy.canWithdraw();
​
if (strategyCanWithdraw >= toWithdraw) {
strategy.withdraw(toWithdraw, _data[i - 1]);
break;
} else if (strategyCanWithdraw > 0) {
strategy.withdraw(strategyCanWithdraw, _data[i - 1]);
toWithdraw -= strategyCanWithdraw;
}
}
}

The function does not perform a validity check on each strategy address before calling its methods.

PoC:
Consider a scenario where the strategies array initially holds valid strategy contracts. Later, due to changes in governance or contract upgrades, one or more strategies are removed from the pool, and their entries in the strategies array are deleted. The contract then attempts to withdraw liquidity without checking whether the strategy exists. This can cause the withdrawal process to revert, locking users’ liquidity.

In Hardhat, you can simulate the vulnerability with this test:

const { expect } = require("chai");
​
describe("Strategy Existence Test", function () {
let owner, addr1, StrategyContract, strategy1, strategy2, pool;
​
beforeEach(async function () {
[owner, addr1] = await ethers.getSigners();
StrategyContract = await ethers.getContractFactory("MockStrategy");
​
// Deploy two strategies
strategy1 = await StrategyContract.deploy(true);
strategy2 = await StrategyContract.deploy(false);
​
// Deploy the pool contract
const PoolContract = await ethers.getContractFactory("Pool");
pool = await PoolContract.deploy();
​
// Add strategies to the pool
await pool.addStrategy(strategy1.address);
await pool.addStrategy(strategy2.address);
​
// Remove the second strategy (simulate deletion)
await pool.removeStrategy(1);
});
​
it("Should revert when withdrawing from a removed strategy", async function () {
await expect(pool.withdrawLiquidity(1000, [])).to.be.revertedWith(
"Invalid strategy"
);
});
});

This test shows that the withdrawLiquidity function will revert because the contract tries to interact with the removed strategy, which no longer exists.

Impact

  1. If a strategy has been removed from the pool, attempting to withdraw liquidity will fail due to an invalid strategy reference. This leads to transactions reverting, effectively blocking users from withdrawing their funds.

  2. Repeated failures to withdraw from invalid strategies could cause a denial of service, where the liquidity pool cannot function properly, impacting user trust and liquidity.

Tools Used

Manual review.

Recommendations

To mitigate this vulnerability, add a check to ensure that a strategy exists and is valid before attempting to interact with it. One way to achieve this is by modifying the _withdrawLiquidity function to check for valid strategy references before invoking any methods.

Fix:

function _withdrawLiquidity(uint256 _amount, bytes[] calldata _data) private {
uint256 toWithdraw = _amount;
​
for (uint256 i = strategies.length; i > 0; i--) {
address strategyAddress = strategies[i - 1];
​
// Check if the strategy address is valid
if (strategyAddress == address(0)) {
continue; // Skip if the strategy has been removed
}
​
IStrategy strategy = IStrategy(strategyAddress);
uint256 strategyCanWithdraw = strategy.canWithdraw();
​
if (strategyCanWithdraw >= toWithdraw) {
strategy.withdraw(toWithdraw, _data[i - 1]);
break;
} else if (strategyCanWithdraw > 0) {
strategy.withdraw(strategyCanWithdraw, _data[i - 1]);
toWithdraw -= strategyCanWithdraw;
}
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
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.