Skip to content
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

Trust - Loss of bond amounts on re-org attacks #201

Open
sherlock-admin3 opened this issue Apr 4, 2024 · 14 comments
Open

Trust - Loss of bond amounts on re-org attacks #201

sherlock-admin3 opened this issue Apr 4, 2024 · 14 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Apr 4, 2024

Trust

high

Loss of bond amounts on re-org attacks

Summary

The move() function lacks proper identification of the target of the move, leading to successful re-org attacks which can take the honest participant's funds.

Vulnerability Detail

Participants in the game can call attack(), defend() or move(), each accepting a parentIndex which corresponds to the claim being challenged, and a _claim commitment.

When participants claim, they have a particular claim in mind which they wish to challenge, and then pass on that claim's index. However, between the moment they sent the TX and the moment that TX is executed, a block reorg can take place. When it occurs, the challenge corresponding to that ID may change to another challenge, which may be valid or invalid in a different way. Regardless, the participant's commitment to that move() will be wrong, and they stand to lose their bond amount.

Chain reorgs are very prevalent in Ethereum mainnet, where the contract is deployed. You can check this index of reorged blocks on etherscan. It is incorrect to assume the attacker will wait until it achieved finality, because there's no warnings or documentation available for them to identify this as a threat. Therefore, it remains a very valid concern with reasonable hypotheticals.

Note that in high depths, the bond amount is very large, leading to a large loss of funds.

Possible flow:

  • Attacker submits invalid claim hash
  • Honest defenders rush to prove the claim wrong (note that only first defender gets the bond, so they will rush to submit the TX. They would not be concerned about waiting against reorgs without warning)
  • A block re-org occurs
  • The attacker replaces the invalid claim with a valid claim hash
  • Defender's TXs are applied on top of the valid claim.
  • Attacker can scoop up all the defenders' bonds

Impact

Loss of bond value for honest participants of the dispute game.

Code Snippet

Tool used

Manual Review

Recommendation

Every move needs to include the key parameters which it wishes to attack/defend - the claim hash and the Position in the game tree.

@sherlock-admin2 sherlock-admin2 added Sponsor Confirmed The sponsor acknowledged this issue is valid Won't Fix The sponsor confirmed this issue will not be fixed labels Apr 9, 2024
@github-actions github-actions bot added High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Apr 11, 2024
@smartcontracts
Copy link

This is the intended behavior of the contract so we have confirmed the factuality of the report and marked as "won't fix". Challenger software can handle this case offchain.

@nevillehuang
Copy link
Collaborator

I believe this is out of scope, given there is no network admins in mainnet and thus doesn't satisfy the the requirements for the exception

Chain re-org and network liveness related issues are not considered valid.
Exception: If an issue concerns any kind of a network admin (e.g. a sequencer), can be remedied by a smart contract modification, the protocol team considers external admins restricted and the considered network was explicitly mentioned in the contest README, it may be a valid medium. It should be assumed that any such network issues will be resolved within 7 days, if that may be possible.

@nevillehuang nevillehuang removed the High A valid High severity issue label Apr 17, 2024
@sherlock-admin3 sherlock-admin3 changed the title Tart Ultraviolet Kitten - Loss of bond amounts on re-org attacks Trust - Loss of bond amounts on re-org attacks Apr 23, 2024
@sherlock-admin3 sherlock-admin3 added Non-Reward This issue will not receive a payout and removed Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Apr 23, 2024
@trust1995
Copy link
Collaborator

Escalate

The finding is in-scope as Medium severity for the following reasons:

  • The impact is direct loss of bonds of an honest challenger, who did not make any mistakes
  • There are no other preconditions except a re-org on the blockchain
  • As shown in the report, there is more than sufficient likelihood for re-orgs on ETH to render this a valuable concern that needs to be protected from
  • The issue is CLEARLY a smart contract issue, it is a lack of sufficient identification of a claim and is fixed by adding one line of code. It does not belong to the usual category of re-org issues which can be treated as unavoidable risks of blockchain architecture. The impact and circumstances are concrete and likely.
  • The challenge game is presented as a race where the first challenger picks up the bond - it is only natural that challengers will pop up as quickly as possible to challenge a honeypot claim. There are zero warnings or ways where a challenger can foresee such an attack is possible - unless we assume challengers are coding gurus which would audit the code and identify this re-org attack could steal their bonds. To be clear, a simple warning saying challengers should wait until the claim block is finalized would be sufficient to close the issue as a user-error, but that's not the case.

@sherlock-admin2
Copy link
Contributor

Escalate

The finding is in-scope as Medium severity for the following reasons:

  • The impact is direct loss of bonds of an honest challenger, who did not make any mistakes
  • There are no other preconditions except a re-org on the blockchain
  • As shown in the report, there is more than sufficient likelihood for re-orgs on ETH to render this a valuable concern that needs to be protected from
  • The issue is CLEARLY a smart contract issue, it is a lack of sufficient identification of a claim and is fixed by adding one line of code. It does not belong to the usual category of re-org issues which can be treated as unavoidable risks of blockchain architecture. The impact and circumstances are concrete and likely.
  • The challenge game is presented as a race where the first challenger picks up the bond - it is only natural that challengers will pop up as quickly as possible to challenge a honeypot claim. There are zero warnings or ways where a challenger can foresee such an attack is possible - unless we assume challengers are coding gurus which would audit the code and identify this re-org attack could steal their bonds. To be clear, a simple warning saying challengers should wait until the claim block is finalized would be sufficient to close the issue as a user-error, but that's not the case.

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.

@sherlock-admin3 sherlock-admin3 added the Escalated This issue contains a pending escalation label Apr 24, 2024
@nevillehuang
Copy link
Collaborator

Based on sherlock rules, I believe this is still invalid based on comments here.

@Evert0x
Copy link

Evert0x commented Apr 30, 2024

The following rule applies

Chain re-org and network liveness related issues are not considered valid.

Planning to reject escalation and keep issue state as is

@trust1995
Copy link
Collaborator

@nevillehuang
The rationale for the scoping rules on Sherlock excluding re-org attacks is the assumption that a TX sender is responsible for waiting for finality (stated by Judge on discord).
However, due to Optimism-specific circumstances detailed in depth by the dup submission by MiloTruck, it is proven that an honest party cannot afford to wait for finality (the so called chess-clock mechanism).
For this reason, and the fact that re-orgs on L1 are proven to be highly likely, it is only common sense to see that the issue is a valid risk of loss of funds for Medium severity.

@nevillehuang
Copy link
Collaborator

nevillehuang commented Apr 30, 2024

Hi @Evert0x although I believe this issue could still be possibly out of scope due to it being related to resolution logic, I think @trust1995 has a point here. Re-orgs are often times out of the control of a user, and in optimisms case, this directly leads to a serious inconsistency where user would dispute a claim incorrectly (although there are safeguards).

I believe that re-org issues exception could be re-considered for sherlock's scope in the future, possibly a proposal could be put up to address this per discussed, especially so when a fund loss impact is involved.

@Evert0x
Copy link

Evert0x commented May 3, 2024

I understand that re-org attacks are possibly more interesting to L1's or L2's than the average protocol. However, using the same judging rules as every Watson used during the contest, it would only be fair to invalidate it. As the language is clear

Chain re-org and network liveness related issues are not considered valid.

@trust1995
Copy link
Collaborator

@Evert0x The language is clear, but guidelines always have exceptions. It is up to the judge to apply common sense and the contest-specific context to every verdict. Blindly following every rule will lead to injustice and counterexamples where an impact is clearly real and valuable, but is not rewarded. That is well understood in the Sherlock rulebook:

Note: Despite these rules, we must understand that because of the complexity & subjective nature of smart contract security, there may be issues that are judged beyond the purview of this guide. However, for the vast majority of cases, this guide should suffice. Sherlock's internal judges continue to have the last word on considering any issue as valid or not.

The chain re-org and liveness rule already has an exception.

Exception: If an issue concerns any kind of a network admin (e.g. a sequencer), can be remedied by a smart contract modification, the protocol team considers external admins restricted and the considered network was explicitly mentioned in the contest README, it may be a valid medium. It should be assumed that any such network issues will be resolved within 7 days, if that may be possible.

The submission abides by all the criteria for that exception except the network (mainnet) does not have an admin. The intention around that criteria is that because there's no admin, we can assume actors can wait for finality before submitting a transaction, and they could not get attacked. However in the Optimism codebase, we have shown the game clock forces honest parties to respond before blocks are finalized, re-opening the vector.

It is very clear that the combination of the impact, simple code fix, execution before finality, and ease of exploit make an extremely sound case for Medium severity.

Judging is not clerk work, it requires making nuanced decisions and not continuously falling back on previous decisions, which were made with different contexts. Apply common sense, and determine if the submission is worthy of H/M.

@Evert0x
Copy link

Evert0x commented May 5, 2024

Result:
Invalid
Has Duplicates


The judges have the last of opinion but objectivity is held to a high regard. As the language is so clear, I believe it's the correct judgment

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented May 5, 2024

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin3 sherlock-admin3 removed the Escalated This issue contains a pending escalation label May 5, 2024
@sherlock-admin4 sherlock-admin4 added the Escalation Resolved This issue's escalations have been approved/rejected label May 5, 2024
@Evert0x
Copy link

Evert0x commented May 8, 2024

This was initially deemed invalid by a strict interpretation of our judging guidelines ("Chain re-org and network liveness related issues are not considered valid."). This rule exists as the "blockchain is trusted" from the perspective of app builders. However, a different trust level applies when building an L1/L2.

After a discussion with the lead judge and the protocol team I'm assigning Medium severity.

@Evert0x Evert0x reopened this May 8, 2024
@Evert0x Evert0x added the Medium A valid Medium severity issue label May 8, 2024
@sherlock-admin2 sherlock-admin2 added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels May 8, 2024
@sherlock-audit sherlock-audit deleted a comment from 54710adk341 May 12, 2024
@sherlock-admin3 sherlock-admin3 added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label May 15, 2024
@sherlock-admin2
Copy link
Contributor

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

@sherlock-admin3 sherlock-admin3 added Will Fix The sponsor confirmed this issue will be fixed and removed Won't Fix The sponsor confirmed this issue will not be fixed labels May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

7 participants