51651 sc insight redundant array access in removestakerfromvalidator

Submitted on Aug 4th 2025 at 16:13:23 UTC by @AasifUsmani for Attackathon | Plume Network

• Report ID: #51651

• Report Type: Smart Contract

• Report severity: Insight

• Target: https://github.com/immunefi-team/attackathon-plume-network/blob/main/plume/src/lib/PlumeValidatorLogic.sol

Description

Brief/Intro

In the removeStakerFromValidator function of the PlumeValidatorLogic library, an unnecessary array access occurs during the removal of a staker from a validator's list. Specifically, the declaration and access of lastStaker happen unconditionally, even when no swap is needed (i.e., when removing the last element). This leads to a redundant storage read (SLOAD) that can be avoided by moving the access inside the conditional that performs the swap.

Vulnerability Details

The issue is in Part 1 of the function, which uses a swap-and-pop pattern to remove a staker from $.validatorStakers[validatorId]:

if (indexToRemove < listLength && stakersList[indexToRemove] == staker) {
    address lastStaker = stakersList[listLength - 1];  // Redundant if no swap is needed
    if (indexToRemove != listLength - 1) {
        stakersList[indexToRemove] = lastStaker;
        $.userIndexInValidatorStakers[lastStaker][validatorId] = indexToRemove;
    }
    stakersList.pop();
}

• The line address lastStaker = stakersList[listLength - 1]; is executed regardless of whether a swap will occur.

• If indexToRemove == listLength - 1 (removing the last element), no swap is needed — only pop() is required. In that case, lastStaker is never used, making the read unnecessary.

• This results in a redundant SLOAD, costing gas without changing behavior.

• Functionally the code is correct; this is a low-risk optimization opportunity.

Severity: Low / Informational — No security risk or incorrect state change. Optimization and clarity improvement only.

Impact Details

• Gas Efficiency: Each redundant SLOAD adds gas (roughly 100–200 gas per call). In high-frequency removal scenarios (mass unstaking, bulk operations), this can accumulate.

• Code Quality: Removing the redundant read improves clarity and aligns with standard swap-and-pop patterns in Solidity, reducing potential confusion for maintainers and auditors.

Recommended Fix

Move the lastStaker declaration inside the conditional that performs the swap so it is only read when needed:

if (indexToRemove < listLength && stakersList[indexToRemove] == staker) {
    if (indexToRemove != listLength - 1) {
        address lastStaker = stakersList[listLength - 1];
        stakersList[indexToRemove] = lastStaker;
        $.userIndexInValidatorStakers[lastStaker][validatorId] = indexToRemove;
    }
    stakersList.pop();
}

This avoids the unnecessary storage read when removing the last element.

References

https://github.com/immunefi-team/attackathon-plume-network/blob/580cc6d61b08a728bd98f11b9a2140b84f41c802/plume/src/lib/PlumeValidatorLogic.sol#L94

Proof of Concept

Consider a validator's stakersList with 3 elements: [0xA, 0xB, 0xC] (length = 3).

1

Remove last element (0xC)

• indexToRemove = 2

• listLength = 3

Current code:

• Reads lastStaker = stakersList[2] (0xC) unconditionally.

• Checks if (2 != 2) → false, so no swap.

• Pops the last element → resulting list: [0xA, 0xB]

Redundancy: lastStaker was read but never used.

2

Optimized flow

With the recommended change:

• The code checks if (indexToRemove != listLength - 1) first.

• Since it's false, it skips reading stakersList[listLength - 1].

• Calls pop() → resulting list: [0xA, 0xB]

Benefit: One less SLOAD (gas saved) when removing the last element.