-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
obront - Incorrect game type can be proven and finalized due to unsafe cast #84
Comments
We see this as a valid medium severity issue |
The protocol team fixed this issue in the following PRs/commits: |
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
I believe this is a medium severity issue given the following constraints:
|
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. |
Three additional points to support the escalation in favor of high:
1. Games types are not sequential
This appears to be neither a constraint nor a condition:
2. The DoS impact of mitigation qualifies as high
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:
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
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). |
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:
|
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, |
@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. |
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.
This is further confirmed by their docs which outline the intended structure of the GameID:
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:
|
The project clearly decided (before this contest) that game types values higher than
The team's intention (and explicit previous switch) to use This finding justifies high severity for both the unconditionally broken key safety mechanism of |
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.
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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 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 |
Regarding:
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 Second, even if it was an enum, in this case the README explicitly allows "future integrations issues" for
|
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:
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 Making sure you've seen this comment from OptimismPortal2:
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. |
@nevillehuang in addition to the above consideration of off-chain watchers, please also consider the DoS impact of mitigation described above in #84 (comment). I am no expert on Sherlock rules, but to me the DoS impact appears to also qualify for high severity. |
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. |
Please help me understand why all of
Using all of |
@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. |
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. |
This comment was marked as resolved.
This comment was marked as resolved.
I see one fact wrong in your comment @0xjuaan. We can see in the PR with the fix that casting and this part of in-code documentation was updated:
|
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 |
@nevillehuang the last part of that README sentence is important for this discussion:
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. |
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. 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
|
@Evert0x I'm not sure how you think about likelihood vs severity, but for what it's worth, I see this as:
|
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. |
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:
2. 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
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABL3ULEGKDC4LXLA4GJZCFLZAJOKBAVCNFSM6AAAAABFXP4F72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQHA3DKMZUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
This comment was marked as duplicate.
This comment was marked as duplicate.
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. |
@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? |
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 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. |
This is information from the op handbook
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.
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 |
@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).
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. |
@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? |
The full scope quote is this (emphasis mine):
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.
|
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
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABL3ULAFT2MO5GJTVSFCBFLZBE6ATAVCNFSM6AAAAABFXP4F72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJZGI4TSOJYHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
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. |
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. |
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. |
@Evert0x How can there be an extensive limitation if one thing directly causes the other? |
Result: @guhu95 assuming "the game's resolution logic is broken" and assuming "the team will continuously deploy new game types" are two different things. |
Escalations have been resolved successfully! Escalation status:
|
@Evert0x your response:
Did not answer either of the specific questions I've asked.
I can see the escalation shows as resolved now, but #201 was re-opened 3 days after escalation resolution, so not sure how to interpret the resolution update. |
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 thegameType
to auint8
instead of auint32
, 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 currentrespectedGameType
in the Portal to confirm the game is valid.GameType is represented as a
uint32
, allowing numbers up to2 ** 32 - 1
.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
:This means that for any
oldGameType % 256 == X
, anynewGameType % 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: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 aDEFENDER_WINS
state, even when it is not correct.At a future date, when the current game type is
256
, a user creates a game withgameType = 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: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
The text was updated successfully, but these errors were encountered: