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

6 stars 4 forks source link

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

Open sherlock-admin3 opened 7 months ago

sherlock-admin3 commented 7 months ago

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:

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.

smartcontracts commented 7 months ago

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 commented 6 months ago

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.

trust1995 commented 6 months ago

Escalate

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

sherlock-admin2 commented 6 months ago

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.

nevillehuang commented 6 months ago

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

Evert0x commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

Evert0x commented 6 months ago

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.

sherlock-admin2 commented 5 months ago

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