Liquid Staking

Summary

The removeOperators function in the OperatorStakingPool could fail to remove all specified operators particularly when multiple operators are meant to be removed in a single transaction.

Vulnerability Details

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorStakingPool.sol#L176-L180

The issue here is after swapping an operator to be removed with the last element in the array and then removing it, the loop continues to the next index without rechecking the current index. This can lead to skipping the check for the operator that was swapped into the current position.

Consider a case where wee have an operators array with five operators lets say: [A, B, C, D, E]
We want to remove operators B and D.

1. Function starts with: [A, B, C, D, E]

2. Loop starts, j = 0

3. Check A, not a match, j increments to 1

4. Check B, it's a match:
   -- Swap B with E: [A, E, C, D, B]
   -- Remove last element: [A, E, C, D]
   -- Decrement numOperators to 4
   -- j increments to 2

5. Check C, not a match, j increments to 3

6. Check D, it's a match:
   --Swap D with the last element (which is now D itself): [A, E, C, D]
   --Remove last element: [A, E, C]
   -- Decrement numOperators to 3
   -- j increments to 4

7. Loop ends because j (4) is not < numOperators (3)

Result: [A, E, C]
The issue heere is that: E was never checked for removal, even though it was in the removal list.

Impact

Skipped or incomplete removal of operators, leaving unintended operators

Tools Used

Manual

Recommendations

The loop in should recheck the same index after removing an operator:

With the fix sugested above , it means the case we looked at earlier will now be:

1. Function will start with: [A, B, C, D, E]

2. Loop starts, j = 0

3. Check A, not a match, j increments to 1

4. Check B, it's a match:
   -- Swap B with E: [A, E, C, D, B]
   -- Remove last element: [A, E, C, D]
   -- Decrement numOperators to 4
   -- j stays at 1

5. Check E (now at index 1), not a match, j increments to 2

6. Check C, not a match, j increments to 3

7. Check D, it's a match:
   -- Swap D with the last element (C): [A, E, C, C]
   -- Remove last element: [A, E, C]
   -- Decrement numOperators to 3
   -- j stays at 3

8. Loop ends because j (3) is not < numOperators (3)

Result: [A, E, C]
This means all operators were correctly checked including E which was swapped into B's position.