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

The difference between `stakePerShare` at the time of batch and at the time of `WithdrawPool::withdraw()` results in users getting more or less withdraw amount than intended.

falsegenius

Vulnerability Details

The PriorityPool directs user requests to WithdrawPool if queued assets in it are unable to cover the withdrawal amount. When WithdrawalPool gets assets through deposit() function, it triggers _finalizeWithdrawals to cover the queued requests and pushes a WithdrawalBatch into withdrawalBatches which is then used to cover the withdrawals of a user. The WithdrawBatch has the following properties,

  1. indexOfLastWithdrawal

  2. stakePerShares

The issue lies in the stakePerShares that it stores. This represents the the exchange rate of LSTs per underlying shares
at the time of this batch.

The protocol clearly states that lst is a rebasing token so the shares should be worth more tokens after each rebase and
new value is reflected in their token balance. The rebasing can happen any time and the amount user withdraws CAN GO UP OR DOWN depending on the TIMING of when the withdraw() is triggered BECAUSE the RATE CAN CHANGE depending on the current totalShares and totalStaked that StakingPool keeps track of and user SHOULD get amount based on the current rate at which withdraw() takes place, NOT at the rate when WithdrawPool has enough assets to cover the requests due to assets entering the pool through deposit(), or leaving pool, or by some other means.

In WithdrawPool::_finalizeWithdrawals, the rate is cached,

withdrawalBatches.push(WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))) // @audit rate cached

This old outdated rate is used directly in WithdrawPool::withdraw(),

amountToWithdraw += withdrawal.partiallyWithdrawableAmount
+ (uint256(batch.stakePerShares) * uint256(withdrawal.sharesRemaining)) / 1e18;

If the stakePerShares value is cached at the time of batch, the requests WithdrawalBatch covers gets their tokens at this rate no matter when the withdraw() gets triggered. So if withdraw() is called at a later date, users would potentially get tokens at the rate fixed in WithdrawalBatch, not the current rate, resulting in users getting more or less tokens than intended.

Proof-Of-Concept

  1. PriorityPool queues request of UserA for 50 LINK (asset token) in WithdrawalPool.

  2. UserB deposits 50 LINK via PriorityPool::deposit() and it gets deposited to WithdrawPool because there are queued requests there. This calls _finalizeWithdrawals internally and WithdrawBatch is created since the deposit fully covers the queued amount. Now the current rate (stakePerShares) is 2e18 per shares and it is cached in WithdrawBatch.

  3. UserC donates 300 tokens and the rate rises to 2003e15 per shares

  4. UserA calls WithdrawPool::withdraw() and gets tokens at 2e18 instead of 2003e15.

  5. Tokens UserA receives: 50e18
    Tokens UserA should've received: 50075000000000000000

  6. UserA is robbed of 75e15 amount.

The following functions are added in WithdrawPool for testing purposes only. They're used in test added as Proof-Of-Code.

// @self-added-function remove after testing
function getSharesAndStake(uint256 _idx) public view returns (uint256, uint256) {
return (
uint256(queuedWithdrawals[_idx].sharesRemaining),
_getStakeByShares(uint256(queuedWithdrawals[_idx].sharesRemaining))
);
}
// @self-added-function remove after testing
function stakePerSharesForAWithdrawalBatch(uint256 _idx) public view returns (uint256) {
return uint256(withdrawalBatches[_idx].stakePerShares);
}
​
// @self-added-function remove after testing
function calculateAmountToWithdraw(uint256 _stakePerShares, uint256 _sharesRemaining) external view returns (uint256) {
return (_stakePerShares * _sharesRemaining) / 1e18;
}
​

Place the following test in withdraw-pool.test.ts

​
it("Different batch rate and current rate impacts withdrawal amounts", async () => {
const { signers, accounts, withdrawalPool, token, stakingPool } = await loadFixture(deployFixture)
​
// Request for 50 asset tokens is initiated
await withdrawalPool.queueWithdrawal(accounts[0], toEther(50))
// This covers the Withdrawal amount fully and WithdrawBatch is created.
// BatchRate is the rate stored when _getStakeByShares() is triggered in _finalizeWithdrawals()
// BatchRate => stakePerShares stored = 2e18 per shares
await withdrawalPool.deposit(toEther(50))
​
// Custom function used for testing. Idx 1 is provided
// Outputs [ 25 shares, 50 ether ]
const [shares, stakes] = await withdrawalPool.getSharesAndStake(1)
​
// A user donates tokens resulting in increase in getStakeByShares rate
// Current rate => getStakeByShares() => 2003e15 per shares
await stakingPool.donateTokens(toEther(300))
// Custom function used for testing
const stakePerSharesFromBatch = await withdrawalPool.stakePerSharesForAWithdrawalBatch(1)
const currentStakePerShares = await stakingPool.getStakeByShares(toEther(1));
​
// calculateAmountToWithdraw uses the formula used in withdraw() for simulated results.
// withdrawal.partiallyWithdrawableAmount + (uint256(batch.stakePerShares) * uint256(withdrawal.sharesRemaining)) / 1e18;
const amountAtCurrentRate = await withdrawalPool.calculateAmountToWithdraw(currentStakePerShares, shares)
const amountAtBatchRate = await withdrawalPool.calculateAmountToWithdraw(stakePerSharesFromBatch, shares)
// Rate
expect(amountAtCurrentRate).to.be.gt(amountAtBatchRate);
// BatchRate = 2e18 per shares
// currentRate = 2003e15 per shares
})
​

Impact

Users receive tokens at the outdated rate instead of the current rate, depriving them of potential token gains.

Tools Used

Manual Review & Hardhat testing.

Recommendations

Consider one of the following,

  1. Remove stakePerShares property from WithdrawalBatch and using current rate when withdraw() is triggered.

​
function withdraw() {
​
...
...
if (withdrawalId <= batch.indexOfLastWithdrawal) {
+ uint256 rate = _getStakeByShares(1 ether); // current rate
- amountToWithdraw += withdrawal.partiallyWithdrawableAmount + (uint256(batch.stakePerShares) * uint256(withdrawal.sharesRemaining)) / 1e18;
+ amountToWithdraw += withdrawal.partiallyWithdrawableAmount + (rate * uint256(withdrawal.sharesRemaining)) / 1e18;
delete queuedWithdrawals[withdrawalId];
delete withdrawalOwners[withdrawalId];
}
..
..
}

2- Raise totalShares when tokens are donated.

function donateTokens(uint256 _amount) external {
token.safeTransferFrom(msg.sender, address(this), _amount);
+ sharesToMint = (_amount * totalShares) / (totalStaked);
+ _mintShares(address(this), sharesToMint);
totalStaked += _amount;
emit DonateTokens(msg.sender, _amount);
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Appeal created

falsegenius Submitter
10 months ago
inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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