sherlock-audit / 2024-02-optimism-2024-judging

6 stars 4 forks source link

w42d3n - Unrestricted Creation of Games #220

Closed sherlock-admin4 closed 6 months ago

sherlock-admin4 commented 7 months ago

w42d3n

medium

Unrestricted Creation of Games

Summary

Contracts that allow arbitrary user interaction, including creation of clones, need to ensure that restrictions are in place to prevent abuse.

Vulnerability Detail

the contract DisputeGameFactory allows for the creation of Dispute Games. These games are associated with gameImpls via a mapping, which is acceptable. However, the issue lies within the create function, where there aren't enough checks and limits in place to prevent potential misuse.

Impact

The create function allows for the creation of arbitrary games, with the provided game implementation, claim and other parameters. However, a significant problem arises from the fact that there aren’t enough measures implemented to prevent misuse of the function. For example, any user could potentially flood the contract with a large number of games, possibly leading to inefficient storage and processing, increased gas costs and, at worst, a Denial of Service (DoS) if the list or mapping fills up the available space to an unmanageable level. Additionally, this could provide opportunities for front-running attacks, where users with information about future transactions can manipulate the outcome of the games.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol#L84-L107

function create(
        GameType _gameType,
        Claim _rootClaim,
        bytes calldata _extraData
    )
        external
        payable
        returns (IDisputeGame proxy_)
{
        IDisputeGame impl = gameImpls[_gameType];

        if (address(impl) == address(0)) revert NoImplementation(_gameType);

        if (msg.value != initBonds[_gameType]) revert IncorrectBondAmount();

        bytes32 parentHash = blockhash(block.number - 1);

        // problematic line: arbitrary proxy creation
        proxy_ = IDisputeGame(address(impl).clone(abi.encodePacked(_rootClaim, parentHash, _extraData)));
        proxy_.initialize{ value: msg.value }();
}

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, implemented the following measures:

Access Control: Limit who can call the create function. This could be the contract owner or a trusted group of users.

Rate Limiting: Implement a mechanism to limit the frequency of game creation by each user or in total. This could include time-based restrictions or a maximum limit on games created per block.

Game Validation: Implement further checks to validate the game parameters and ensure consistent and fair game creation.

Bonding: The function can demand a security deposit for each game creation. Improper usage would result in the forfeiture of this deposit. This deposit can be proportional to the frequency of game creation, acting as a natural disincentive for flooding the contract with games.

nevillehuang commented 6 months ago

Invalid, this is intended protocol design

Any user can trigger the DisputeGameFactory to create a new claim and a corresponding FaultDisputeGame so that any other user may argue about the validity of that claim.