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

Merkle Proofs Can Be Double Spent

auditweiler

Summary

The PriorityPool's merkle tree implementation is vulnerable to double spend attacks.

Vulnerability Details

In the PriorityPool, the same merkleRoot and proof can be used to satisfy multiple simultaneous flows, enabling a user to both claim LSDs and the ability to withdraw simultaneously, if spent in different ways.

For example, the same merkle proof can be used to satisfy both a call to withdraw:

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

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

And simultaneously a call to unqueueTokens:

address account = msg.sender;
// verify merkle proof only if sender is included in tree
if (accountIndexes[account] < merkleTreeSize) {
@> bytes32 node = keccak256(
@> bytes.concat(keccak256(abi.encode(account, _amount, _sharesAmount)))
@> );
@> if (!MerkleProofUpgradeable.verify(_merkleProof, merkleRoot, node))
@> revert InvalidProof();
}
if (_amountToUnqueue > getQueuedTokens(account, _amount)) revert InsufficientBalance();

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

In both journeys, proof construction takes the exact same leaf shape:

keccak256(abi.encode(account, _amount, _sharesAmount))

This makes the same proof valid in both journeys, and consequently, they can be double spent.

Impact

It would be impossible to give someone a proof to permit them to only unqueue their tokens without inadvertently also allowing that same user to simultaneously withdraw a balance of these tokens too.

Tools Used

Manual Review

Recommendations

Prevent leaves from being double spent by ensuring that the proof additionally encodes the intended user journey (i.e. unqueue or withdraw) within the pool, instead of being applicable to multiple flows.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Same merkle proof used for `claimLSDTokens` as well as `unqueueTokens`

It does concern different variables. But using the same merkle inside 3 different functions is not a good practice. Nonces, separators and safety.

Support

FAQs

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