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

Insufficient Merkle Tree Validation in `withdraw` Function of `PriorityPool` Contract

nexarion

Summary

The withdraw function in the PriorityPool contract does not adequately verify the sender's inclusion in the Merkle tree when the _merkleproof.length ==0. This could allow unauthorized withdrawals by an account that is outside the Merkle tree by setting the _merkleproof to zero. Although this is of low severity, it is worth considering

Vulnerability Details

Consider the code snippet below:

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L284-L297

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

In the PriorityPool contract, the withdraw function is responsible for allowing users to withdraw their tokens. The function includes a validation step to verify the sender's inclusion in the Merkle tree using a Merkle proof. However, if the _merkleProof is empty, the function only checks if accountIndexes[account] < merkleTreeSize. This check might not be sufficient to ensure that the sender is included in the Merkle tree. A sender outside the Merkle tree can bypass the validation by setting _merkleProof array to zero

Impact

Insufficient validation could allow unauthorized withdrawals if the accountIndexes mapping is not correctly updated. However, the impact is considered low severity because it relies on the improper maintenance of accountIndexes, which is not likely

Tools Used

  • Manual code review

Recommendations

Allow this adjustment in the code

If (accountIndexes[account] < merkleTreeSize){
if (_merkleProof.length != 0) {
bytes32 node = keccak256(
bytes.concat(keccak256(abi.encode(account, _amount, _sharesAmount)))
);
if (!MerkleProofUpgradeable.verify(_merkleProof, merkleRoot, node))
revert InvalidProof();
}
} else {
revert InvalidProof();
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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