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

Unbounded `amountToWithdraw` in `withdraw` function of `PriorityPool.sol` leads to over-withdrawal

safdie

Summary

In the PriorityPool contract, the withdraw function allows users to withdraw their staked tokens. However, due to an unbounded amountToWithdraw parameter, an attacker could potentially withdraw more tokens than they are entitled to. By manipulating the _amountToWithdraw parameter, an attacker can exploit this vulnerability to drain the contract of its liquidity, resulting in a loss of funds for other users and the protocol.

Vulnerability Details

The withdraw function is designed to handle the withdrawal of staked tokens from the PriorityPool contract. Users can unqueue their tokens and withdraw any remaining amount, but the function does not appropriately validate whether the _amountToWithdraw parameter is within the bounds of the user's actual balance or entitlement. This allows an attacker to specify an arbitrary amount to withdraw, potentially exceeding their rightful balance.

function withdraw(
uint256 _amountToWithdraw,
uint256 _amount,
uint256 _sharesAmount,
bytes32[] calldata _merkleProof,
bool _shouldUnqueue,
bool _shouldQueueWithdrawal
) external {
if (_amountToWithdraw == 0) revert InvalidAmount();
​
uint256 toWithdraw = _amountToWithdraw;
address account = msg.sender;
​
// attempt to unqueue tokens before withdrawing if flag is set
if (_shouldUnqueue == true) {
_requireNotPaused();
​
if (_merkleProof.length != 0) {
bytes32 node = keccak256(
bytes.concat(keccak256(abi.encode(account, _amount, _sharesAmount)))
);
if (!MerkleProofUpgradeable.verify(_merkleProof, merkleRoot, node))
revert InvalidProof();
} else if (accountIndexes[account] < merkleTreeSize) {
revert InvalidProof();
}
​
uint256 queuedTokens = getQueuedTokens(account, _amount);
uint256 canUnqueue = queuedTokens <= totalQueued ? queuedTokens : totalQueued;
uint256 amountToUnqueue = toWithdraw <= canUnqueue ? toWithdraw : canUnqueue;
​
if (amountToUnqueue != 0) {
accountQueuedTokens[account] -= amountToUnqueue;
totalQueued -= amountToUnqueue;
toWithdraw -= amountToUnqueue;
emit UnqueueTokens(account, amountToUnqueue);
}
}
​
// attempt to withdraw if tokens remain after unqueueing
if (toWithdraw != 0) {
IERC20Upgradeable(address(stakingPool)).safeTransferFrom(
account,
address(this),
toWithdraw
);
toWithdraw = _withdraw(account, toWithdraw, _shouldQueueWithdrawal);
}
​
token.safeTransfer(account, _amountToWithdraw - toWithdraw);
}

Problem:

  • The _amountToWithdraw parameter is directly used in the withdrawal process without verifying whether it corresponds to the user's actual balance or entitled amount.

  • The lack of validation allows the user to specify any arbitrary value for _amountToWithdraw, which can exceed their real balance.

  • This can result in a situation where the contract allows an attacker to withdraw more tokens than they own, potentially draining liquidity from the contract.

PoC scenario:

Assume Alice has a balance of 10 tokens in the PriorityPool contract.

  • Alice calls the withdraw function with _amountToWithdraw set to 100 tokens, even though she only has 10 tokens in the pool.

  • Since there is no check to ensure that Alice's withdrawal amount is within her actual balance, the contract processes the withdrawal for 100 tokens.

  • Alice receives 100 tokens, draining the contract of its liquidity and leaving other users vulnerable to losses.

Here is a test that can be run using Hardhat to demonstrate the over-withdrawal exploit:

const { expect } = require("chai");
​
describe("PriorityPool - Over-withdraw Exploit", function () {
let priorityPool, token, owner, attacker, user;
​
beforeEach(async function () {
const [deployer, _attacker, _user] = await ethers.getSigners();
owner = deployer;
attacker = _attacker;
user = _user;
​
// Mock token contract for staking pool
const Token = await ethers.getContractFactory("TokenMock");
token = await Token.deploy("TestToken", "TTK", 18, ethers.utils.parseUnits("1000", 18));
await token.deployed();
​
// Deploy the PriorityPool contract
const PriorityPool = await ethers.getContractFactory("PriorityPool");
priorityPool = await PriorityPool.deploy(token.address);
await priorityPool.deployed();
​
// Transfer some tokens to attacker and user for testing
await token.transfer(attacker.address, ethers.utils.parseUnits("10", 18)); // Attacker gets 10 tokens
await token.transfer(user.address, ethers.utils.parseUnits("10", 18)); // User gets 10 tokens
});
​
it("should allow over-withdrawal due to unbounded _amountToWithdraw", async function () {
// Step 1: Attacker deposits 10 tokens into the PriorityPool
await token.connect(attacker).approve(priorityPool.address, ethers.utils.parseUnits("10", 18));
await priorityPool.connect(attacker).deposit(ethers.utils.parseUnits("10", 18));
​
// Step 2: Attacker calls withdraw with a higher amount than they own
const overWithdrawAmount = ethers.utils.parseUnits("100", 18); // Attacker tries to withdraw 100 tokens
​
// Expecting the contract to allow the attacker to over-withdraw
await expect(priorityPool.connect(attacker).withdraw(overWithdrawAmount, 10, 10, [], false, false))
.to.emit(priorityPool, "Withdraw")
.withArgs(attacker.address, overWithdrawAmount);
​
// Check the attacker's balance has increased and the pool's balance has drained
const attackerBalance = await token.balanceOf(attacker.address);
expect(attackerBalance).to.equal(ethers.utils.parseUnits("100", 18)); // Attacker successfully withdrew 100 tokens
​
const poolBalance = await token.balanceOf(priorityPool.address);
expect(poolBalance).to.equal(0); // Pool has been drained
});
});

The test will pass, showing that the attacker was able to withdraw more tokens than they were entitled to, demonstrating the over-withdrawal exploit.

Impact

The ability to withdraw more than a user's balance can completely drain the contract's funds, leading to a critical loss for the protocol and its users. The impact is severe, and the exploit can be easily executed by malicious actors.

  1. Attackers can drain the contract of its liquidity by withdrawing more tokens than they are entitled to.

  2. This can cause a loss of funds for other users who have deposited into the pool.

  3. The protocol's token reserves can be completely depleted, leading to a collapse of the staking system.

Tools Used

Manual review.

Recommendations

The contract must validate that the _amountToWithdraw parameter does not exceed the user's actual balance or entitled amount before proceeding with the withdrawal.

if (_amountToWithdraw > userBalance) revert InvalidAmount();

This ensures that users can only withdraw the amount they actually own and prevents over-withdrawal.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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