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

StakingPool:`totalStaked` in some accounting does not include tokens that were transferred directly to the contract which ends up skewing certain functions

j1v9

Summary

The totalStaked function does not keep track of tokens that were transferred into the contract directly in some accounting and that can skew/mess up certain functionality.

The withdraw function in the staking pool can revert even if there are enough tokens required to satisfy the withdrawal request due to the totalStake accounting is handled.

The StakingPool canDeposit and the canWithdraw can represent skewed values.

Vulnerability Details

The withdraw function in the staking pool contains this logic

​
function withdraw(address _account, address _receiver, uint256 _amount, bytes[] calldata _data)
external
onlyPriorityPool
{
// .... some code here ....
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; // here
token.safeTransfer(_receiver, toWithdraw);
}

Where a check is made to make sure that the contract has sufficient balance to satisfy the withdrawal from the priority pool for a certain account.

If it does, it skips withdrawing the liquidity from startegies and then burns the LST.
The totalStaked is reduced and the tokens are transfered to the account or withdrawal pool to be used to finalize withdrawals.

The problem arises due to the totalStaked value only updated when:

  • a deposit is made into the staking pool by the priority pool by calling the deposit function

  • tokens are donated to the stakingPool through the donateTokens function

  • a new set of rewards have been updated in the _updateStrategyRewards

  • the withdraw function in the staking pool by the prioruty pool

It does not consider tokens that were transferred directly into the stakingPool even though when its time to deposit those tokens we deposit them with the _depositLiquidity function as part of the contract's balance, we also account for them as part of our unused balance and other forms of accounting.

A situation where tokens were transferred directly into the staking pool will allow the require check to pass,

Assuming the toWithdraw is 32 staked tokens and the balance of the stakingPool is 35 ( 30 due to the natural ways totalStaked is updated i.e deposits, withdrawals, updating strategy rewards while the remaining 5 are the tokens that were transferred directly into the contract )

// .... some code here ....
require(token.balanceOf(address(this)) >= toWithdraw, "Not enough liquidity available to withdraw");
// ..... some code here .....

Since the balance of the stakingPool contract(35 tokens) is greater than the toWithdraw(32 tokens) amount, the initial check passes, but further down the code, the reduction of the totalStaked(30) by the toWithdraw would revert due to overflow even though there are enough tokens in the stakingPool to satisfy the withdrawal requests and these tokens would still end up being used in other forms of accounting.

totalStaked -= toWithdraw; // reverts here due to contract having enough liquidity but totalStaked not being up to it

The canDeposit and canWithdraw functions in the stakingPool also make use of the totalStaked to keep track of how much tokens can be deposited into and withdrawn from the contract. The priorityPool uses these functions as well.

The canDeposit function using the totalStaked, uses the totalStaked value to know if there is still space for the depositing available in the stakingPool.

function canDeposit() external view returns (uint256) {
// returns the maximum amount of tokens the pool can hold across all strategies
uint256 max = getMaxDeposits();
​
if (max <= totalStaked) {
return 0;
} else {
// returns the difference
return max - totalStaked;
}
}

The prioriyPool uses the canDeposit here where it makes sure it deposits what it can deposit, if the canDeposit returns a skewed value e.g getMaxDeposits is 2000 tokens and totalStaked = 1900 tokens but the tokens transferred directly into the contract plus the totalStaked, i.e contract balance = 2100 tokens. The canDeposit would return 100 tokens worth of deposit space. The same logic applies for the canWithdraw.

function _deposit(address _account, uint256 _amount, bool _shouldQueue, bytes[] memory _data) internal {
//.... some code here ....
​
if (totalQueued == 0) {
// .... some code here ...
if (toDeposit != 0) {
// using the skewed canDeposit function here
uint256 canDeposit = stakingPool.canDeposit();
if (canDeposit != 0) {
uint256 toDepositIntoPool = toDeposit <= canDeposit ? toDeposit : canDeposit;
stakingPool.deposit(_account, toDepositIntoPool, _data);
toDeposit -= toDepositIntoPool;
}
}
}
//.... some code here .....
}

Impact

Withdrawals in the staking pool by the priority pool can still revert even if there are enough tokens to satisfy withdrawal.

The canWithdraw and the canDeposit function return values used in the priorityPool deposit and withdrawal calculations are skewed.

Recommendations

When using the totalStaked for some accounting, include the balance of the stakingPool contract as well.

Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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