From 8a7c366f308e03988adc5e6e358573fd061e02e3 Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Fri, 10 Jan 2025 15:31:44 -0300 Subject: [PATCH 1/7] feat: add fma ban deposits --- security/fma-ban-deposits.md | 102 +++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 security/fma-ban-deposits.md diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md new file mode 100644 index 00000000..476dd4cf --- /dev/null +++ b/security/fma-ban-deposits.md @@ -0,0 +1,102 @@ +# Ban `ExecutingMessage` Deposits: Failure Modes and Recovery Path Analysis + +| Author | Skeletor, Parti, Joxes | +| --- | --- | +| Created at | 2025-01-10 | +| Initial Reviewers | Pending | +| Needs Approval From | Pending | +| Status | In Review | + +## Introduction + +This document is intended to be shared publicly for reviews and visibility purposes. It covers the changes introduced to ban calls to `validateMessage` in the `CrossL2Inbox` within a deposit. These changes involve contracts and client: + +- **Contracts**: + - Updates to the `CrossL2Inbox` to revert on deposit transactions. + - Updates to the `L1BlockInterop` to add `isDeposit`, `depositsComplete`, and `setL1BlockValuesInterop` external functions. + +- **Client**: + - Updates to the `PreparePayloadAttributes` function within `derive/attributes.go`. + - Adds `AfterForceIncludeSource` to `derive/deposit_source.go`. + - Updates to the `derive/l1_block_info.go` to comply with the new contract changes specified above. + +Below are references for this project: +- Both Contract and Client updates are documented in the following specs: + - [interop: Specify deposit handling #258](https://github.com/ethereum-optimism/specs/pull/258). +- Reference implementation and tests can be found in the following PR: + - [feat: ban deposits interop #11712](https://github.com/ethereum-optimism/optimism/pull/11712) + +Note that this FMA doesn’t intend to cover the main features of core interop contracts for message passing. + +> 🗂️ **For more context about the Interop project, refer to the following docs:** +> 1. [Interop System Diagram](https://www.notion.so/Superchain-Interop-16c8052fcbb24b93ad1a539b5f8db4c1?pvs=21) +> 2. [Interop PID](https://www.notion.so/Superchain-Interop-16c8052fcbb24b93ad1a539b5f8db4c1?pvs=21) +> 3. [Interop Audit Request](https://docs.google.com/document/d/1Rcuzbsguh7koT2jFru5ft9T8zAvjBEzbt0zF5LNQQ08/edit?tab=t.0) + +## Failure Modes and Recovery Paths + +### FM1: An `ExecutingMessage` is emitted even if the `isDeposit` flag is correctly set + +- **Description:** If a deposit transaction calls `validateMessage` without reverting and emitting the event `ExecutingMessage`, the sequencer could be forced to include messages that do not correspond to an existing identifier. This could break multiple interop invariants and eventually lead to a bricked chain. +- **Risk Assessment:** Medium. + - Potential Impact: High. The impact of such an attack would vary depending on the message. Examples of economic attacks include: + - Releasing all ETH from the `ETHLiquidity` contract. + - Relaying an ERC20 with a custom amount never burned on the source chain. + + In the worst-case scenario, the chain could produce unsafe blocks that are subsequently rejected, leading to a reorg, which would effectively brick the chain afterwards. This occurs because the chain cannot progress with a deposit transaction containing an invalid execution. Services such as relay-based bridges could also face significant consequences if they fail to detect the situation promptly. If the `op-supervisor` can stop such deposits, the chain can continue, at the impact of halting deposits, until [sequencing windows](https://specs.optimism.io/glossary.html?highlight=sequencing%20window#sequencing-window) end. + + - Likelihood: Low. This could only happen with a bugged implementation of the `CrossL2Inbox` check. Even if the current implementation is bug-free, future upgrades can introduce these bugs. + Something important to notice is that most chains within the same cluster will probably share implementations, so a bug might affect all chains. +- **Mitigations:** The security team should be aware of this issue and check for it in every protocol version upgrade. +- **Detection:** The `op-supervisor` should detect such a situation within the invariants checks. If `op-supervisor` simulations are properly implemented before including new deposits, it would allow the sequencer to delay the deposit’s inclusion until the sequencing window ends. This delay would provide a time buffer to fix the issue. +- **Recovery Path(s):** Stop processing deposits and stay aware of the sequencing window. A chain halt followed by a fix and a reorg would be necessary. + +### FM2: `isDeposit` is not turned on before deposit transactions + +- **Description:** This would allow deposit transactions to emit `ExecutingMessage` events, even if the check in the `CrossL2Inbox` is working correctly. +- **Risk Assessment:** Medium. + - Potential Impact: High. All the consequences from FM1 would apply. + - Likelihood: Low. This would also correspond to a bugged implementation in the `L1Block` contract or client-triggered calls to it. +- **Mitigations:** The security team should know this issue and check for it in every protocol version. +- **Detection:** Same as FM1. Off-chain services can detect flag misbehavior by checking every block's first transaction. +- **Recovery Path(s):** Stop processing deposits and stay aware of the sequencing window. A chain halt followed by a fix would be necessary. A reorg can also be considered if invariant breaking messages were created. + +### FM3: `isDeposit` is not turned off after deposit transactions + +- **Description:** If the `depositComplete()` call within `L1BlockInterop` fails or is never initiated, the `isDeposit` flag might remain on. This would imply that every call to `validateMessage()` will be seen as a deposit and therefore revert. +- **Risk Assessment:** Low. + - Potential Impact: Medium. Genuine cross-chain messages will not be able to execute. This should not be a major issue, as users can re-execute after the fix. + - Likelihood: Low. This could happen if the `depositComplete()` implementation is bugged or the sequencer is not triggering the call to the function. The latter could occur due to a client bug or an out-of-gas error, which is unlikely. +- **Mitigations:** The security team should know this issue and check for it in every protocol version. +- **Detection:** Offchain services should be aware of this possibility for `validateMessage` reverts. +- **Recovery Path(s):** Execute the proper fixes depending on whether it was a sequencer or contract error. Valid reverted messages can be re executed on destination, or resent if expired. + +### FM4: `upgradeTxs` include an invalid `ExecutingMessage` + +- **Description:** The `isDeposit` bool is set off before the upgrade transactions, which are force-included. This implies that, if an upgrade transaction includes a call to `validateMessage`, the sequencer will be forced to include it, even if it doesn't point to an existing identifier. +- **Risk Assessment:** Low to Medium. + - Potential Impact: High. It could impact the same way described in the first failure (TODO: add link). + - Likelihood: Low. An upgrade transaction should not call `validateMessage()` unless it is somehow intended to (and therefore not malicious). What's more, every upgrade transaction should bypass many security checks. +- **Mitigations:** Every upgrade transaction should be simulated. An invalid Superchain state would be caught by the `op-supervisor` in simulations. +- **Detection:** Upgrades are heavily monitored transactions, making it very unlikely to go unnoticed. +- **Recovery Path(s):** Recovery would be similar to other bugged upgrades. See [Generic Hardfork FMA](https://github.com/ethereum-optimism/design-docs/blob/main/security/fma-generic-hardfork.md) for more details. + +### Generic items we need to take into account: + +- Every consideration already covered by the [Generic Hardfork FMA](https://github.com/ethereum-optimism/design-docs/blob/main/security/fma-generic-hardfork.md) will also apply to these changes. +- It is important to have a good gas benchmark for `L1Block.depositsComplete()` in the client to minimize out of gas errors. + +## Action Items + +- [ ] Resolve all the comments. +- [ ] Moving into testing of all the components. +- [ ] Make sure `op-supervisor`, and relevant off-chain components are properly put in place to monitor and cover all the cases, being ready before to this implementation goes to production. + +## Audit Requirements + +We suggest the modifications for banning deposit triggering `ExecutingMessage` events go through an audit, as they affect a sensitive part of the protocol. Following on the [Audit Framework](https://gov.optimism.io/t/op-labs-audit-framework-when-to-get-external-security-review-and-how-to-prepare-for-it/6864), some presented failure modes are similar to the ”Deposit path no spoofing”, as it affects the validity of the deposits. In particular, referencing a non-existing cross-chain message with a deposit can be considered spoofing. + +## Additional Notes + +Something worth noticing is that implementing the ban deposit closes the door for intended `ExecutingMessage` emissions from deposits. This feature could be desirable to force valid cross-chain messages from L1 and bypass sequencer censorship. +We could consider developing a new type of deposit that can prove the initiating message's validity and trigger the `ExecutingMessage` event at some point in the future. From 649e9573a1e9c4d777dda02e0fd9337a4ec40c4b Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Fri, 24 Jan 2025 10:50:20 -0300 Subject: [PATCH 2/7] update to include improvements and minor fixes --- security/fma-ban-deposits.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md index 476dd4cf..31f1d96e 100644 --- a/security/fma-ban-deposits.md +++ b/security/fma-ban-deposits.md @@ -4,7 +4,7 @@ | --- | --- | | Created at | 2025-01-10 | | Initial Reviewers | Pending | -| Needs Approval From | Pending | +| Needs Approval From | Kelvin Fichter | | Status | In Review | ## Introduction @@ -24,7 +24,8 @@ Below are references for this project: - Both Contract and Client updates are documented in the following specs: - [interop: Specify deposit handling #258](https://github.com/ethereum-optimism/specs/pull/258). - Reference implementation and tests can be found in the following PR: - - [feat: ban deposits interop #11712](https://github.com/ethereum-optimism/optimism/pull/11712) + - `CrossL2Inbox`: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol + - `L1BlockInterop`: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L2/L1BlockInterop.sol Note that this FMA doesn’t intend to cover the main features of core interop contracts for message passing. @@ -43,12 +44,12 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - Releasing all ETH from the `ETHLiquidity` contract. - Relaying an ERC20 with a custom amount never burned on the source chain. - In the worst-case scenario, the chain could produce unsafe blocks that are subsequently rejected, leading to a reorg, which would effectively brick the chain afterwards. This occurs because the chain cannot progress with a deposit transaction containing an invalid execution. Services such as relay-based bridges could also face significant consequences if they fail to detect the situation promptly. If the `op-supervisor` can stop such deposits, the chain can continue, at the impact of halting deposits, until [sequencing windows](https://specs.optimism.io/glossary.html?highlight=sequencing%20window#sequencing-window) end. + In the worst-case scenario, the chain could produce unsafe blocks that are subsequently rejected, leading to a reorg, which would effectively brick the chain afterwards. This occurs because the chain cannot progress with a deposit transaction containing an invalid execution. Services such as relay-based bridges could also face significant consequences if they fail to detect the situation promptly. - Likelihood: Low. This could only happen with a bugged implementation of the `CrossL2Inbox` check. Even if the current implementation is bug-free, future upgrades can introduce these bugs. Something important to notice is that most chains within the same cluster will probably share implementations, so a bug might affect all chains. -- **Mitigations:** The security team should be aware of this issue and check for it in every protocol version upgrade. -- **Detection:** The `op-supervisor` should detect such a situation within the invariants checks. If `op-supervisor` simulations are properly implemented before including new deposits, it would allow the sequencer to delay the deposit’s inclusion until the sequencing window ends. This delay would provide a time buffer to fix the issue. +- **Mitigations:** Our current codebase includes tests to check that the `CrossL2Inbox` contract validate messages only when `isDeposit` is `false` ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol#L139)) and revert when it is `true` ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol#L166)). The `L1BlockInterop` contract also includes test to check this property ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L205)). The security team should be aware of this issue and check for it in every protocol version upgrade. There should be a way to prevent to process such deposits (through `op-supervisor` or other), so the chain can continue, at the impact of halting deposits, until [sequencing windows](https://specs.optimism.io/glossary.html?highlight=sequencing%20window#sequencing-window) end. +- **Detection:** The `op-supervisor` could detect such a situation within the invariants checks. If `op-supervisor` simulations (or if there are tools for local sequencers to simulate deposits) are properly implemented before including new deposits, it would allow the sequencer to delay the deposit’s inclusion until the sequencing window ends. This delay would provide a time buffer to fix the issue. - **Recovery Path(s):** Stop processing deposits and stay aware of the sequencing window. A chain halt followed by a fix and a reorg would be necessary. ### FM2: `isDeposit` is not turned on before deposit transactions @@ -57,7 +58,7 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - **Risk Assessment:** Medium. - Potential Impact: High. All the consequences from FM1 would apply. - Likelihood: Low. This would also correspond to a bugged implementation in the `L1Block` contract or client-triggered calls to it. -- **Mitigations:** The security team should know this issue and check for it in every protocol version. +- **Mitigations:** Our current codebase include tests to check `L1BlockInterop` set `isDeposit` as `true` during the deposit context ([reference](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L239)). The security team should know this issue and check for it in every protocol version. - **Detection:** Same as FM1. Off-chain services can detect flag misbehavior by checking every block's first transaction. - **Recovery Path(s):** Stop processing deposits and stay aware of the sequencing window. A chain halt followed by a fix would be necessary. A reorg can also be considered if invariant breaking messages were created. @@ -67,7 +68,7 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - **Risk Assessment:** Low. - Potential Impact: Medium. Genuine cross-chain messages will not be able to execute. This should not be a major issue, as users can re-execute after the fix. - Likelihood: Low. This could happen if the `depositComplete()` implementation is bugged or the sequencer is not triggering the call to the function. The latter could occur due to a client bug or an out-of-gas error, which is unlikely. -- **Mitigations:** The security team should know this issue and check for it in every protocol version. +- **Mitigations:** Our current codebase includes tests to check `L1BlockInterop` set `isDeposit` as `false` after the deposit context ends ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L292)). The security team should know this issue and check for it in every protocol version. - **Detection:** Offchain services should be aware of this possibility for `validateMessage` reverts. - **Recovery Path(s):** Execute the proper fixes depending on whether it was a sequencer or contract error. Valid reverted messages can be re executed on destination, or resent if expired. @@ -75,7 +76,7 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - **Description:** The `isDeposit` bool is set off before the upgrade transactions, which are force-included. This implies that, if an upgrade transaction includes a call to `validateMessage`, the sequencer will be forced to include it, even if it doesn't point to an existing identifier. - **Risk Assessment:** Low to Medium. - - Potential Impact: High. It could impact the same way described in the first failure (TODO: add link). + - Potential Impact: High. It could impact the same way described in FM1. - Likelihood: Low. An upgrade transaction should not call `validateMessage()` unless it is somehow intended to (and therefore not malicious). What's more, every upgrade transaction should bypass many security checks. - **Mitigations:** Every upgrade transaction should be simulated. An invalid Superchain state would be caught by the `op-supervisor` in simulations. - **Detection:** Upgrades are heavily monitored transactions, making it very unlikely to go unnoticed. @@ -89,8 +90,7 @@ Note that this FMA doesn’t intend to cover the main features of core interop c ## Action Items - [ ] Resolve all the comments. -- [ ] Moving into testing of all the components. -- [ ] Make sure `op-supervisor`, and relevant off-chain components are properly put in place to monitor and cover all the cases, being ready before to this implementation goes to production. +- [ ] FM1, FM2, FM3: Make sure `op-supervisor`, deposit simulations and other relevant off-chain components are properly put in place (TO BE DECIDED) to monitor and cover all the cases, being ready before to this implementation goes to production. ## Audit Requirements From dacf8c42cb85211d1dc8d9a60c2f9989fa4b8340 Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Fri, 24 Jan 2025 14:57:10 -0300 Subject: [PATCH 3/7] add preregistration context --- security/fma-ban-deposits.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md index 31f1d96e..f908524c 100644 --- a/security/fma-ban-deposits.md +++ b/security/fma-ban-deposits.md @@ -98,5 +98,4 @@ We suggest the modifications for banning deposit triggering `ExecutingMessage` e ## Additional Notes -Something worth noticing is that implementing the ban deposit closes the door for intended `ExecutingMessage` emissions from deposits. This feature could be desirable to force valid cross-chain messages from L1 and bypass sequencer censorship. -We could consider developing a new type of deposit that can prove the initiating message's validity and trigger the `ExecutingMessage` event at some point in the future. +Something worth noticing is that implementing the ban deposit closes the door for intended `ExecutingMessage` emissions from deposits. This feature could be desirable to force valid cross-chain messages from L1 and bypass sequencer censorship. There is an active exploration to ensure that the censorship-resistance property, with the introduction of [preregistrations](https://github.com/ethereum-optimism/specs/issues/520). From 2443451e78e8aca92513276e6391dbc5b84e51c4 Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Thu, 30 Jan 2025 14:22:27 -0300 Subject: [PATCH 4/7] correct intro --- security/fma-ban-deposits.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md index f908524c..dd206b65 100644 --- a/security/fma-ban-deposits.md +++ b/security/fma-ban-deposits.md @@ -23,7 +23,7 @@ This document is intended to be shared publicly for reviews and visibility purpo Below are references for this project: - Both Contract and Client updates are documented in the following specs: - [interop: Specify deposit handling #258](https://github.com/ethereum-optimism/specs/pull/258). -- Reference implementation and tests can be found in the following PR: +- Reference implementation can be found below: - `CrossL2Inbox`: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol - `L1BlockInterop`: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L2/L1BlockInterop.sol From 2b9a5ff42b477689ec0108835eb29f9f5b29e577 Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Thu, 30 Jan 2025 15:35:53 -0300 Subject: [PATCH 5/7] merge fm1 and fm2 and update action items --- security/fma-ban-deposits.md | 40 ++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md index dd206b65..6d83b32f 100644 --- a/security/fma-ban-deposits.md +++ b/security/fma-ban-deposits.md @@ -36,43 +36,38 @@ Note that this FMA doesn’t intend to cover the main features of core interop c ## Failure Modes and Recovery Paths -### FM1: An `ExecutingMessage` is emitted even if the `isDeposit` flag is correctly set +### FM1: An `ExecutingMessage` is emitted even if the `isDeposit` flag is correctly set, or `isDeposit` is not turned on before deposit transactions. - **Description:** If a deposit transaction calls `validateMessage` without reverting and emitting the event `ExecutingMessage`, the sequencer could be forced to include messages that do not correspond to an existing identifier. This could break multiple interop invariants and eventually lead to a bricked chain. -- **Risk Assessment:** Medium. - - Potential Impact: High. The impact of such an attack would vary depending on the message. Examples of economic attacks include: +- **Risk Assessment:** High. + - Potential Impact: Critical. The impact of such an attack would vary depending on the message. Examples of economic attacks include: - Releasing all ETH from the `ETHLiquidity` contract. - Relaying an ERC20 with a custom amount never burned on the source chain. In the worst-case scenario, the chain could produce unsafe blocks that are subsequently rejected, leading to a reorg, which would effectively brick the chain afterwards. This occurs because the chain cannot progress with a deposit transaction containing an invalid execution. Services such as relay-based bridges could also face significant consequences if they fail to detect the situation promptly. - - Likelihood: Low. This could only happen with a bugged implementation of the `CrossL2Inbox` check. Even if the current implementation is bug-free, future upgrades can introduce these bugs. - Something important to notice is that most chains within the same cluster will probably share implementations, so a bug might affect all chains. -- **Mitigations:** Our current codebase includes tests to check that the `CrossL2Inbox` contract validate messages only when `isDeposit` is `false` ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol#L139)) and revert when it is `true` ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol#L166)). The `L1BlockInterop` contract also includes test to check this property ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L205)). The security team should be aware of this issue and check for it in every protocol version upgrade. There should be a way to prevent to process such deposits (through `op-supervisor` or other), so the chain can continue, at the impact of halting deposits, until [sequencing windows](https://specs.optimism.io/glossary.html?highlight=sequencing%20window#sequencing-window) end. -- **Detection:** The `op-supervisor` could detect such a situation within the invariants checks. If `op-supervisor` simulations (or if there are tools for local sequencers to simulate deposits) are properly implemented before including new deposits, it would allow the sequencer to delay the deposit’s inclusion until the sequencing window ends. This delay would provide a time buffer to fix the issue. -- **Recovery Path(s):** Stop processing deposits and stay aware of the sequencing window. A chain halt followed by a fix and a reorg would be necessary. + - Likelihood: Low. This could only happen with a bugged implementation of the `CrossL2Inbox` check or a misconfiguration in `L1BlockInterop`. Even if the current implementation is bug-free, future upgrades can introduce these bugs. +- **Mitigations:** Our current codebase includes the following tests: + - Test to check that the `CrossL2Inbox` contract validates messages only when `isDeposit` is `false` ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol#L139)). + - Test to check that the `CrossL2Inbox` contract reverts messages only when `isDeposit` is `true` ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol#L166)). + - Test to check that the `L1BlockInterop` contract enforce the same `isDeposit` logic ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L205)). + - Test to check that the `L1BlockInterop` set `isDeposit` to `true` during the deposit context ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L239)). -### FM2: `isDeposit` is not turned on before deposit transactions + The security team should be aware of this issue and check for it in every protocol version upgrade. There should be a way to prevent to process such deposits. One solution could be update `op-geth` to revert automatically if it detects an execution of a deposit transactions that generates any logs in the `CrossL2Inbox`. +- **Detection:** It is possible to monitor new incoming deposits and simulate them before to include in L2. This also would allow the sequencer to delay the deposit’s inclusion until the sequencing window ends, in order to have a delay a a time buffer to fix the issue. Otherwise, chain will face a halt after deposit is tried to be included, which is easily detected. +- **Recovery Path(s):** A chain halt followed by a fix and a reorg would be necessary. -- **Description:** This would allow deposit transactions to emit `ExecutingMessage` events, even if the check in the `CrossL2Inbox` is working correctly. -- **Risk Assessment:** Medium. - - Potential Impact: High. All the consequences from FM1 would apply. - - Likelihood: Low. This would also correspond to a bugged implementation in the `L1Block` contract or client-triggered calls to it. -- **Mitigations:** Our current codebase include tests to check `L1BlockInterop` set `isDeposit` as `true` during the deposit context ([reference](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L239)). The security team should know this issue and check for it in every protocol version. -- **Detection:** Same as FM1. Off-chain services can detect flag misbehavior by checking every block's first transaction. -- **Recovery Path(s):** Stop processing deposits and stay aware of the sequencing window. A chain halt followed by a fix would be necessary. A reorg can also be considered if invariant breaking messages were created. +### FM2: `isDeposit` is not turned off after deposit transactions -### FM3: `isDeposit` is not turned off after deposit transactions - -- **Description:** If the `depositComplete()` call within `L1BlockInterop` fails or is never initiated, the `isDeposit` flag might remain on. This would imply that every call to `validateMessage()` will be seen as a deposit and therefore revert. +- **Description:** If the `depositComplete` call within `L1BlockInterop` fails or is never initiated, the `isDeposit` flag might remain on. This would imply that every call to `validateMessage` will be seen as a deposit and therefore revert. - **Risk Assessment:** Low. - Potential Impact: Medium. Genuine cross-chain messages will not be able to execute. This should not be a major issue, as users can re-execute after the fix. - - Likelihood: Low. This could happen if the `depositComplete()` implementation is bugged or the sequencer is not triggering the call to the function. The latter could occur due to a client bug or an out-of-gas error, which is unlikely. + - Likelihood: Low. This could happen if the `depositComplete` implementation is bugged or the sequencer is not triggering the call to the function. The latter could occur due to a client bug or an out-of-gas error, which is unlikely. - **Mitigations:** Our current codebase includes tests to check `L1BlockInterop` set `isDeposit` as `false` after the deposit context ends ([test](https://github.com/ethereum-optimism/optimism/blob/ef6ef6fd45fc2b7ccd4bc06dc7e24f75c0dda362/packages/contracts-bedrock/test/L2/L1BlockInterop.t.sol#L292)). The security team should know this issue and check for it in every protocol version. - **Detection:** Offchain services should be aware of this possibility for `validateMessage` reverts. - **Recovery Path(s):** Execute the proper fixes depending on whether it was a sequencer or contract error. Valid reverted messages can be re executed on destination, or resent if expired. -### FM4: `upgradeTxs` include an invalid `ExecutingMessage` +### FM3: `upgradeTxs` include an invalid `ExecutingMessage` - **Description:** The `isDeposit` bool is set off before the upgrade transactions, which are force-included. This implies that, if an upgrade transaction includes a call to `validateMessage`, the sequencer will be forced to include it, even if it doesn't point to an existing identifier. - **Risk Assessment:** Low to Medium. @@ -90,7 +85,8 @@ Note that this FMA doesn’t intend to cover the main features of core interop c ## Action Items - [ ] Resolve all the comments. -- [ ] FM1, FM2, FM3: Make sure `op-supervisor`, deposit simulations and other relevant off-chain components are properly put in place (TO BE DECIDED) to monitor and cover all the cases, being ready before to this implementation goes to production. +- [ ] FM1: Confirm whether the suggested changes to `op-geth` are considered (and implemented) or if other options are chosen. +- [ ] FM1: Confirm whether deposit simulations and procedures are feasible or if alternative monitoring methods are being considered (as action items). ## Audit Requirements From cbaf53fdce664a8df7667d09ebc362be21139bb1 Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Thu, 6 Feb 2025 00:03:57 -0300 Subject: [PATCH 6/7] improve upgradeTx fm --- security/fma-ban-deposits.md | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md index 6d83b32f..6dfd9168 100644 --- a/security/fma-ban-deposits.md +++ b/security/fma-ban-deposits.md @@ -57,6 +57,16 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - **Detection:** It is possible to monitor new incoming deposits and simulate them before to include in L2. This also would allow the sequencer to delay the deposit’s inclusion until the sequencing window ends, in order to have a delay a a time buffer to fix the issue. Otherwise, chain will face a halt after deposit is tried to be included, which is easily detected. - **Recovery Path(s):** A chain halt followed by a fix and a reorg would be necessary. +### FM2: `upgradeTxs` include an invalid `ExecutingMessage` + +- **Description:** The `isDeposit` bool is set off before the upgrade transactions, which are force-included. This implies that, if an upgrade transaction includes a call to `validateMessage`, the sequencer will be forced to include it, even if it doesn't point to an existing identifier. +- **Risk Assessment:** Medium. + - Potential Impact: High. It could impact the same way described in FM1. + - Likelihood: Low. An upgrade transaction should not call `validateMessage()` unless it is somehow intended to (and therefore not malicious). What's more, every upgrade transaction should bypass many security checks. +- **Mitigations:** Ideally, `upgradeTxs` should be enforced to be part of the deposit context, but since such upgrades have sudo privileges by nature—including the ability to turn off/remove the `isDeposit` flag—protocol changes would be required to actually enforce this rule. With the current implementation, during operations, every upgrade transaction should be simulated. An invalid Superchain state would be caught by the `op-supervisor` in simulations. +- **Detection:** Upgrades are heavily monitored transactions, making it very unlikely to go unnoticed. +- **Recovery Path(s):** Recovery would be similar to other bugged upgrades. See [Generic Hardfork FMA](https://github.com/ethereum-optimism/design-docs/blob/main/security/fma-generic-hardfork.md) for more details. + ### FM2: `isDeposit` is not turned off after deposit transactions - **Description:** If the `depositComplete` call within `L1BlockInterop` fails or is never initiated, the `isDeposit` flag might remain on. This would imply that every call to `validateMessage` will be seen as a deposit and therefore revert. @@ -67,16 +77,6 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - **Detection:** Offchain services should be aware of this possibility for `validateMessage` reverts. - **Recovery Path(s):** Execute the proper fixes depending on whether it was a sequencer or contract error. Valid reverted messages can be re executed on destination, or resent if expired. -### FM3: `upgradeTxs` include an invalid `ExecutingMessage` - -- **Description:** The `isDeposit` bool is set off before the upgrade transactions, which are force-included. This implies that, if an upgrade transaction includes a call to `validateMessage`, the sequencer will be forced to include it, even if it doesn't point to an existing identifier. -- **Risk Assessment:** Low to Medium. - - Potential Impact: High. It could impact the same way described in FM1. - - Likelihood: Low. An upgrade transaction should not call `validateMessage()` unless it is somehow intended to (and therefore not malicious). What's more, every upgrade transaction should bypass many security checks. -- **Mitigations:** Every upgrade transaction should be simulated. An invalid Superchain state would be caught by the `op-supervisor` in simulations. -- **Detection:** Upgrades are heavily monitored transactions, making it very unlikely to go unnoticed. -- **Recovery Path(s):** Recovery would be similar to other bugged upgrades. See [Generic Hardfork FMA](https://github.com/ethereum-optimism/design-docs/blob/main/security/fma-generic-hardfork.md) for more details. - ### Generic items we need to take into account: - Every consideration already covered by the [Generic Hardfork FMA](https://github.com/ethereum-optimism/design-docs/blob/main/security/fma-generic-hardfork.md) will also apply to these changes. @@ -87,6 +87,7 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - [ ] Resolve all the comments. - [ ] FM1: Confirm whether the suggested changes to `op-geth` are considered (and implemented) or if other options are chosen. - [ ] FM1: Confirm whether deposit simulations and procedures are feasible or if alternative monitoring methods are being considered (as action items). +- [ ] FM2: Confirm that the security team is aware of the lack of restrictions over upgradeTxs and closely monitor any future upgrade to ensure it does not call `validateMessage`. ## Audit Requirements From 14acd7c68eb9e84f9cdcaa8ce11815d2254edfe0 Mon Sep 17 00:00:00 2001 From: Joxes <91908708+Joxess@users.noreply.github.com> Date: Thu, 6 Feb 2025 00:08:05 -0300 Subject: [PATCH 7/7] improve fm2 action item --- security/fma-ban-deposits.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/fma-ban-deposits.md b/security/fma-ban-deposits.md index 6dfd9168..a8d6bd5a 100644 --- a/security/fma-ban-deposits.md +++ b/security/fma-ban-deposits.md @@ -87,7 +87,7 @@ Note that this FMA doesn’t intend to cover the main features of core interop c - [ ] Resolve all the comments. - [ ] FM1: Confirm whether the suggested changes to `op-geth` are considered (and implemented) or if other options are chosen. - [ ] FM1: Confirm whether deposit simulations and procedures are feasible or if alternative monitoring methods are being considered (as action items). -- [ ] FM2: Confirm that the security team is aware of the lack of restrictions over upgradeTxs and closely monitor any future upgrade to ensure it does not call `validateMessage`. +- [ ] FM2: Confirm that the security team is aware of the lack of restrictions over upgradeTxs and closely monitor any future upgrade to ensure it does not call `validateMessage`. Ideally, we would like to have strong restrictions for `upgradeTxs`. ## Audit Requirements