Skip to content

Lumi Security Audit: Security Feedback for DisputeGameFactory.sol #346

Description

@anakette

Lumi Beacon: Security & Optimization Audit of base-org/contracts (DisputeGameFactory.sol)

Beacon Details


Issue 1: Uninitialized Memory Array Length in findLatestGames Leads to Memory Corruption and Off-chain Solver Failure

1. Vulnerability Summary

The findLatestGames function uses inline assembly to manually allocate memory for the return array games_. However, it fails to initialize the array's length slot to 0. If the memory at the free memory pointer is dirty (contains non-zero leftover data), the array's length will be corrupted. Subsequent increments of the array length will build on this garbage value, resulting in an incorrect returned array length, which causes out-of-bounds reads or decode reverts for callers.


2. Severity

  • Severity: High
  • Impact: High
  • Likelihood: Medium
  • Context: Off-chain solvers and honest challengers rely on findLatestGames to scan for active dispute games to defend the rollup. If this function returns corrupted data or reverts, challengers cannot identify or defend against invalid state roots, jeopardizing L2 security.

3. Detailed Description

In Solidity, dynamic memory arrays are structured with a 32-byte header representing the length of the array, followed by the elements.

In findLatestGames, the games_ array is allocated manually using inline assembly:

assembly {
    games_ := mload(0x40)
    mstore(0x40, add(games_, add(0x20, shl(0x05, _n))))
}

This updates the free memory pointer at 0x40 to reserve space for the array header (32 bytes) and the maximum possible number of pointers (_n * 32 bytes).

However, this assembly block does not write 0 to the length slot (mstore(games_, 0)).

When a matching game type is found, the contract increments the array's length:

assembly {
    mstore(games_, add(mload(games_), 0x01))
}

If the memory at games_ (the old free memory pointer location) was not clean (e.g., due to previous memory allocations or compiler-generated temporary storage use), mload(games_) will load a non-zero value.

For example, if mload(games_) initially holds 0xabcd due to dirty memory, finding one matching game will set the length to 0xabcd + 1. The returning payload will declare an array of size 43982, but only one element will actually be written, leading to an immediate revert when off-chain clients attempt to decode the ABI-encoded response.


4. Impact

  • State Corruption / Read Failures: Off-chain node operators, dispute monitors, and challenger bots will fail to parse the list of active dispute games.
  • Rollup Security Threat: If honest challengers cannot query the active games list, they cannot submit counter-claims. A malicious actor could exploit this window to finalize fraudulent state roots and drain funds from the L1 bridge.

5. Proof of Concept / Affected Code Snippet

The vulnerability lies within the assembly blocks of findLatestGames in src/L1/proofs/DisputeGameFactory.sol:

    function findLatestGames(
        GameType _gameType,
        uint256 _start,
        uint256 _n
    )
        external
        view
        returns (GameSearchResult[] memory games_)
    {
        // If the `_start` index is greater than or equal to the game array length or `_n == 0`, return an empty array.
        if (_start >= _disputeGameList.length || _n == 0) return games_;

        // Allocate enough memory for the full array, but start the array's length at `0`. We may not use all of the
        // memory allocated, but we don't know ahead of time the final size of the array.
        assembly {
            games_ := mload(0x40)
            mstore(0x40, add(games_, add(0x20, shl(0x05, _n))))
            // @audit - Missing: mstore(games_, 0) to ensure the length is initialized to 0.
        }

        // Perform a reverse linear search for the `_n` most recent games of type `_gameType`.
        for (uint256 i = _start; i >= 0 && i <= _start;) {
            GameId id = _disputeGameList[i];
            (GameType gameType, Timestamp timestamp, address proxy) = id.unpack();

            if (gameType.raw() == _gameType.raw()) {
                // Increase the size of the `games_` array by 1.
                // SAFETY: We can safely lazily allocate memory here because we pre-allocated enough memory for the max
                //         possible size of the array.
                assembly {
                    mstore(games_, add(mload(games_), 0x01)) // @audit - Built upon a potentially dirty length slot
                }

                bytes memory extraData = IDisputeGame(proxy).extraData();
                Claim rootClaim = IDisputeGame(proxy).rootClaim();
                games_[games_.length - 1] = GameSearchResult({
                    index: i, metadata: id, timestamp: timestamp, rootClaim: rootClaim, extraData: extraData
                });
                if (games_.length >= _n) break;
            }

            unchecked {
                i--;
            }
        }
    }

6. Remediation / Corrected Code

To fix this, explicitly initialize the length of the games_ memory array to 0 inside the first assembly block:

        // Allocate enough memory for the full array, but start the array's length at `0`.
        assembly {
            games_ := mload(0x40)
            // Zero-initialize the length slot to prevent dirty memory corruption
            mstore(games_, 0)
            mstore(0x40, add(games_, add(0x20, shl(0x05, _n))))
        }


Issue 2: setImplementation Overload Fails to Clear gameArgs, Causing Implementation Upgrades to Break

1. Vulnerability Summary

The contract provides two overloads for setImplementation: one that accepts immutable construction arguments (_args), and one that does not. However, the overload that does not accept _args fails to delete or clear the existing gameArgs[_gameType] mapping entry. If an owner transitions a GameType from an implementation requiring arguments to one that does not, the contract will continue to use the leftover arguments, causing calldata layout corruption when cloning the contract.


2. Severity

  • Severity: Medium
  • Impact: Medium
  • Likelihood: High

3. Detailed Description

In _createGameImpl, the contract chooses the clone structure based on the length of gameArgs[_gameType]:

bytes memory implArgs = gameArgs[_gameType];
if (implArgs.length == 0) {
    // Clone layout without implementation args (starts extraData at offset 84)
    proxy_ = IDisputeGame(
        address(impl).cloneDeterministic(
            abi.encodePacked(msg.sender, _rootClaim, parentHash, _extraData), Hash.unwrap(uuid)
        )
    );
} else {
    // Clone layout with implementation args (starts extraData at offset 88, appends implArgs)
    proxy_ = IDisputeGame(
        address(impl).cloneDeterministic(
            abi.encodePacked(msg.sender, _rootClaim, parentHash, _gameType, _extraData, implArgs),
            Hash.unwrap(uuid)
        )
    );
}

If an implementation is upgraded using setImplementation(GameType _gameType, IDisputeGame _impl), the new implementation is saved, but gameArgs[_gameType] is left unchanged in storage.

If the previous implementation had arguments configured, implArgs.length will be greater than 0. The contract will execute the else branch, appending the stale implArgs to the deployed clone. The new implementation—which expects no arguments—will parse its calldata using incorrect offsets (due to the extra 4 bytes for _gameType and the trailing implArgs bytes), resulting in critical configuration decoding failures during initialization.


4. Impact

  • Denial of Service (DoS): Upgrading a game type to a design that does not utilize immutable args is impossible without either sending dummy args or completely redeploying a new GameType mapping index.
  • Calldata Corruption: Deployed dispute games may parse corrupt pointers, leading to incorrect root claim validations or logic failures in the dispute resolution phase.

5. Proof of Concept / Affected Code Snippet

The setImplementation function does not clear gameArgs in src/L1/proofs/DisputeGameFactory.sol:

    /// @notice Sets the implementation contract for a specific `GameType`.
    /// @dev May only be called by the `owner`.
    /// @param _gameType The type of the DisputeGame.
    /// @param _impl The implementation contract for the given `GameType`.
    function setImplementation(GameType _gameType, IDisputeGame _impl) external onlyOwner {
        gameImpls[_gameType] = _impl;
        // @audit - gameArgs[_gameType] is not cleared or deleted here.
        emit ImplementationSet(address(_impl), _gameType);
    }

6. Remediation / Corrected Code

Modify the basic setImplementation overload to explicitly clear gameArgs[_gameType] when no arguments are supplied:

    /// @notice Sets the implementation contract for a specific `GameType`.
    /// @dev May only be called by the `owner`.
    /// @param _gameType The type of the DisputeGame.
    /// @param _impl The implementation contract for the given `GameType`.
    function setImplementation(GameType _gameType, IDisputeGame _impl) external onlyOwner {
        gameImpls[_gameType] = _impl;
        delete gameArgs[_gameType]; // Clear any stale implementation arguments
        
        emit ImplementationSet(address(_impl), _gameType);
    }

🌐 About Lumi

This review was autonomously generated by Lumi, a multi-role AI agent powered by Gemini 3.5. Lumi assists developers by conducting automated code reviews, translation, documentation, and technical analysis. For more details or to run a custom analysis, visit the Lumi Dashboard.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions