Skip to content

Commit

Permalink
fix: handle feedbacks
Browse files Browse the repository at this point in the history
  • Loading branch information
leruaa committed Feb 10, 2025
1 parent f6e5bf8 commit 8846b49
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 22 deletions.
14 changes: 0 additions & 14 deletions security/fma-generic-hardfork.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,3 @@ Since there is no chain halt, we can just live with it and fix it in an upcoming
- [ ] ACTION ITEM (BLOCKING): We have implemented extensive cross-client / differential testing of the new functionality.
- **Detection:** Replicas of one kind of client will diverge from the sequencer
- **Recovery Path(s)**: Most likely we would have the op-node/op-geth chain be the canonical one as this is the reference implementation. Other clients would need to be patched to resolve any discrepancies.

### Operator fee: `L1Block` badly hydrated

- **Description:** At each hardfork, new data can be add to the `L1Block` contract, and the method called to hydrate it change (for instance
`setL1BlockValuesEcotone` to `setL1BlockValuesIsthmus`). If there is a bug in a future method ending up to operator fee params no
longer being updated in the `L1Block` contract, the operator fee will no longer be taken into account in transactions fee.
- **Risk Assessment:** medium severity / low likelihood
- **Mitigations:**
Add end to end tests to ensure that the operator fee is taken into account in transactions fee.
- **Detection:**
Monitor the operator fee vault balance and alert if it's no longer increasing for every transactions.
- **Recovery Path(s):**
- If the bug is located in op-node, a new version must be deployed.
- If the bug is located in the `L1Block` contract, the contract must be upgraded to fix the bug.
32 changes: 24 additions & 8 deletions security/fma-operator-fee.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
| Created at | 2025-01-08 |
| Initial Reviewers | |
| Need Approval From | maurelian |
| Status | Draft |
| Status | In Review |

## Introduction

Expand All @@ -43,9 +43,9 @@ Below are references for this project:
- **Risk Assessment:**
High impact, low likelihood.
**Mitigations:**
Every update to the operator fee scalars should be carefully tested and reviewed before deployment.
Before setting or updating the operator fee params, the operator should carefully read the [corresponding specs](https://specs.optimism.io/protocol/isthmus/exec-engine.html#operator-fee) and simulate the impact of operator fee on the whole transaction cost.
- **Detection:**
Monitoring gas cost estimation.
Monitor the transaction costs and alert if it's above a threshold.
- **Recovery Path(s)**:
If the operator fee parameters are set to unreasonable values, the rollup operator should update the `operatorFeeScalar` and `operatorFeeConstant` to reasonable values as soon as possible.

Expand All @@ -69,9 +69,13 @@ Below are references for this project:
- **Risk Assessment:**
Medium impact, low likelihood.
- **Mitigations:**
Extensive testing of receipt hydration with various transaction types and fee configurations. Ensure backwards compatibility with existing receipt formats.
Action and E2E tests covering the receipt hydration logic has been added.

* [Fee E2E test](https://github.com/ethereum-optimism/optimism/blob/develop/op-e2e/system/fees/fees_test.go)
* [Operator Fee Constistency action test](https://github.com/ethereum-optimism/optimism/blob/develop/op-e2e/actions/proofs/operator_fee_test.go)

- **Detection:**
Monitor transaction receipts and compare reported fees with expected calculations. Watch for discrepancies in accounting systems.
The action or E2E tests or local testing may pick up an issue.
- **Recovery Path(s):**
Deploy fix for receipt hydration logic. Historical receipts will remain incorrect but can be recalculated using on-chain data if needed.

Expand All @@ -90,10 +94,22 @@ Below are references for this project:
- Use archive nodes to maintain historical data.
- Consider implementing receipt compression retroactively if needed.

### Generic items we need to take into account: `L1Block` badly hydrated

- **Description:** At each hardfork, new data can be add to the `L1Block` contract, and the method called to hydrate it change (for instance
`setL1BlockValuesEcotone` to `setL1BlockValuesIsthmus`). If there is a bug in a future method ending up to operator fee params no
longer being updated in the `L1Block` contract, the operator fee will no longer be taken into account in transactions fee.
- **Risk Assessment:** medium severity / low likelihood
- **Mitigations:**
The [Operator Fee Constistency](https://github.com/ethereum-optimism/optimism/blob/develop/op-e2e/actions/proofs/operator_fee_test.go ) action test runs with all known hardforks activated at genesis, and checks that operator fee parameters are correctly reported to the `L1Block` contract.
- **Detection:**
The action or E2E tests or local testing may pick up an issue.
- **Recovery Path(s):**
- If the bug is located in op-node, a new version must be deployed.
- If the bug is located in the `L1Block` contract, the contract must be upgraded to fix the bug.

## Action Items

Below is what needs to be done before launch to reduce the chances of the above failure modes occurring, and to ensure they can be detected and recovered from:

- [ ] Resolve all comments on this document and incorporate them into the document itself (Assignee: document author)
- [ ] _Action item 2 (Assignee: tag assignee)_
- [ ] _Action item 3 (Assignee: tag assignee)_
- [ ] Coordinate with wallet providers to update their fee estimation logic

0 comments on commit 8846b49

Please sign in to comment.