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

unable to remove strategies with token balance

eason

Summary

The owner of StakingPool.sol can add or remove strategies from the pool. However, the owner cannot withdraw all tokens from the strategies, resulting in a revert when calling removeStrategy(), thereby allowing these strategies to remain operational.

Vulnerability Details

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/base/Strategy.sol#L54

The owner of StakingPool.sol can add or remove tokens from the strategy contracts. To remove a strategy, the owner must call the removeStrategy() function. Within this function, the owner first retrieves the total amount of deposits in the strategy by calling strategy.getTotalDeposits(), and then proceeds to withdraw all deposits using strategy.withdraw(totalStrategyDeposits, _strategyWithdrawalData).

However, it's not possible to remove all deposits from the strategy because the logic within strategy.withdraw(totalStrategyDeposits, _strategyWithdrawalData) requires that the total deposits must remain greater than or equal to the minimum specified in getMinDeposits() within the strategy contract. Therefore, if the owner attempts to remove all deposits, they will encounter a revert with the message "Total deposits must remain >= minimum".

Impact

  • The owner is unable to remove strategies that they intend to eliminate.

  • Users can still deposit into strategies that the owner wants to remove.

PoC

it('unable to remove strategies with token balance', async () => {
const { signers, accounts } = await getAccounts()
const adrs: any = {}
​
const token = (await deploy('contracts/core/tokens/base/ERC677.sol:ERC677', [
'Chainlink',
'LINK',
1000000000,
])) as ERC677
adrs.token = await token.getAddress()
await setupToken(token, accounts)
​
const erc677Receiver = (await deploy('ERC677ReceiverMock')) as ERC677ReceiverMock
adrs.erc677Receiver = await erc677Receiver.getAddress()
​
const stakingPool = (await deployUpgradeable('StakingPool', [
adrs.token,
'LinkPool LINK',
'lpLINK',
[
[accounts[4], 1000],
[adrs.erc677Receiver, 2000],
],
toEther(10000),
])) as StakingPool
adrs.stakingPool = await stakingPool.getAddress()
​
const strategy1 = (await deployUpgradeable('StrategyMock', [
adrs.token,
adrs.stakingPool,
toEther(1000),
toEther(10),
])) as StrategyMock
adrs.strategy1 = await strategy1.getAddress()
​
const strategy2 = (await deployUpgradeable('StrategyMock', [
adrs.token,
adrs.stakingPool,
toEther(2000),
toEther(20),
])) as StrategyMock
adrs.strategy2 = await strategy2.getAddress()
​
const strategy3 = (await deployUpgradeable('StrategyMock', [
adrs.token,
adrs.stakingPool,
toEther(10000),
toEther(10),
])) as StrategyMock
adrs.strategy3 = await strategy3.getAddress()
​
await stakingPool.addStrategy(adrs.strategy1)
await stakingPool.addStrategy(adrs.strategy2)
await stakingPool.addStrategy(adrs.strategy3)
await stakingPool.setPriorityPool(accounts[0])
await stakingPool.setRebaseController(accounts[0])
​
await token.approve(adrs.stakingPool, ethers.MaxUint256)
await stakingPool.deposit(accounts[0], toEther(1000), ['0x', '0x'])
​
//--------------------------------------------------------------------------------
​
// can remove strategy with empty token balance
await stakingPool.removeStrategy(1, '0x', '0x')
let strategies = await stakingPool.getStrategies()
assert.equal(
JSON.stringify(strategies),
JSON.stringify([adrs.strategy1, adrs.strategy3]),
'Remaining strategies incorrect'
)
​
// can remove strategy with empty token balance
await stakingPool.removeStrategy(1, '0x', '0x')
strategies = await stakingPool.getStrategies()
assert.equal(
JSON.stringify(strategies),
JSON.stringify([adrs.strategy1]),
'Remaining strategies incorrect'
)
​
// cannot remove strategy with token balance
await expect(stakingPool.removeStrategy(0, '0x', '0x')).to.be.revertedWith(
'Total deposits must remain >= minimum'
)
​
// cannot withdraw all token in strategy
let canWithdraw = BigInt(await strategy1.canWithdraw())
const strategy1BalanceBefore = BigInt(await token.balanceOf(adrs.strategy1));
assert.isTrue(strategy1BalanceBefore - canWithdraw > 0)
​
await stakingPool.strategyWithdraw(0, canWithdraw, '0x')
​
// cannot withdraw any token from strategy more
canWithdraw = BigInt(await strategy1.canWithdraw())
const strategy1BalanceAfter = BigInt(await token.balanceOf(adrs.strategy1))
let totalStrategyDeposits = BigInt(await strategy1.getTotalDeposits())
assert.equal(canWithdraw, BigInt(0), "can withdraw 0")
assert.isTrue(strategy1BalanceAfter > 0, "token locked in strategy")
assert.isTrue(totalStrategyDeposits > 0, "total strategy deposits still great than 0")
​
// still cannot remove strategy
await expect(stakingPool.removeStrategy(0, '0x', '0x')).to.be.revertedWith(
'Total deposits must remain >= minimum'
)
})

Tools Used

Manual code review

Recommendations

Add a withdrawAll() function to the strategy contract that can only be called by the owner, allowing owner to withdraw all deposits from the strategy and change the code below.

IStrategy strategy = IStrategy(strategies[_index]);
uint256 totalStrategyDeposits = strategy.getTotalDeposits();
if (totalStrategyDeposits > 0) {
- strategy.withdraw(totalStrategyDeposits, _strategyWithdrawalData);
+ strategy.withdrawAll(_strategyWithdrawalData)
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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