sammy - The 255th staker in `L1Staking.sol` can avoid getting slashed and inadvertently cause fund loss to stakers

[sherlock-admin2]

sammy

High

The 255th staker in L1Staking.sol can avoid getting slashed and inadvertently cause fund loss to stakers

Summary

The 255th staker in L1Staking.sol can avoid getting slashed and inadvertently cause fund loss to stakers

Vulnerability Detail

The L1Staking.sol contract supports upto 255 stakers :

    /// @notice all stakers (0-254)
    address[255] public stakerSet;

    /// @notice all stakers indexes (1-255). '0' means not exist. stakerIndexes[1] releated to stakerSet[0]
    mapping(address stakerAddr => uint8 index) public stakerIndexes;

Everytime a staker registers in L1Staking.sol they are added to the stakerSet and their index is stored in stakerIndexes as index+1

These stakers, while active, can commit batches in Rollup.sol using commitBatch() and the batchDataStore[] mapping is updated as follows :

            batchDataStore[_batchIndex] = BatchData(
                block.timestamp,
                block.timestamp + finalizationPeriodSeconds,
                _loadL2BlockNumber(batchDataInput.chunks[_chunksLength - 1]),
                // Before BLS is implemented, the accuracy of the sequencer set uploaded by rollup cannot be guaranteed.
                // Therefore, if the batch is successfully challenged, only the submitter will be punished.
                IL1Staking(l1StakingContract).getStakerBitmap(_msgSender()) // => batchSignature.signedSequencersBitmap
            );

On a successful challenge, the _challengerWin() function is called, and here the sequencersBitmap is the one that was stored in batchDataStore[]

    function _challengerWin(uint256 batchIndex, uint256 sequencersBitmap, string memory _type) internal {
        revertReqIndex = batchIndex;
        address challenger = challenges[batchIndex].challenger;
        uint256 reward = IL1Staking(l1StakingContract).slash(sequencersBitmap);
        batchChallengeReward[challenges[batchIndex].challenger] += (challenges[batchIndex].challengeDeposit + reward);
        emit ChallengeRes(batchIndex, challenger, _type);
    }

This function calls the slash() function in L1Staking.sol :

    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);

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

        // send message to remove stakers on l2
        _msgRemoveStakers(sequencers);

        return reward;
    }

The function converts the sequencersBitmap into an array by calling getStakersFromBitmap() :

    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++) {
            if ((bitmap & (1 << i)) > 0) {
                stakerAddrs[index] = stakerSet[i - 1];
                index = index + 1;
                if (index >= stakersLength) {
                    break;
                }
            }
        }
    }

Since bitmap will only contain 1 staker's bit, the stakersLength here will be 1. The loop then checks every single bit of the bitmap to see if it's active. Notice, however, that i only goes up to 254, and 255 is skipped. This means that for the 255th staker having index of 254, the array will contain address(0).

This means that in slash(), this code will be execued :

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;
            }

_removeStaker() is called with addr = address(0):

    function _removeStaker(address addr) internal {
        require(deleteableHeight[addr] == 0, "already in deleteList");
        deleteList.push(addr);
        deleteableHeight[addr] = block.number + withdrawalLockBlocks;
    }

This means that the staker avoids being removed and the intended state changes are made to address(0) instead. The staker can continue committing invalid batches to the Rollup and not get slashed. Additionally, the stakingValue is still rewarded to the challenger, while the staker isn't actually removed from the protocol. Over time, the ETH of L1Staking.sol will run out and it won't be possible for stakers to withdraw or for them to get slashed.

Impact

Critical - loss of funds and breaks protocol functionality

Coded POC

    function test_poc_255() external{
         address[] memory add = new address[](255);

         for(uint256 i = 0 ; i < 255 ; i++)
         {
            add[i] = address(bytes20(bytes32(keccak256(abi.encodePacked(1500 + i)))));
         }

        hevm.prank(multisig);
        l1Staking.updateWhitelist(add, new address[](0));

        // register all the 255 stakers
        for(uint256 i = 0 ; i < 255 ; i++)
         {
        Types.StakerInfo memory info;
        info.tmKey = bytes32(i+1);
        bytes memory blsKey = new bytes(256);
        blsKey[31] = bytes1(uint8(i)); 
        info.blsKey = blsKey;
        assert(info.blsKey.length == 256);
        hevm.deal(add[i], 5 * STAKING_VALUE);
        hevm.prank(add[i]);
        l1Staking.register{value: STAKING_VALUE}(info.tmKey, info.blsKey);
         }

        assertEq(add.length, 255);
        address[] memory arr = new address[](1);
        arr[0] = add[254];
         uint256 _bitmap = l1Staking.getStakersBitmap(arr); // this bitmap will contain the 255th staker only
        address[] memory stakers = l1Staking.getStakersFromBitmap(_bitmap);

        // as you can see the array is {address(0)}
        assertEq(stakers[0], address(0));

        // simulate the challenger win flow
        hevm.prank(l1Staking.rollupContract());
        uint256 balanceBefore = address(l1Staking).balance;
        uint256 reward = l1Staking.slash(_bitmap);    
        uint256 balanceAfter = address(l1Staking).balance;

        // the contract loses "reward" amount of ETH
        assertEq(balanceBefore, balanceAfter + reward);

        // the 255th staker still remains an active staker
        assert(l1Staking.isActiveStaker(arr[0]) == true);
}

To run the test, copy the above in L1Staking.t.sol and run forge test --match-test "test_poc_255"

Code Snippet

Tool used

Manual Review

Recommendation

Make the following change :

-        for (uint8 i = 1; i < 255; i++) {
+        for (uint8 i = 1; i <= 255; i++) {

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/morph-l2/morph/pull/562