055.md

Crazy Jetblack Pigeon

High

sequencer cannot be penalized when it is at index 244 in the stakerSet of the L1Staking contract, which will result in incorrect slashing

Summary and Vulnerability Detail

The slash function is used to penalize a malicious sequencer. It accepts a sequencersBitmap and identifies the sequencers using the getStakersFromBitmap function.

    function getStakersFromBitmap(uint256 bitmap) public view returns (address[] memory stakerAddrs) {
        // skip first bit
        uint256 _bitmap = bitmap >> 1;
        uint256 stakersLength = 0;
        while (_bitmap > 0) {
            stakersLength = stakersLength + 1;
            _bitmap = _bitmap & (_bitmap - 1);
        }

        stakerAddrs = new address[](stakersLength);
        uint256 index = 0;
        for (uint8 i = 1; i < 255; i++) {  //-> HERE we should use i < 256
            if ((bitmap & (1 << i)) > 0) {
                stakerAddrs[index] = stakerSet[i - 1];
                index = index + 1;
                if (index >= stakersLength) {
                    break;
                }
            }
        }
    }

Currently, the loop iterates with i from 1 to 244, but it should iterate from 1 to 255 because we can have up to 255 sequencers.

For example, if we have 255 sequencers and the bitmap is 0x8000000000000000000000000000000000000000000000000000000000000000, the getStakersFromBitmap function will incorrectly return an array containing zero address which is a mistake.

If our list contains zero address then It will go in else if block because withdrawals[sequencers[i]] > 0 will be false and !isStakerInDeleteList(sequencers[i]) will be true due to which we will add stakingValue to valueSum but As we know address(0) was not in staker list.

Because of this we will transfer reward to rollupContract and will add valueSum - reward to slashRemaining which is wrong.

    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 it is the first time to be 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);
        ...
    }

Impact

The impact is high because the sequencer cannot be penalized if it is at index 244 in the stakerSet. Additionally, the presence of a zero address in the sequencer list will create problems for future sequencers attempting to withdraw, as there will not be enough ETH available.

Code Snippet

    function getStakersFromBitmap(uint256 bitmap) public view returns (address[] memory stakerAddrs) {
        // skip first bit
        uint256 _bitmap = bitmap >> 1;
        uint256 stakersLength = 0;
        while (_bitmap > 0) {
            stakersLength = stakersLength + 1;
            _bitmap = _bitmap & (_bitmap - 1);
        }

        stakerAddrs = new address[](stakersLength);
        uint256 index = 0;
        for (uint8 i = 1; i < 255; i++) {  //-> HERE we should use i < 256
            if ((bitmap & (1 << i)) > 0) {
                stakerAddrs[index] = stakerSet[i - 1];
                index = index + 1;
                if (index >= stakersLength) {
                    break;
                }
            }
        }
    }

Tool used

Manual Review

Recommendation

My recommendation is that we should iterate from 1 to 255. Below is the correct implementation:

    function getStakersFromBitmap(uint256 bitmap) public view returns (address[] memory stakerAddrs) {
        // skip first bit
        uint256 _bitmap = bitmap >> 1;
        uint256 stakersLength = 0;
        while (_bitmap > 0) {
            stakersLength = stakersLength + 1;
            _bitmap = _bitmap & (_bitmap - 1);
        }


        stakerAddrs = new address[](stakersLength);
        uint256 index = 0;
        for (uint8 i = 1; i < 256; i++) {  // Updated to iterate up to 255
            if ((bitmap & (1 << i)) > 0) {
                stakerAddrs[index] = stakerSet[i - 1];
                index = index + 1;
                if (index >= stakersLength) {
                    break;
                }
            }
        }
    }

References

https://github.com/sherlock-audit/2024-08-morphl2/blob/main/morph/contracts/contracts/l1/staking/L1Staking.sol#L417