search

sherlock-audit / 2024-08-morphl2-judging

6 stars 3 forks source link

underdog - `proveState()` can be frontrun, enabling malicious actors to steal sequencer's proof submission rewards #185

Open sherlock-admin3 opened 1 month ago

sherlock-admin3 commented 1 month ago

underdog

High

proveState() can be frontrun, enabling malicious actors to steal sequencer's proof submission rewards

Summary

Because proveState() is permissionless, malicious actors can frontrun sequencers proving the correctness of a batch in order to steal the challengeDeposit().

Root Cause

Found in Rollup.sol#L465.

Morph incorporates a novel state verification process called Responsive Validity Proof (RVP). In this process, sequencers submit batches to the L1’s rollup contract. If a submitted batch is found to be incorrect, challengers can trigger challengeState() in order to start a challenge period, in which it is the sequencer’s responsibility to prove the correctness of the batch by submitting a ZK proof via the proveState() function in Rollup.sol.

As detailed in the RVP is Friendly to Challengers section the novel RVP process opens a potential attack vector. Quoting Morph’s documentation:

Essentially, when a batch is challenged, generating a proof to prove the correctness of the batch will incur computational costs that need to be handled by the sequencer that submitted the batch. Malicious challengers could then always challenge batches (even if they’re correct) to force sequencers to waste money and resources to generate the proof. In order to mitigate this, challengers are required to deposit a minimum challengeDeposit() when initiating a challenge:

// File: Rollup.sol

function challengeState(uint64 batchIndex) external payable onlyChallenger nonReqRevert whenNotPaused { 
        ...
        // check challenge amount
        require(msg.value >= IL1Staking(l1StakingContract).challengeDeposit(), "insufficient value");

        ...
    }

Then, if the proveState() function is triggered with a valid proof proving the correctness of the batch, the defender will win and the challengeDeposit() will be

// File: Rollup.sol
function proveState(
        bytes calldata _batchHeader,
        bytes calldata _aggrProof,
        bytes calldata _kzgDataProof
    ) external nonReqRevert whenNotPaused {
        ...

        // Check for timeout 
        if (challenges[_batchIndex].startTime + proofWindow <= block.timestamp) { 
            ...
        } else {
            _verifyProof(memPtr, _aggrProof, _kzgDataProof); 
            // Record defender win
            _defenderWin(_batchIndex, _msgSender(), "Proof success");
        }
    }

    function _defenderWin(uint256 batchIndex, address prover, string memory _type) internal {
        uint256 challengeDeposit = challenges[batchIndex].challengeDeposit;
        batchChallengeReward[prover] += challengeDeposit;
        emit ChallengeRes(batchIndex, prover, _type);
    }

It is important to note how the call to _defenderWin() passes _msgSender() as the prover, which is the address that will obtain the challengeDeposit.

As shown in the code snippet, there is no access control in proveState(), so it is possible for a malicious actor to frontrun sequencers that want to prove the correctness of a batch (and which have wasted money and resources to generate such proof) in order to obtain the challengeDeposit. It is then possible for anyone to steal challenge deposits every time a batch is proven to be correct, leading to a loss of funds for the sequencer that has generated the proof.

Internal pre-conditions

  1. A batch is challenged. The batch is correct.
  2. The sequencer that submitted the batch has generated the proof that proves the correctness of the batch, and has triggered a call to proveState() to submit the proof.

External pre-conditions

None.

Attack Path

  1. A sequencer submits a batch via commitBatch().
  2. A challenger then challenges the batch by calling challengeState(), and deposits the minimum challengeDeposit.
  3. The sequencer generates the proof for the batch. When generated, it calls proveState().
  4. A malicious actor is monitoring calls to proveState(), and frontruns the sequencer. He calls proveState() with the exact same parameters triggered by the sequencer.
  5. Due to the lack of access control in proveState(), the malicious’ actor call is first executed. The parameters (_batchHeader, _aggrProof and _kzgDataProof) are correct and prove the batch’s correctness, so a call to _defenderWin() is triggered, passing the address of the malicious actor as the prover.
  6. The malicious actor obtains the challenge deposit, effectively stealing it from the sequencer.

Note that this bug also essentially enables malicious challengers to always challenge batches at no cost. If the malicious challengers act as the malicious actor frontrunning the call to proveState(), it is basically free (excluding gas costs) for malicious challengers to always challenge batches, affecting the proper functioning of Morph, given that data availability can be effectively delayed, and bypassing the expected behavior of the system for the vulnerability described in Morph documentation.

Impact

The sequencers can always lose the challengeDeposit() entitled to them in order to compensate them for the costs of generating the zero knowledge proof to prove the correctness of the batch. Moreover, there is essentially no cost for challengers to challenge correct batches and affect the proper behavior of the network.

PoC

No response

Mitigation

In order to mitigate this issue, ensure that proofs can only be submitted by the sequencer that submitted the batch. For this, the sequencer should be stored when committing a batch, given that as per the documentation the expected design of the system wants to reward the sequencer generating the proof. Another possible option is to allow any active sequencer to submit the proof, if any sequencer is expected to prove the correctness of batches:

// File: Rollup.sol

function proveState(
        bytes calldata _batchHeader,
        bytes calldata _aggrProof,
        bytes calldata _kzgDataProof
    ) external nonReqRevert whenNotPaused {
        ...

        // Check for timeout 
        if (challenges[_batchIndex].startTime + proofWindow <= block.timestamp) { 
            ...
        } else {
+          require(IL1Staking(l1StakingContract).isActiveStaker(_msgSender()), "only active staker allowed");
            _verifyProof(memPtr, _aggrProof, _kzgDataProof); 
            // Record defender win
            _defenderWin(_batchIndex, _msgSender(), "Proof success");
        }
    }
sherlock-admin2 commented 3 weeks ago

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

ulasacikel commented 1 week ago

@sherlock-admin2 The issue is not fixed in the https://github.com/morph-l2/morph/pull/563. It fixes another issue that's unrelated to front-running. In fact they implemented the fix I recommended here: https://github.com/sherlock-audit/2024-08-morphl2-judging/issues/38 I think this comment needs to be updated.