m-01 inconsistent requirement at the contract procedure at `DisputeGameFactory` contract arabgodx

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol#L84

Vulnerability details

Summary

0xaliyah arabgodx

1. inconsistent requirement at the contract procedure at DisputeGameFactory contract

Vulnerability Detail

according for the docs bot;

1. at create function the contract checks if the sent value (msg.value) matches the required initBond for the specified GameType
2. the amount of initBond is indeed specific to each game type and must be set when configuring the game implementations in the DisputeGameFactory
3. amount is required to be greater than 0 to ensure that there is a financial commitment involved in initiating a dispute game

in light of what is the given constraint then

1. L100 (line 100) does a requirements correctly. however,
2. L200 created implementation (gameImpls) given the argument; gameType without having first created required initBond for game type in prior
3. once implementation for game type exists anyone can call a create. create is indeed even acceptable provided 0*value msg.value or with no any bond committed until some intervention from owner or admin by properly set initBond
4. the _claim argument in the move function of FaultDisputeGame that relate to i.e the claim created at create function of DisputeGameFactory where the move function hasn't no any checking if _claim locked bond value is indeed is it existent at all then i.e such as the move function is unsafe due to initBond not secured correctly or create function approved non-honesty zero bond value claims

Impact

1. med impact and med likeliness owasp
2. frivolous claims will be available easily and protocol docs states that honest participant rely on the refunding of bond. additionally,
3. if a subgame root resolves correctly , the bond is returned to the claimant
4. if it is resolved incorrectly, the bond is distributed to the leftmost claimant that countered it

Code Snippet

1. poc 01
2. poc 02

Tool used

Manual Review

Recommendation

1. recommend change to

   diff --git a/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol b/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol
   index abc1234..def5678 100644
   --- a/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol
   +++ b/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol
   @@ -196,7 +196,8 @@ contract DisputeGameFactory is OwnableUpgradeable, IDisputeGameFactory, ISemver 
    /// @inheritdoc IDisputeGameFactory
    function setImplementation(GameType _gameType, IDisputeGame _impl) external onlyOwner {
   +        require(initBonds[_gameType], "initBond must be set before adding implementation");
        gameImpls[_gameType] = _impl;
        emit ImplementationSet(address(_impl), _gameType);
    }

2. also recommend

a. the time-lock mechanism for the sensitive functions b. the two-step process with the mandatory delay for the impact changes

openzeppelin consensys

Assessed type

Invalid Validation

[zobront]

This was rejected in the Sherlock contest (https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/87) but I could see an argument either way. I think it likely should be deemed out of scope since it was submitted there.

[clabby]

This report is technically valid, but we don't see it as an issue. The worst case scenario here is that a game is created without an initial bond when it is first registered, which wouldn't be the end of the world.

Going to mark this as sponsor acknowledged as I see where they're coming from, but this is likely a wontfix.

[zobront]

Reminder that this was also submitted in Oak Cobalt's QA report, so if it's deemed valid, that report should be dup'd in.

Currently leaning towards not rewarding this for the same reason it wasn't rewarded with Sherlock (requires admin error), as well as the fact that it was in the linked Known Issues repo.

[zobront]

Confirming the decision that this will be downgraded to QA for the above reasons.

[c4-judge]

zobront changed the severity to QA (Quality Assurance)

[c4-judge]

zobront marked the issue as grade-a