guhu95 - Blacklisting a malfunctioning game may still prevent reproving if `status()` reverts

[sherlock-admin4]

guhu95

medium

Blacklisting a malfunctioning game may still prevent reproving if status() reverts

Summary

In the OptimismPortal2.proveWithdrawalTransaction reproving a withdrawal is allowed if the old dispute game associated with the withdrawal is blacklisted for any reason. However, an external call is made to the blacklisted game's contract, so it is still relied on to function correctly, not reverting, and returning sufficient data. If this call reverts, or its return data does not decode correctly, it can prevent the reproving of the withdrawal. This undermines the safety mechanism provided by the blacklisting functionality.

Vulnerability Detail

During the re-proving checks the blacklisted status of the oldGame is checked after a call is made to it's status() function. As this may revert, either during the external call, or during decoding of the return data, the safety mechanism provided by blacklisting is ineffective.

If a game is blacklisted, it is no longer trusted, and it is possible that the blacklisted implementation may now revert when the status() function is called, or not match the interface (e.g., return no data). It is possible that this is what caused the blacklisting of a malfunctioning game in the first place. One possible scenario is if the implementation itself was upgradable, and was incorrectly upgraded.

Impact

1. This issue undermines a key safety mechanism, which according to the contest rules is the main focus of the audit:

In the readme: "Our primary goal with this particular contest is to gain confidence in the correctness of these safety mechanisms"

In the handbook docs: "care has been taken to create fallbacks and safety nets designed to guarantee safety if these complex components fail. As a result, the only focus of this particular contest is on these safety nets and on the points where the fault proving system integrates with existing smart contracts"

2. DoS, and breaking core contract functionality due to the user not being able to re-prove their withdrawal despite the old game being blacklisted.

Code Snippet

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

Tool used

Manual Review

Recommendation

To fix this vulnerability, the disputeGameBlacklist should be checked before calling the status() function on the old game. This way, even if the blacklisted game's implementation reverts, it won't prevent the reproving of the withdrawal.

require(  
-    provenWithdrawal.timestamp == 0 || oldGame.status() == GameStatus.CHALLENGER_WINS  
-        || disputeGameBlacklist[oldGame] || oldGame.gameType().raw() != respectedGameType.raw(),  
+    provenWithdrawal.timestamp == 0 || disputeGameBlacklist[oldGame]  
+        || oldGame.status() == GameStatus.CHALLENGER_WINS || oldGame.gameType().raw() != respectedGameType.raw(),  
    "OptimismPortal: withdrawal hash has already been proven, and the old dispute game is not invalid"  
);

[nevillehuang]

Invalid, if oldGame is blacklisted (i.e disputeGameBlacklist[oldGame] == true), it will also not revert. Don't believe the order of check matters here.

[guhu95]

@nevillehuang I believe this was incorrectly rejected.

Don't believe the order of check matters here.

The order matters because || short-circuits on any true operand. Please see the POC below (can be run in remix).

pragma solidity 0.8.15;

contract Test {
    function orderOfChecksOk() external {
        // this order will short-circuit before the reverting call
        require(true || this.revertingCall() == 0, "ok");
    }

    function orderOfChecksBad() external {
        // this order will revert
        require(this.revertingCall() == 0 || true , "bad");
    }

    function revertingCall() external returns (uint) {
        revert();
    }
}

So if oldGame.status() reverts, even though the game is blacklisted, proveWithdrawalTransaction will revert. This makes the blacklisting safety mechanism ineffective.

[zzykxx]

Escalate

Escalating on behalf of @guhu95, refer to his comment.

[sherlock-admin2]

Escalate

Escalating on behalf of @guhu95, refer to his comment.

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.

[zobront]

Note, this rejected issue is a dup making the same point: https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/89

[guhu95]

I think #89 was also incorrectly rejected:

Per sherlock rules, the self-destruct vector must have resulted from a future integration to allow the issue to occur, that would make this issue out of scope and invalid.

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.

First, this is in scope because the focus of the contest is the safety mechanisms, under the assumption of faulty / buggy game:

Given the relative complexity of the onchain and offchain fault proving software, much care has been taken to create fallbacks and safety nets designed to guarantee safety if these complex components fail. As a result, the only focus of this particular contest is on these safety nets and on the points where the fault proving system integrates with existing smart contracts.

The kind of bug / issue that is assumed is not restricted in any way, and so can and should be any kind, to ensure that the safety mechanisms work in all cases. This should include reverting calls due to reasons like buggy implementation / incorrect return data / corrupt upgrade of implementation / self-destruct / etc.

Second, in this case the docs/README also explicitly allow "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.

[guhu95]

Please note also that it's not only about reverts. As mentioned in the finding:

If this call reverts, or its return data does not decode correctly, it can prevent the reproving of the withdrawal

Even if oldGame.status() itself doesn't revert, its return data could cause a revert if it can't be ABI decoded according to the expected interface.

This could happen as a consequence of a bug in the game's implementation which is assumed to be buggy and unreliable as part of the contest rules.

[nevillehuang]

@guhu95 @zobront My apologies for the wrong comment highlighted above.

However, this issue still sounds like speculation to me, because I don't see any current logic/exploit pointing to the following possibility, and it seems like the example is pointing to an admin upgrade mistake?

If a game is blacklisted, it is no longer trusted, and it is possible that the blacklisted implementation may now revert when the status() function is called, or not match the interface (e.g., return no data). It is possible that this is what caused the blacklisting of a malfunctioning game in the first place. One possible scenario is if the implementation itself was upgradable, and was incorrectly upgraded.

89 highlights the possibility of selfdestruct so the oldGame could have no code logic and hence reverts, but how would that be possible currently other than again, just speculating that the attacker can perform the attack in the future?

Assuming such an exploit is possible on any future game, the result will be that provenWithdrawal.timestamp > 0, and

[guhu95]

this issue still sounds like speculation to me

@nevillehuang I agree that this issue is not about a concrete exploit of the game in FaultDisputeGame.sol - that game cannot be upgraded, cannot self-destruct, and cannot return unsuitable data.

Instead, this issue is only about OptimismPortal2's blacklisting safety-mechanism being inadequate for games other than FaultDisputeGame. Considering "other games" that will be integrated with portal / factory is necessarily speculation. However, since the audit was to ensure the safety-mechanisms are suitable to other games and possible integrations, I understand this "speculation" was what the project team intended for OptimismPortal2. Additionally, I've quoted above from the README regarding "future integrations" being explicitly in scope for OptimismPortal2, which again presumes speculation.

it seems like the example is pointing to an admin upgrade mistake

highlights the possibility of selfdestruct so the oldGame could have no code logic and hence reverts

While #89 focuses on selfdestruct as the scenario, I believe it's a subset of what my issue is describing. I describe it from the point of view of the portal: the call will revert either if the call's stack output (success) is false, or if the return data cause a decoding error.

While I mention one family of scenarios (a bad upgrade), it includes both a self-destruct (e.g., of a UUPS proxy implementation) for any reason (e.g., uninitialized implementation constructor of a UUPS), but also any other reason that the code would start reverting - a beacon or transparent proxy pointing at empty code, or code that doesn't include the expected selector after upgrade, or code that returns no data for that selector to be decoded, or code that panics, or runs OOG, or reverts due to a bad dependency of its own.. The scenarios in which an external calls reverts or fails decoding are numerous and I felt that giving one family of examples that covers many of them was sufficient. Most of the above would not be admin mistake, but rather bugs or faults of different kinds. The key to assume these faults was in the fact that the game was already blacklisted. The fact that it's blacklisted presumes something is wrong with it - so relying on external calls to it is a mistake.

To me it appeared that explaining that the external call to an already blacklisted game can fail and the impact it can have was sufficient, and describing the details of exactly why it fails was not part of the vulnerability in OptimismPortal2.

[Evert0x]

I believe this is report is a design recommendation to mitigate errors in a future game contract.

[Evert0x]

Planning to reject escalation and keep issue as is

[guhu95]

To me low severity makes more sense than invalid. However, I'm not sure if that distinction matters in any way.

My reasoning for low is that it's an actual bug in OptimismPortal2 (not in a future game contract), but with low impact due to existing workaround. The fact that it will be trigerred only for future integrations is allowed in the README (for OptimismPortal2 contract only).

The reason it was submitted and escalated as medium was due to the impact on blacklisting as a "safety mechanism", which I interpreted as being separately prioritized.

[Evert0x]

Low/invalid is the same severity category. As we only look at Medium and High

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• zzykxx: rejected