code-423n4 / 2024-07-optimism-findings

3 stars 0 forks source link

Frontrunning of successful `step()` and `challengeRootL2Block()` calls allows piggybacking on honest challengers' work #53

Open c4-bot-2 opened 3 months ago

c4-bot-2 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L234 https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L492

Vulnerability details

Although bonds exist in part to refund honest challengers for the gas costs incurred from countering malicious claims, they are chosen such that the bond is with overwhelming likelihood larger than said costs. Hence moves will generally result in a profit for the caller, as long as they win the subgame. From the specs:

FPM bonds are priced in the amount of gas that they are intended to cover. Bonds start at the very first depth of the game at a baseline of 400_000 gas. The 400_000 value is chosen as a deterrence amount that is approximately double the cost to respond at the top level. Bonds scale up to a value of 300_000_000 gas, a value chosen to cover approximately double the cost of a max-size Large Preimage Proposal.

In most cases, outsiders cannot steal bond payouts by frontrunning moves because they lack knowledge of the correct state. However, successful calls to step() or challengeRootL2Block() guarantee a payout when all subgames are resolved and resolveClaim() is called.

This creates an opportunity for attackers to watch the mempool and frontrun these specific calls by simulating their execution and frontrunning successful cases, allowing them to claim the payout intended for honest challengers. At the deepest level (i.e. in calls to step()), the profit will be largest (usually ~60 ETH, and at least half at a gas price of 200 gwei if they need to submit a max-size LPP).

They can do this without performing any of the work honest challengers do, and must only resolve the claim subsequently if nobody else does (which is unlikely as honest challengers will want to resolve the game to receive their bonds higher up the graph).

Impact

Proof of Concept

  1. Alice, an honest challenger, prepares a valid step() transaction that will successfully counter a claim.
  2. Bob, an attacker, monitors the mempool for step() and challengeRootL2Block() transactions.
  3. Bob sees Alice's transaction and frontruns it with an identical transaction but a higher gas price.
  4. Bob's transaction is executed first, successfully countering the claim.
  5. When resolveClaim() is called later, Bob receives the bond payout instead of Alice.

Tools Used

Manual review

Recommended Mitigation Steps

Consider any of the following mitigations:

  1. Implement a commit-reveal scheme for step() and challengeRootL2Block() calls:
    • Challengers first submit a commitment (hash) of their move.
    • After a short timelock period, they reveal their actual move.
    • Only the revealer of a successful move can claim the payout.
  2. Implement additional checks in resolveClaim() to ensure that the payout recipient has a history of honest participation in the game, rather than just being the last successful caller of step().
  3. For challengeRootL2Block(), the current implementation may be fine as the bond is small.

These solutions would make it much harder for attackers to steal payouts without actually participating in the dispute resolution process.

Assessed type

MEV

zobront commented 3 months ago

Note: Some of the dups for this focus on challengeLPP() and challengeFirstLPP() but the issue is the exact same, so considering them all dups.

zobront commented 3 months ago

Reminder that @etherSky111 had this in QA report, so should be dup'd in with this one.

ajsutton commented 3 months ago

The report is accurate. The risk of front running was known as part of designing the dispute game but not something we were concerned with. Honest actors can use a variety of services to ensure their transactions are not exposed to the public mempool before being included in a block if this is a concern for them.

zobront commented 3 months ago

I'm inclined to go with the sponsor here. It seems clear that frontrunning is possible, but also expected that users interacting with these functions are sophisticated and know to use Flashbots/similar to protect their transactions from frontrunning. I believe this is better considered as QA.

c4-judge commented 3 months ago

zobront changed the severity to QA (Quality Assurance)

c4-judge commented 3 months ago

zobront marked the issue as grade-b

0xEVom commented 3 months ago

The main purpose of this version of the fault dispute game is that the process is now permissionless. Hence it cannot be expected that all actors interacting with the system are sophisticated enough to independently expect the game to be vulnerable to frontrunning, or use private mempools by default.

The requirements to run op-challenger are best summarized here I believe, and it does not mention specific transactions would need to be submitted to block builders directly in order to prevent frontrunning.

While this finding requires explicit intent by an attacker with some understanding of the implementation and hence can be considered to be low likelihood, in the case of challengeLPP() and challengeFirstLPP() (which we submitted in https://github.com/code-423n4/2024-07-optimism-findings/issues/32 as a separate finding for this reason), those transactions can almost with certainty be expected to be frontrun. Those bonds will in all likelihood go to external actors as long as the transaction is submitted via the mempool, to the detriment of honest participants.

zobront commented 3 months ago

Hi @0xEVom — thanks for the thoughtful reply.

We spent a lot of time discussing this and it feels right on the border. Your point that a private mempool isn't mentioned anywhere in the challenger docs is a good one, and something they should fix.

That being said, we're going to stick with the original judging and award all the frontrunning findings as QA.

The main reasons for this decision are:

This could cause a user who doesn't understand the system to waste quite a bit of gas once, but the fact that the downside is limited and the the fix is simple leads us to land on the side of judging this as QA.