070.md

Fierce Coral Turkey

Medium

multiple RemoveStakers messages can be pending resulting in a failing slash

Description

Inside the slash function, a message is dispatched to remove all stakers on L2:

    function slash(uint256 sequencersBitmap) external onlyRollupContract nonReentrant returns (uint256) {
        address[] memory sequencers = getStakersFromBitmap(sequencersBitmap);
        uint256 valueSum;
        for (uint256 i = 0; i < sequencers.length; i++) {
            if (withdrawals[sequencers[i]] > 0) {
                delete withdrawals[sequencers[i]];
                valueSum += stakingValue;
            } else if (!isStakerInDeleteList(sequencers[i])) {
                // If this is the first time the sequencer is slashed
                valueSum += stakingValue;
                _removeStaker(sequencers[i]);
                // Remove from whitelist
                delete whitelist[sequencers[i]];
                removedList[sequencers[i]] = true;
            }
        }

        uint256 reward = (valueSum * rewardPercentage) / 100;
        slashRemaining += valueSum - reward;
        _transfer(rollupContract, reward);

        emit Slashed(sequencers);
        emit StakersRemoved(sequencers);

        // Dispatch message to remove stakers on L2
=>        _msgRemoveStakers(sequencers);

        return reward;
    }

The issue is that the sequencers array may contain stakers that have an already pending removal. To illustrate, consider the removeStaker function, for instance:

    function removeStaker(address[] memory _stakers) external onlyOwner {
        for (uint256 i = 0; i < _stakers.length; i++) {
            require(isActiveStaker(_stakers[i]), "only active staker can be removed");
            require(withdrawals[_stakers[i]] == 0, "withdrawing");

            withdrawals[_stakers[i]] = block.number + withdrawalLockBlocks;
            _removeStaker(_stakers[i]);
            emit Withdrawn(_stakers[i], withdrawals[_stakers[i]]);

            delete whitelist[_stakers[i]];
            removedList[_stakers[i]] = true;
        }
        emit StakersRemoved(_stakers);

        // Dispatch message to remove stakers on L2
=>        _msgRemoveStakers(_stakers);
    }

This function also sends a message to remove stakers on L2.

Given this, the following scenario can occur:

• The contract Owner invokes removeStaker for Alice.
• A message is sent to remove Alice as a staker.
• not much later, a slash function is triggered, perhaps because a challenger was successful.
• Alice is included in the array of sequencers to be slashed
• After some checks, _msgRemoveStakers is called to dispatch a message to remove all sequencers, including Alice.
• However, Alice's first removal request goes through first
• This will cause the second removal request, initiated by slashing, to fail.
• Ultimately the complete slashing operation will fail

As per the sponsor:

there should be no situation where stakers are removed repeatedly

In the current scenario, two messages will be sent to remove Alice. The first message, removeStaker, will succeed, while shortly after, the slash function will be called with Alice included as a sequencer, which will ultimately fail.

Recommendation

introduce some sort of logic to mitigate this issue. Perhaps you might want to perform a pending removal check whenever performing the slash function.