Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FMA for contract version upgrade #69

Closed
wants to merge 12 commits into from
Closed

FMA for contract version upgrade #69

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
208371b
fma-contract-version-upgrade first instance
AmadiMichael Aug 23, 2024
20aecb3
edit and remove 2 fmas
AmadiMichael Aug 23, 2024
e5b7651
add table of contents and add block.difficulty removal
AmadiMichael Aug 24, 2024
06a4916
modify
AmadiMichael Aug 28, 2024
978b3c9
modify...
AmadiMichael Aug 29, 2024
0ecf6c5
add reviewers and approvers
AmadiMichael Aug 30, 2024
27ecd01
add more failure modes, add recommended changes
AmadiMichael Sep 7, 2024
5a27688
move "Change in Default EVM version between different Solidity versio…
AmadiMichael Sep 9, 2024
ef095c8
add audit requirement reason, add action item
AmadiMichael Sep 12, 2024
ce8caed
Update audit requirements
AmadiMichael Sep 14, 2024
0d47830
Update FMA status
AmadiMichael Sep 14, 2024
2594711
Update security/fma-contract-version-upgrade.md
AmadiMichael Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 172 additions & 0 deletions security/fma-contract-version-upgrade.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
# Solidity Version Upgrade: Failure Modes and Recovery Path Analysis

<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->

**Table of Contents**

- [Introduction](#introduction)
- [Failure Modes and Recovery Paths](#failure-modes-and-recovery-paths)
- [Solidity Bugs Fixed](#solidity-bugs-fixed)
- [Missing side effect on `.selector` access bug (Bug fixed after 0.8.15)](#missing-side-effect-on-selector-access-bug-bug-fixed-after-0815)
- [Storage write removal before conditional termination (Bug fixed after 0.8.15)](#storage-write-removal-before-conditional-termination-bug-fixed-after-0815)
- [Other Failure Modes](#other-failure-modes)
- [Change in Default EVM version between different Solidity versions](#change-in-default-evm-version-between-different-solidity-versions)
- [Chosen solidity version is too recent and might contain unknown bugs](#chosen-solidity-version-is-too-recent-and-might-contain-unknown-bugs)
- [Stack too deep errors during compilation](#stack-too-deep-errors-during-compilation)
- [Contracts exceeding code size when compiled](#contracts-exceeding-code-size-when-compiled)
- [Audit Requirements](#audit-requirements)
- [Action Items](#action-items)
- [Appendix](#appendix)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

| | |
| ------------------- | ------------- |
| Author | Michael Amadi |
| Created at | 2024-08-20 |
| Needs Approval From | Matt Solomon |
| Other Reviewers | Mark Tyneway |
| Status | In Review |

> [!NOTE]
> 📢 Remember:
>
> - The single approver in the “Need Approval From” must be from the Security team.
> - Maintain the “Status” property accordingly.

> [!TIP]
> Guidelines for writing a good analysis, and what the reviewer will look for:
>
> - Show your work: Include steps and tools for each conclusion.
> - Completeness of risks considered.
> - Include both implementation and operational failure modes
> - Provide references to support the reviewer.
> - The size of the document will likely be proportional to the project's complexity.
> - The ultimate goal of this document is to identify action items to improve the security of the project. The FMA review process can be accelerated by proactively identifying action items during the writing process.

## Introduction

This document covers solidity version upgrade of all OP Stack smart contracts from 0.8.15 code to a more recent version with support for transient storage and mcopy opcodes (i.e versions >=0.8.24).

Below are references for this project:

- [Issue 11527: Smart Contracts: Update all 0.8.15 code to a more recent version](https://github.com/ethereum-optimism/optimism/issues/11527).

## Failure Modes and Recovery Paths
mds1 marked this conversation as resolved.
Show resolved Hide resolved

### Solidity Bugs Fixed

This section lists solidity bugs that were fixed as failure modes and important changes to default values used for compilation and explains why these might be a risk for us. This set of bug fixes and changes was chosen because:

- Bug fixes: These are bugs that are only introduced when code is written a specific way, though meant to be correct but the compiler generates incorrect bytecode that can include a vulnerability.
- Changes to default compilation settings: These are changes that if not put into consideration when writing and compiling code can lead to generating non-optimized, non-compiling and/or non-executable code.

#### Missing side effect on `.selector` access bug (Bug fixed after 0.8.15)

- **Description:**
- Introduced: v0.6.2
- Fixed: v0.8.21
- Explanation: The legacy compiler assumes that conditionals used to determine which selector is used/removed have no side effects. So if the result of the conditional is known at compile time the legacy compiler will hardcode the corresponding selector (since selectors are known at compile time). This bug does no exist when the conditional is used with other types of constants.
- **Risk Assessment:**
- Severity: High (if present)
- Likelihood: Low (and mostly present in anti-patterns like using conditionals that have side effects)
- **Mitigations:** Using a solidity version above 0.8.20, if lower versions must be used, conditionals should have no side effects.
- **Detection:** Confidence on the absence of this bug can be stronger with robust unit tests that check that all side effects of a call happens and correctly.
- **Recovery Path(s)**: Redeployment and upgrade of the affected contracts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe we have this issue in our codebase

$ git grep -rin '.selector' src 

src/L1/DelayedVetoable.sol:10:///         Additionally, this contract cannot be used to forward calls with data beginning with the function selector
src/L1/L1ERC721Bridge.sol:106:        bytes memory message = abi.encodeWithSelector(
src/L1/L1ERC721Bridge.sol:107:            L2ERC721Bridge.finalizeBridgeERC721.selector, _remoteToken, _localToken, _from, _to, _tokenId, _extraData
src/L2/L1Block.sol:141:                mstore(0x00, 0x3cc50b45) // 0x3cc50b45 is the 4-byte selector of "NotDepositor()"
src/L2/L1Block.sol:142:                revert(0x1C, 0x04) // returns the stored 4-byte selector from above
src/L2/L2ERC721Bridge.sol:114:        bytes memory message = abi.encodeWithSelector(
src/L2/L2ERC721Bridge.sol:115:            L1ERC721Bridge.finalizeBridgeERC721.selector, remoteToken, _localToken, _from, _to, _tokenId, _extraData
src/cannon/PreimageOracle.sol:272:                // Store the "ShaFailed()" error selector.
src/cannon/PreimageOracle.sol:303:                // Store the "InvalidProof()" error selector.
src/cannon/PreimageOracle.sol:518:                // Store "InvalidInputSize()" error selector
src/dispute/FaultDisputeGame.sol:213:        // - 0x04 selector
src/dispute/FaultDisputeGame.sol:221:                // Store the selector for `BadExtraData()` & revert
src/legacy/L1ChugSplashProxy.sol:35:            owner.staticcall(abi.encodeWithSelector(IL1ChugSplashDeployer.isUpgrading.selector));
src/legacy/LegacyMintableERC20.sol:48:        bytes4 secondSupportedInterface = ILegacyMintableERC20.l1Token.selector ^ ILegacyMintableERC20.mint.selector
src/legacy/LegacyMintableERC20.sol:49:            ^ ILegacyMintableERC20.burn.selector;
src/libraries/SafeCall.sol:123:                // Store the "Error(string)" selector in scratch space.
src/universal/CrossDomainMessenger.sol:189:            _data: abi.encodeWithSelector(
src/universal/CrossDomainMessenger.sol:190:                this.relayMessage.selector, messageNonce(), msg.sender, _target, msg.value, _minGasLimit, _message
src/universal/StandardBridge.sol:333:            _message: abi.encodeWithSelector(this.finalizeBridgeETH.selector, _from, _to, _amount, _extraData),
src/universal/StandardBridge.sol:378:            _message: abi.encodeWithSelector(
src/universal/StandardBridge.sol:379:                this.finalizeBridgeERC20.selector,


#### Storage write removal before conditional termination (Bug fixed after 0.8.15)

- **Description:**

- Introduced: v0.8.13
- Fixed: v0.8.17
- Explanation: Incorrect assumption that a storage write is redundant so the optimizer removes it. But this assumption can be wrong when the developer intended otherwise. A storage write is considered redundant when the storage slot is written to twice within an execution context without that slot being read before the second write or execution unconditionally terminates before the slot is read again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug fix. We may have been exposed to the bug but unlikely. The way that this could hurt us is if we depended on the buggy behavior

Copy link
Contributor

@mds1 mds1 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely recall talking with @maurelian about this a while ago, and think he said we checked our codebase when this was announced? However it's been 2 years since this bug was fixed so we may have introduced this bug at some point after.

Regardless, agree with @tynes that depending on the buggy behavior is the primary risk, but that is unlikely

Blog post for reference: https://soliditylang.org/blog/2022/09/08/storage-write-removal-before-conditional-termination/

Copy link
Contributor

@maurelian maurelian Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't recall doing that check.

The one that I recall we did a review for was the Size Check Bug in Nested Calldata Array ABI-Reencoding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, so we probably should add an action to track verifying this

According to the solidity blog, this bug can only occur if a contract contains pattern:

1. A storage write. Note that the write may still be removed even if it happens only conditionally or within a call to a function that ends up being inlined.
2. A call to a function that conditionally terminates using inline assembly as described above, but also has a different code path that returns to the caller.
3. Any continuing control flow path does one of the following:
- It overwrites the storage write in (1).
- It reverts.

- **Risk Assessment:**
- Severity: High (if present)
- Likelihood: Low
- **Mitigations:** Use a solidity version above 0.8.17.
- **Detection:** If a solidity contract (using solidity version earlier than 0.8.17) follows the pattern described above, its potentially vulnerable to this.
- **Recovery Path(s)**: Redeployment and upgrade of the affected contracts

### Other Failure Modes

#### Change in Default EVM version between different Solidity versions

- **Description:** Version 0.8.25 sets the default evm version to cancun
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no problem

- **Risk Assessment:**
- Severity: Low/Medium/High
- Likelihood: Medium
- **Mitigations:** As at 0.8.15, the default EVM version is 'London', if no explicit version override is declared, 'Cancun' will be used rather which if not intended can lead to code that either does not deploy or reverts un-ideally at runtime even though non-fork tests might have passed. A mitigation is explicitly declaring the intended EVM version to use for compilation regardless of if its the same as the default the compiler uses. That way, a new compiler version that changes the default EVM version does not change the one that is intended to be used. In this scenario however, this is not possible because the intended EVM version is the same as the compiler's EVM version default.
- **Detection:** Running unit tests as fork test.
- **Recovery Path(s)**: Redeployment and upgrade of the affected contracts

#### Chosen solidity version is too recent and might contain unknown bugs

- **Description:** Choosing a very recent solidity version which hasn't stood a relatively long enough test of time can pose the risk of having an unknown bug introduced into the OP Stack smart contracts.
- **Risk Assessment:**
- Severity: High
- Likelihood: Low/Medium
- **Mitigations:** Might be safer to use a solidity version 1 or 2 releases earlier than the latest.
- **Detection:** -
- **Recovery Path(s)**: Redeployment and upgrade of the affected contracts

#### Stack too deep errors during compilation

- **Description:** Current OP Stack contracts only compile with the legacy pipeline with the optimizer on. Compiling with the optimizer off or using the via-ir pipeline result in stack too deep errors. When upgrading to a new solidity version, we have to make sure that successful compilation using the current build settings is possible.
- **Risk Assessment:**
- Severity: -
- Likelihood: -
- **Mitigations:** -
- **Detection:** During compilation
- **Recovery Path(s)**: Code modification (local variable scoping, local variables in memory etc), build settings modification or/and usage of a different compiler version.

#### Contracts exceeding code size when compiled

- **Description:** There are a few OP Stack contracts that are close to the contract code size limit. We have to ensure that the chosen compiler version to upgrade to compiles all OP Stack contracts successfully and without any exceeding the contract code size limit.
- **Risk Assessment:**
- Severity: -
- Likelihood: -
- **Mitigations:** -
- **Detection:** During compilation
- **Recovery Path(s)**: Code modification (runtime code size optimization), build settings modification or/and usage of a different compiler version.

## Audit Requirements

_Will this project require an audit according to the guidance in [OP Labs Audit Framework: When to get external security review and how to prepare for it](https://gov.optimism.io/t/op-labs-audit-framework-when-to-get-external-security-review-and-how-to-prepare-for-it/6864)? Please explain your reasoning._
mds1 marked this conversation as resolved.
Show resolved Hide resolved

This project would not require an audit. Because we don't expect to change Solidity code aside from the pragmas, an audit would have to review the bytecode diff, which is not practical. Also, historically, we trust compiler generated code and don't audit bytecode so we are continuing to do the same here.

## 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:

- [ ] Decide on the solidity version to upgrade to
- [ ] Resolve all comments on this document and incorporate them into the document itself (Assignee: document author)
- [ ] Contracts compile successfully with upgraded solidity version and without stack too deep error(s)
- [ ] Contracts compile successfully with upgraded solidity version and without any contract intended to be deployed exceeding the contract code size limit (24576 bytes)
Comment on lines +153 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure each action is assigned to someone so we know who owns which action, I'll defer to @AmadiMichael @tynes for this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smartcontracts can help here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump on this, once:

  1. the suggested action from https://github.com/ethereum-optimism/design-docs/pull/69/files#r1773672065 is added, and
  2. all actions have an assignee

I will approve this and we can transition this to implementing actions


## Appendix

- [Solidity 0.8.25 Release Announcement](https://soliditylang.org/blog/2024/03/14/solidity-0.8.25-release-announcement/)
- [Solidity 0.8.24 Release Announcement](https://soliditylang.org/blog/2024/01/26/solidity-0.8.24-release-announcement/)
- [Solidity 0.8.23 Release Announcement](https://soliditylang.org/blog/2023/11/08/solidity-0.8.23-release-announcement/)
- [Solidity 0.8.22 Release Announcement](https://soliditylang.org/blog/2023/10/25/solidity-0.8.22-release-announcement/)
- [Solidity 0.8.21 Release Announcement](https://soliditylang.org/blog/2023/07/19/solidity-0.8.21-release-announcement/)
- [Solidity 0.8.20 Release Announcement](https://soliditylang.org/blog/2023/05/10/solidity-0.8.20-release-announcement/)
- [Solidity 0.8.19 Release Announcement](https://soliditylang.org/blog/2023/02/22/solidity-0.8.19-release-announcement/)
- [Solidity 0.8.18 Release Announcement](https://soliditylang.org/blog/2023/02/01/solidity-0.8.18-release-announcement/)
- [Solidity 0.8.17 Release Announcement](https://soliditylang.org/blog/2022/09/08/solidity-0.8.17-release-announcement/)
- [Solidity 0.8.16 Release Announcement](https://soliditylang.org/blog/2022/08/08/solidity-0.8.16-release-announcement/)
- [Head Overflow Bug in Calldata Tuple ABI-Reencoding](https://soliditylang.org/blog/2022/08/08/calldata-tuple-reencoding-head-overflow-bug/)
- [Storage Write Removal Bug On Conditional Early Termination](https://soliditylang.org/blog/2022/09/08/storage-write-removal-before-conditional-termination/)
- [Bug in Legacy Code Generation When Accessing the .selector Member on Expressions with Side Effects](https://soliditylang.org/blog/2023/07/19/missing-side-effects-on-selector-access-bug/)