Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

obront - Deposits are not guaranteed to be reflected within sequencing window #106

Open
github-actions bot opened this issue Feb 20, 2023 · 5 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Reward A payout will be made for this issue Specification An issue related to the specification (low severity)

Comments

@github-actions
Copy link

obront

low

Deposits are not guaranteed to be reflected within sequencing window

Summary

There is an inconsistency between the spec and the code regarding guarantees for deposits.

Vulnerability Detail

In the spec, it states:

Deposits are guaranteed to be reflected in the L2 state within the sequencing window.

However, until fraud proofs are implemented, there are no guarantees that this will actually be the case.

Impact

Users may expect that there are guarantees in the system that ensure their deposits will be processed within a given number of blocks, but these guarantees do not exist yet.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/develop/specs/overview.md#l1-components

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L160-L210

Tool used

Manual Review

Recommendation

Remove this language from the spec until fraud proofs are live.

@rcstanciu
Copy link
Contributor

Comment from Optimism


Description: Deposits are not guaranteed without fault proofs

Reason: This is actually guaranteed, and part of the protocol definition for batch derivation.

@sherlock-admin sherlock-admin added the Non-Reward This issue will not receive a payout label Feb 21, 2023
@zobront
Copy link
Collaborator

zobront commented Feb 23, 2023

Escalate for 250 USDC

This issue is a valid Low severity, and should be included.

Optimism's response was:

Reason: This is actually guaranteed, and part of the protocol definition for batch derivation.

However, that isn't the case. In my conversation with Maurelian over DMs during the contest, he explained: "basically, you could say that the protocol guarantees this, based on the execution rules in the sequencer. However that 'guarantee' is based on the assumption that we have a fault proof working, which would enforce those execution rules."

Since we don't have a fault proof working, we do not have a guarantee.

@sherlock-admin
Copy link
Contributor

Escalate for 250 USDC

This issue is a valid Low severity, and should be included.

Optimism's response was:

Reason: This is actually guaranteed, and part of the protocol definition for batch derivation.

However, that isn't the case. In my conversation with Maurelian over DMs during the contest, he explained: "basically, you could say that the protocol guarantees this, based on the execution rules in the sequencer. However that 'guarantee' is based on the assumption that we have a fault proof working, which would enforce those execution rules."

Since we don't have a fault proof working, we do not have a guarantee.

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Feb 23, 2023
@Evert0x
Copy link
Contributor

Evert0x commented Mar 2, 2023

Escalation accepted.

The specification is correct as it related the L2 state ( not the L1 representation for the L2 state ) but the context of the specification can be improved.

@sherlock-admin
Copy link
Contributor

Escalation accepted.

The specification is correct as it related the L2 state ( not the L1 representation for the L2 state ) but the context of the specification can be improved.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation labels Mar 2, 2023
@Evert0x Evert0x reopened this Mar 2, 2023
@Evert0x Evert0x added the Specification An issue related to the specification (low severity) label Mar 2, 2023
@rcstanciu rcstanciu added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Reward A payout will be made for this issue Specification An issue related to the specification (low severity)
Projects
None yet
Development

No branches or pull requests

4 participants