-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Re-add superchainERC20 FMA in a single PR for FMA review #188
base: main
Are you sure you want to change the base?
Conversation
- **Risk Assessment**: Medium. | ||
- Potential impact: High. All tokens based on this implementation could be potentially at risk. | ||
- Likelihood: Very Low. `Predeploys.SUPERCHAIN_TOKEN_BRIDGE` is defined via protocol upgrades. The conditional is sufficiently simple and battle-tested to give confidence in the implementation. | ||
- **Mitigation**: There are tests for this authorization check, and tests that the `SuperchainTokenBridge` predeploy is correctly set during the interop fork upgrade, and isn’t subject to unexpected changes. Additionally, users should check that the `SuperchainTokenBridge` predeploy is correctly deployed and configured before trusting it to mint and burn their token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Joxess to add links to the tests mentioned here
- **Detection**: Verify contract addresses upon deployment. | ||
- **Recovery Path(s)**: Redeploy token contracts correctly. | ||
|
||
### FM3: Same Token Address but Different (or Malicious) Implementations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For FM2 and FM3, there are some open questions:
- Do we want to merge FM2 and FM3 into a single invariant of "SuperchainERC20 Tokens MUST have the same code at the same address across all chains in the interoperable set".
- Or, do we want to support having the same token address but with different implementations, for use cases such as a gov token has a primary chain and a simpler implementaiton when bridged to other chains. If that use case is something we need to support, FM3 isn't actually a failure mode, and becomes a user-level concern (i.e. user has to verify if implementation on destination chain is safe)
This is similar to the above paragraph:
Similar to ERC20, implementations SuperchainERC20 should be considered untrusted by default, as the implementations of
crosschainMint
andcrosschainBurn
methods are not constrained by IERC7802 or the SuperchainERC20 implementation. As a result, failure modes resulting from malicious implementations of SuperchainERC20 are not considered here.
In other words, we want to decide whether this is a protocol-level failure mode that should have detection and mitigation (i.e. we are responsible for ensuring same code at same address is always attainable) or a user-level failure mode where users must verify all implementations for a token are not malicious
|
||
## Audit Requirements | ||
|
||
No audit should be required, as it is simple and isn’t expected to have dependencies or impact on other core OP contracts. In any case, an audit over the whole system is expected, including the `SuperchainERC20` contract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the upcoming audit so let's update this text
## Additional Notes | ||
|
||
The proposed implementation of the standard doesn’t prevent a token issuer from using other token standards, such as xERC20, with the `SuperchainTokenBridge` to mint and burn tokens across an [interoperable set of chains](https://specs.optimism.io/interop/overview.html). More about this is explained in [xERC20-ERC7802 compatilibility](https://defi-wonderland.notion.site/xERC20-ERC7802-compatibility-14c9a4c092c780ca94a8cb81e980d813). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this into the intro section for visibility
Replaces #187 so the full content of the FMA is present in a single PR, to facilitate FMA review