Surge Rollup

Inbox Contracts

6.1 Front-running can steal deposited funds

Verifying the batch of Surge blocks is made on-chain in two steps: the batch is proposed, and then the transition with the corresponding proof is provided. The proposed batch needs to deposit funds that act as a bond. If the proof of correct transition is submitted in the proving window (24 hours), the bond is returned to the proposer. However, if that time has passed, any prover can submit the proof, and such a prover would collect the funds deposited by the proposer. This is done by storing the msg.sender address of the prover in the TaikoInbox contract:

__ts.bondReceiver = inProvingWindow ? meta.proposer : msg.sender;

There are also other cases where the prover challenges the previously submitted proof, in which case such a prover would collect the bond funds as well:

transitions[mti].bondReceiver = msg.sender;
...
_ts.bondReceiver = msg.sender;

However, such transactions can be front-run by anyone with the exact same data. This allows any entity to steal the funds from the prover that actually constructed the correct proof. Note however, that the risk of front-running can be avoided if the proof is provided in the proving window, since in that case the bond goes back to the proposer.

Recommendation

Consider including the bond receiver address into the proof, which would allow only the proof creator to safely submit the proof without the risk of losing the reward.

Resolution

We acknowledge this issue. We intend to fix this in a future fork, and not immediately for the following reasons:

• The effect of the issue is highly restrictive:
  
  • Given the high cost of running a prover infrastructure and considering Surge's positioning of "a not-for-profit rollup", Nethermind is likely the only entity that is going to run the prover for Surge mainnet.
  • We are keeping a proving window of 24 hours at launch, and setting up a sufficiently fault-tolerant infrastructure so as to not miss the initial proving window.
• The solution requires effort from both the smart contract and prover engineers, which may cause potential delays in launch.

6.2 The verification streak check can be bypassed with incorrect batch

One of the requirements for a Stage 2 rollup implemented by Surge is a 45-day upgrade timelock, with execution allowed only if there has not been any period of a week or more in the past 45 days during which no batches were verified. The verification delay checkpoint is registered in verifyBatches(...) function:

if (block.timestamp - batch.lastBlockTimestamp > _config.maxVerificationDelay) {
    _state.stats1.verificationStreakStartedAt = uint64(block.timestamp);
}

However, a verification delay detection can be avoided by proposing and proving the incorrect batches. Since proposing the batch with proposeBatch(...) is permissionless, any caller can propose a batch that is incorrect. Such an incorrect batch is treated as empty when proving the transition. Such transition is accepted on-chain with the batch.lastBlockTimestamp set to an arbitrary timestamp (less than the current timestamp). As a result, the L2 chain could be inactive for over a week, but the on-chain verification delay check would not detect that. Note, however, that this scenario can only happen if no batches are proposed at all.

Recommendation

Consider performing the verification delay on the last correct batch.

Resolution

This issue can happen only if the chain completely halts for over a week (no batches are proposed). In that case, the described avoidance of the check is not seen as a problem. The verification streak check exists for the case where the batches are proposed, but the verification is delayed. Moreover, clarification on incorrect and unprovable batches is described below:

• An "incorrect batch" is not the same as an "unprovable batch".
• A batch is deemed "incorrect" if the transactions listed in the blob cannot be decoded into a list of valid RLP encoded transactions.
  
  • In this case, the prover overwrites the faulty transaction list with an empty transaction list.
  • Prover reference: raiko
  • This is a provable batch and cannot halt the chain.
• A batch is deemed "unprovable" when the prover panics, or is not able to generate the proof at all.
  
  • This is possible due to (but not limited to):
    
    • A bug in the prover software or in the underlying execution client.
    • A bug or a limitation in the underlying zkVM suite.
  • Such a batch is unprovable, not verifiable on-chain, and can halt the chain.
  • Currently, extensive research is ongoing to identify and bypass prover killer blocks.
  • We believe there is no on-chain strategy to immediately mitigate this.

6.3 Lack of setting finalisingTransitionIndex to synchronised batch

When the batch is verified, the synchronisation happens in verifyBatches(...). The state synchronisation happens for every other block. Only the last batch in the group of proven batches is updated in the verifiedTransitionId and finalisingTransitionIndex fields. A situation can occur where the synchronized batch is not the last verified batch. In such a situation, this batch's verifiedTransitionId field also is updated:

function verifyBatches(...)
    public
{
    ...
        for (++ls.batchId; ls.batchId < ls.stopBatchId; ++ls.batchId) {
            ...

            if (ls.batchId % _config.stateRootSyncInternal == 0) {
                synced.batchId = ls.batchId;
                synced.blockId = batch.lastBlockId;
                synced.tid = ls.tid;
                synced.stateRoot = transitions[ls.fti].stateRoot;
            }
        }

        unchecked {
            --ls.batchId;
        }

        if (_stats2.lastVerifiedBatchId != ls.batchId) {
            _stats2.lastVerifiedBatchId = ls.batchId;

            batch = _state.batches[_stats2.lastVerifiedBatchId % _config.batchRingBufferSize];
            // @audit: update of the verified batch
            batch.verifiedTransitionId = ls.tid;
            // Surge: update the finalising transition index for the batch
            batch.finalisingTransitionIndex = uint8(ls.fti);

            emit ITaikoInbox.BatchesVerified(_stats2.lastVerifiedBatchId, ls.blockHash);

            if (synced.batchId != 0) {
                if (synced.batchId != _stats2.lastVerifiedBatchId) {
                    // We write the synced batch's verifiedTransitionId to storage
                    batch = _state.batches[synced.batchId % _config.batchRingBufferSize];
                    // @audit: update of the synchronised 
                    batch.verifiedTransitionId = synced.tid;
                    // @audit: lack of update of
                    // finalisingTransitionIndex for sync batch
                }

                ITaikoInbox.Stats1 memory stats1 = _state.stats1;
                stats1.lastSyncedBatchId = batch.batchId;
                stats1.lastSyncedAt = uint64(block.timestamp);
                _state.stats1 = stats1;

                emit ITaikoInbox.Stats1Updated(stats1);

                // Ask signal service to write cross chain signal
                _signalService.syncChainData(
                    _config.chainId, LibStrings.H_STATE_ROOT, synced.blockId, synced.stateRoot
                );
            }
        }
    ...
}

However, the synchronised batch does not update the finalisingTransitionIndex field. This creates the issue because the function getBatchVerifyingTransition(...) would return the first conflicting transition, which might not to be the correct transition that proved the batch.

function getBatchVerifyingTransition(uint64 _batchId)
    public
    view
    returns (TransitionState memory ts_)
{
    Config memory config = pacayaConfig();

    uint64 slot = _batchId % config.batchRingBufferSize;
    Batch storage batch = state.batches[slot];
    require(batch.batchId == _batchId, BatchNotFound());

    if (batch.verifiedTransitionId != 0) {
        // @audit: since finalisingTransitionIndex is not set
        // it would be 0 and would return the first transition
        // but it may not be the correct transition if there are conflicting
        ts_ =
            state.transitions[slot][batch.verifiedTransitionId][batch.finalisingTransitionIndex];
    }
}

Recommendation

Consider updating the finalisingTransitionIndex for the synchronised batch.

6.4 Lack of resolving ZK+TEE conflicting transitions

The conflicting transitions are resolved in the finality gadget. However, currently only the conflicting transitions with a single proof type are resolved. The transitions with full proof containing both ZK and TEE proofs are not handled, and the transition provided as last is used as the finilising.

 function _tryFinalising(...)
    internal
    returns (uint256)
{
    ...
    if (numTransitions == 1) {
        // If the first transition is just ZK or TEE proven
        if (!_transitions[0].proofType.isZkTeeProof()) {
            if (_transitions[0].createdAt + _config.cooldownWindow > block.timestamp) {
                return fti;
            }
        }

        // The first transition itself is the finalising transition
        fti = 0;
    } else {
        // Proof type(s) to upgrade
        LibProofType.ProofType ptToUpgrade;

        // Try to find a finalising proof
        for (uint256 i; i < numTransitions; ++i) {
            //@audit: the last conflicting ZK+TEE proof
            //        is chosen as the finalising
            if (_transitions[i].proofType.isZkTeeProof()) {
                fti = i;
            } else {
                ptToUpgrade = ptToUpgrade.combine(_transitions[i].proofType);
            }
        }
        ...
}
 

Recommendation

Consider handling the conflicting transitions with ZK and TEE proofs.

Resolution

We believe this does not require an immediate fix:

• While not impossible, it is improbable that a transition proven by both ZK and TEE will receive a conflict.
• Another strategy to mitigate the risk immediately on our end would be to incorporate different implementations of the execution client for ZK and TEE. For instance, Reth (Rust client) with ZK and Geth (Golang client) with TEE.

6.5 Finalization logic does not match the specification

The specification provided by the developers specifies that the ZK proof has the priority over the TEE proof. This means that if there are conflicting ZK and TEE proofs, the challenging TEE proof is ignored and the batch can be finalised by the ZK proof alone (after the cooldown of 7 days). However, the logic in the _tryFinalising(...) treats both ZK and TEE proofs as equal, meaning that if we challenge the ZK proof with the TEE proof, the protocol waits for a complementary proof or a new full (ZK+TEE) proof.

function _tryFinalising(...)
    internal
    returns (uint256)
{
    ...
    if (numTransitions == 1) {
        ...
    } else {
        // @audit: this branch is executed if
        // conflicting transitions are recorded
        
        // Proof type(s) to upgrade
        LibProofType.ProofType ptToUpgrade;

        // Try to find a finalising proof
        for (uint256 i; i < numTransitions; ++i) {
            // @audit: the transition is only considered
            // as finalising if both ZK+TEE are submitted
            if (_transitions[i].proofType.isZkTeeProof()) {
                fti = i;
            } else {
                ptToUpgrade = ptToUpgrade.combine(_transitions[i].proofType);
            }
        }

        // If no finalising transition is found, we return
        if (fti == type(uint256).max) {
            return fti;
        } else {
            // Mark non finalising verifiers for upgrade
            ISurgeVerifier(_verifier).markUpgradeable(ptToUpgrade);
        }
    }
    ...
}
 

This logic is different from the one specified in the documentation.

Recommendation

Consider aligning the implemented logic with the documentation.

Resolution

The documentation (flow diagram) precedes the code implementation. Based on later decisions, we made the implementation more generic compared to the diagram. We believe updating the diagram and polishing the documentation would be the correct approach here.

6.6 Unused pausing logic

TaikoInbox.sol

The pausing logic is implemented in the TaikoInbox, however there is no possibility of pausing the protocol. Below we list the pausing logic in the TaikoInbox:

• _pause(...) _unpause(...) functions
• paused(...) view function
• require(!stats2.paused, ContractPaused()); in proveBatches(...)
• lastUnpausedAt in inProvingWindow = block.timestamp <= uint256(meta.proposedAt).max(stats2.lastUnpausedAt) + config.provingWindow; in the proveBatches(...)
• the whenNotPaused(...) modifier

Recommendation

Consider removing unused code that handles pausing the contract.