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

StakingPool::strategyDeposit doesnt update total link staked leading to wrong staked tracking balance and wrong staked return values of how much can be staked

0xw3

Summary

StakingPool::strategyDeposit doesnt update totalStaked variable when depositing to strategies, leading to wrong staked tracking balance and wrong staked return values in StakingPool::canDeposit()

Vulnerability Details

StakingPool uses totalStaked variable to keep track of how much token have been deposited to its strategies.
For example in 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);
//...
@=> totalStaked += _amount; // totalStaked updated
}
}

However in strategyDeposit this doesnt occours:

function strategyDeposit(
uint256 _index,
uint256 _amount,
bytes calldata _data
) external onlyOwner {
require(_index < strategies.length, "Strategy does not exist");
IStrategy(strategies[_index]).deposit(_amount, _data);
}

So, when this method its called, will deposit to strategy, decreasing strategy capacity and leading to inacurate link staked/deposited balance tracking amount in StakingPool.
Also, this leads to canDeposit() function to malfunction because it mainly depends on totalStaked variable value

function canDeposit() external view returns (uint256) {
uint256 max = getMaxDeposits();
​
if (max <= totalStaked) {
return 0;
} else {
return max - totalStaked;
}
}

The following PoC defines an scenario with:
A StakingPool with:
unusedDepositLimit = 20 Eth and
One Strategy with maxDeposits = 100 Eth

StakingPool's owner calls strategyDeposit with 90 eth of link this decreases strategy capacity.
But StakingPool::strategyDeposit() doesnt increment totalStaked variable, so, its like nothing was deposited.
After this, StakingPool::canDeposit() will return a wrong amount of how much can be staked in this pool.
Any user who calls StakingPool::canDeposit() to query how much can be deposited receives a wrong value, and if it uses this value, his calls to StakingPool::deposit() will fail.

Create test in test/core/priorityPool/priority-pool.test.ts

it.only('strategyDeposit doesnt update totalStaked causing canDeposit() to give wrong return value', async () => {
const { signers, accounts, adrs, token, stakingPool, strategy, sdlPool, pp, withdrawalPool } = await loadFixture(
deployFixture
)
//
await strategy.setMaxDeposits(toEther(100));
await stakingPool.setUnusedDepositLimit(toEther(20));
​
let stakingLimit = await stakingPool.getMaxDeposits()
console.log("StakingLimit\t\t\t",fromEther(stakingLimit))
​
let unusedDeposits = await stakingPool.unusedDepositLimit()
console.log("unusedDeposits\t\t\t",fromEther(unusedDeposits));
console.log(
"stakingLimit + unusedDeposits\t",
fromEther(stakingLimit + unusedDeposits)
);
​
//let etherAmount = toEther(stakingLimit);
let etherAmount = toEther(90);
let index = 1;
let signer = signers[index]
let account = accounts[index]
console.log("\namountToStake\t\t\t",fromEther(etherAmount));
​
console.log(`Will use strategyDeposit instead of deposit to deposit ${fromEther(etherAmount)}`)
console.log(
`\ncanDeposit() before strategyDeposit\t`,
fromEther(await stakingPool.canDeposit())
)
​
// strategyDeposit
console.log("\nCalling stakingPool.strategyDeposit");
await token.transfer(stakingPool.getAddress(), etherAmount)
await stakingPool.strategyDeposit(0, etherAmount, '0x')
​
console.log(
`\ncanDeposit() after strategyDeposit\t`,
fromEther(await stakingPool.canDeposit())
)
console.log("await stakingPool.getMaxDeposits()\t",fromEther(await stakingPool.getMaxDeposits()));
​
let depositAmount = await stakingPool.canDeposit();
console.log(`\nUser use canDeposit() value to stake calling SPool::deposit() but his transaction fails`)
console.log(`(tx is catched with expect .to.be.reverted)`)
await token.connect(signers[2]).transfer(accounts[0], depositAmount)
​
await expect(pp.connect(signers[2]).deposit(
await stakingPool.canDeposit(),
true,
['0x', '0x', '0x']
)).to.be.reverted
})

Impact

Severity: Medium due to this flaw leads to wrong balance tracking and wrong return value of how much can be staked in canDeposit() function

Tools Used

Manual Review

Recommendations

SPool::strategyDeposit should update totalStaked amount (the variable that tracks deposits to strategies) and check if max deposit limit has been reached, calling SPool::canDeposit()

Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

[INVALID] `strategyDeposit` doesn't update `totalStaked`

Appeal created

demorextess Judge
10 months ago
0xw3 Submitter
10 months ago
demorextess Judge
10 months ago
0xw3 Submitter
10 months ago
demorextess Judge
10 months ago
inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[INVALID] `strategyDeposit` doesn't update `totalStaked`

Support

FAQs

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