FMA for contract version upgrade #69
Conversation
| - 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 |
There was a problem hiding this comment.
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,
|
|
||
| ### Change in Default EVM version between different Solidity versions | ||
|
|
||
| - **Description:** Version 0.8.25 sets the default evm version to cancun |
| - 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. | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Got it, so we probably should add an action to track verifying this
| | Author | Michael Amadi | | ||
| | Created at | 2024-08-20 | | ||
| | Needs Approval From | _Security Reviewer Name_ | | ||
| | Other Reviewers | _Reviewer Name 1, Reviewer Name 2_ | |
There was a problem hiding this comment.
You can put needs approval from as Matt Solomon and put me (Mark Tyneway) as other reviewer
|
This is important to prevent tech debt accumulation. We depend on modern solidity in the interop contracts and using contracts at different versions was starting to break tests and make them difficult to write |
…ns" to "Other Failure Modes" subsection
| - [ ] 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@smartcontracts can help here
There was a problem hiding this comment.
Bump on this, once:
- the suggested action from https://github.com/ethereum-optimism/design-docs/pull/69/files#r1773672065 is added, and
- all actions have an assignee
I will approve this and we can transition this to implementing actions
Co-authored-by: Matt Solomon <[email protected]>
Co-authored-by: Matt Solomon <[email protected]>
Co-authored-by: Matt Solomon <[email protected]>
|
Closing since this has been merged #106 |
Description
This covers solidity version upgrade of all OP Stack smart contracts from 0.8.15 code to 0.8.25.
Metadata