105.md

Uneven Leather Salamander

Medium

Wrong proofWindow could be passed eventually slashing honest sequencers

Summary

First of all thanks to the Morph team that provided a spreadsheet in the README containing all values that could be set by admin for all state variables including proofWindow this helped uncovering the vulnerability.

The default value range for the proofWindow variable is from 172800 to 604800. Meaning that this value could be updated and impact a deterministic check within the _proveState function because of it's direct usage.

Vulnerability Detail

Due to the proofWindow (state variable) directly being used within the check,relying on it's state any update would directly impact its functionality. ultimately this check would fail for a ongoing challenge.

if (challenges[_batchIndex].startTime + proofWindow <= block.timestamp) {
            // set status
            challenges[_batchIndex].challengeSuccess = true;
            _challengerWin(_batchIndex, batchDataStore[_batchIndex].signedSequencersBitmap, "Timeout");
        } else {
            _verifyProof(memPtr, _aggrProof, _kzgDataProof);
            // Record defender win
            _defenderWin(_batchIndex, _msgSender(), "Proof success");
        }

Updating proofWindow to a lower value even if initially within the range (604800), would make the window to a shorter time due to the dicrepancy between the higher and lower proofWindow, thus block.timestamp would be too far in the future for a ongoing challenge.

note: Value used for the flow is as example, by nature this vulnerability is unpredictable. Any value whithin the provided range could trigger the issue

Possible flow:

1. state before updateproofWindow = 604.800
   • challenges[100]: (10.001.000 + 604.800 <= 10.500.000)
2. state after update proofWindow = 178.200
   • challenges[100]: (10.001.000 + 178.200 <= 10.500.001) => challenges[100] => "timeout"

Impact

Sequencer or prover would not be able to prove the commited batch, the _challengerWin function called due to "timeout" would slash deposit (L1 - 1 ETH deposit and L2 - remove staker). Challenger receives the reward due to the bug.

Code Snippet

https://github.com/sherlock-audit/2024-08-morphl2/blob/98e0ec4c5bbd0b28f3d3a9e9159d1184bc45b38d/morph/contracts/contracts/l1/rollup/Rollup.sol#L484 https://github.com/sherlock-audit/2024-08-morphl2/blob/98e0ec4c5bbd0b28f3d3a9e9159d1184bc45b38d/morph/contracts/contracts/l1/rollup/Rollup.sol#L392

Tool Used

Manual review

Recommandation

Fix - doing the addition when the challenge is being initiated and inserting it to the BatchChallenge struct.

function challengeState(uint64 batchIndex) external payable onlyChallenger nonReqRevert whenNotPaused {
    require(!inChallenge, "already in challenge");
    require(lastFinalizedBatchIndex < batchIndex, "batch already finalized");
    require(committedBatches[batchIndex] != 0, "batch not exist");
    require(challenges[batchIndex].challenger == address(0), "batch already challenged");
    // check challenge window
    require(batchInsideChallengeWindow(batchIndex), "cannot challenge batch outside the challenge window");
    // check challenge amount
    require(msg.value >= IL1Staking(l1StakingContract).challengeDeposit(), "insufficient value");

    batchChallenged = batchIndex;
-   challenges[batchIndex] = BatchChallenge(batchIndex, _msgSender(), msg.value, block.timestamp, false, false);
+   challenges[batchIndex] = BatchChallenge(batchIndex, _msgSender(), msg.value, block.timestamp, block.timestamp + proofWindow, false, false); 
    emit ChallengeState(batchIndex, _msgSender(), msg.value);
    for (uint256 i = lastFinalizedBatchIndex + 1; i <= lastCommittedBatchIndex; i++) {
        if (i != batchIndex) {
            batchDataStore[i].finalizeTimestamp += proofWindow;
        }
    }

    inChallenge = true;    
}

struct BatchChallenge {
    uint64 batchIndex;
    address challenger;
    uint256 challengeDeposit;
    uint256 startTime;
+   uint256 endTime;  
    bool challengeSuccess;
    bool finished;
}

function proveState(
    bytes calldata _batchHeader,
    bytes calldata _aggrProof,
    bytes calldata _kzgDataProof
) external nonReqRevert whenNotPaused {
    // get batch data from batch header
    (uint256 memPtr, bytes32 _batchHash) = _loadBatchHeader(_batchHeader);
    // check batch hash
    uint256 _batchIndex = BatchHeaderCodecV0.getBatchIndex(memPtr);
    require(committedBatches[_batchIndex] == _batchHash, "incorrect batch hash");

    // Ensure challenge exists and is not finished
    require(batchInChallenge(_batchIndex), "batch in challenge");

    // Mark challenge as finished
    challenges[_batchIndex].finished = true;
    inChallenge = false;
    // Check for timeout
-   if (challenges[_batchIndex].startTime + proofWindow <= block.timestamp) {
+   if (challenges[_batchIndex].endTime <= block.timestamp) {
        // set status
        challenges[_batchIndex].challengeSuccess = true;
        _challengerWin(_batchIndex, batchDataStore[_batchIndex].signedSequencersBitmap, "Timeout");
    } else {
        _verifyProof(memPtr, _aggrProof, _kzgDataProof);
        // Record defender win
        _defenderWin(_batchIndex, _msgSender(), "Proof success");
    }
}