Summary

A logic flaw in the PriorityPools deposit function leads to duplicate entries in the accounts array and incorrect values in the accountIndexes mapping. Specifically, when an account with index 0 deposits additional tokens, the contract mistakenly treats the account as new and re-adds it to the accounts array. This causes the contract to incorrectly index accounts, resulting in failures during withdrawals and unqueueing of tokens when certain checks incorrectly revert. The root cause lies in using 0 as both a valid index and a sentinel value, which creates ambiguity in the handling of accounts.

Vulnerability Details

The logic checks if accountIndexes[_account] == 0 to determine whether the account has been added to the accounts array. However, 0 is also the index for the first account in the array. This creates a conflict: if an account already exists at index 0, subsequent deposits by the same account will mistakenly trigger the condition accountIndexes[_account] == 0 and treat the account as new.

https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/priorityPool/PriorityPool.sol#L632-L644

As a result:

1. The account is added again to the accounts array, creating a duplicate entry.

2. The accountIndexes mapping is updated to point to the new index (the length of the accounts array minus 1), overriding the previous index.

This results in the following incorrect state:

• The accounts array contains duplicate entries of the same account.

• The accountIndexes mapping points to the latest index of the duplicate account, causing issues in later functions that rely on accurate indexing.

Example

1. User 0x0000000000000000000000000000000000000001 makes an initial deposit of 100e18 tokens. The user is added to the accounts array at index 0, and accountIndexes[_account] is correctly set to 0.

   accounts = [0x0000000000000000000000000000000000000001];

   accountIndexes[0x0000000000000000000000000000000000000001] = 0;

2. The same user deposits 150e18 tokens. Since accountIndexes[_account] == 0 is true, the contract incorrectly assumes the account is new and adds it again to the accounts array, updating accountIndexes[_account] to 1.

   accounts = [0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000001];

   accountIndexes[0x0000000000000000000000000000000000000001] = 1;

3. Now, the contract contains duplicate entries for the same account, and accountIndexes[_account] no longer points to the first occurrence of the account.

Impact on Withdrawals and Unqueueing Tokens

The duplicate entry causes the accountIndexes mapping to return an incorrect index when the user tries to withdraw or unqueue tokens. This leads to false reverts in the following code:

• The merkleTreeSize is set to the length of the accounts array, which now includes the duplicate entries. However, the accountIndexes[_account] value points to the second occurrence of the account (index 1), leading to a condition where accountIndexes[_account] < merkleTreeSize is true (e.g., 1 < 2), causing the contract to revert with InvalidProof().

As a result, the user is unable to withdraw or unqueue their tokens, effectively locking their funds within the contract.

Affected Functions

1. Withdraw Function:

   else if (accountIndexes[account] < merkleTreeSize) {

   revert InvalidProof();

   }

2. Unqueue Tokens Function:

   if (accountIndexes[account] < merkleTreeSize) {

   revert InvalidProof();

   }

In both cases, the incorrect indexing causes the contract to treat valid users as invalid, preventing them from accessing their funds.

Impact

The primary impact of this vulnerability is denial of service for affected users. Specifically:

• Users whose accounts are mistakenly duplicated in the accounts array will be unable to withdraw or unqueue their tokens due to the incorrect accountIndexes values.

• This could lock significant amounts of user funds in the contract without a way to retrieve them, depending on the deposit and withdrawal patterns.

The severity of this issue is high, as it can affect multiple users and prevent them from accessing their tokens, leading to a loss of trust in the protocol and potential financial losses.

Tools Used

Manual review

Recommendations

Instead of using 0 as a sentinel value for uninitialized accounts, use a value that cannot be confused with a valid index. For example, you can initialize accountIndexes[_account] to a special value like type(uint256).max to indicate that the account has not been added:

Another solution is to avoid using 0 as a valid index altogether. For example, you could shift all indices by 1 and initialize accountIndexes[_account] to 1 rather than 0. This ensures that accountIndexes[_account] == 0 always indicates that the account has not been added yet:

Then, when accessing the array, always subtract 1 from the index.