Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: medium
Invalid

Improper Access Control in `updateWithdrawalBatchIdCutoff` Function

Summary

updateWithdrawalBatchIdCutoff function in lacks proper access control, allowing any external entity to modify critical variables related to the withdrawal process.

Vulnerability Details

The updateWithdrawalBatchIdCutoff function in the WithdrawalPool contract is marked as external, allowing anyone to call it. This poses a potential vulnerability as it enables any external entity to manipulate the withdrawalIdCutoff and withdrawalBatchIdCutoff variables without authorization.

The function is intended to update the withdrawalBatchIdCutoff value, which is used to efficiently return data in the getBatchIds function by skipping old withdrawal batches. However, the current implementation lacks proper access control mechanisms, leaving it open to unauthorized access.

Proof of Vulnerability

Upon reviewing the WithdrawalPool contract code, it is evident that the updateWithdrawalBatchIdCutoff function is defined with the external visibility modifier: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L370

function updateWithdrawalBatchIdCutoff() external {
// @audit The function is marked as external, allowing anyone to call it
uint256 numWithdrawals = queuedWithdrawals.length;
uint256 newWithdrawalIdCutoff = withdrawalIdCutoff;
// ...
// find the last batch where all withdrawals have no funds remaining
for (uint256 i = newWithdrawalBatchIdCutoff; i < numBatches; ++i) {
if (withdrawalBatches[i].indexOfLastWithdrawal >= newWithdrawalIdCutoff) {
break;
}
newWithdrawalBatchIdCutoff = i;
}
withdrawalIdCutoff = uint128(newWithdrawalIdCutoff);
withdrawalBatchIdCutoff = uint128(newWithdrawalBatchIdCutoff);
}

The external modifier allows any external account or contract to invoke the function, regardless of their permissions or role within the system. This lack of access control can lead to unintended behavior and potential manipulation of critical variables.

Furthermore, the function directly modifies the withdrawalIdCutoff and withdrawalBatchIdCutoff variables without any additional checks or validations: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L396-L397

withdrawalIdCutoff = uint128(newWithdrawalIdCutoff);
withdrawalBatchIdCutoff = uint128(newWithdrawalBatchIdCutoff);

These variables are used in other functions, such as getBatchIds, to determine the range of withdrawal batches to consider. Allowing unrestricted access to modify these variables can compromise the integrity of the withdrawal process and lead to incorrect or manipulated results.

Impact

  • If the withdrawalBatchIdCutoff value is manipulated, it can lead to incorrect retrieval of withdrawal batches in the getBatchIds function. This can result in users receiving inaccurate information about their withdrawal batches and associated data.

  • An attacker could repeatedly call the updateWithdrawalBatchIdCutoff function with malicious intent, causing unnecessary computations and potentially impacting the performance and availability of the WithdrawalPool contract.

Tools Used

Manual Review

Recommendations

  1. If the function should indeed be callable by anyone, remove any unintended access control restrictions.

  2. If access control is required, update the function's visibility and add appropriate modifiers to enforce the desired access control rules.

- function updateWithdrawalBatchIdCutoff() external {
+ function updateWithdrawalBatchIdCutoff() external onlyOwner {
// function implementation
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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