feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

agusduha · 2024-10-04T14:11:01Z

Description

The main idea is migrating the sendERC20 and relayERC20 functions from the SuperchainERC20 standard to a new SuperchainERC20Bridge. Also introducing ICrosschainERC20 interface for mint/burn functions to be cross-chain neutral and extensible to other L2s.

Specs:
ethereum-optimism/specs#384
ethereum-optimism/specs#413

Tests

Add SuperchainERC20Bridge tests
Refactor the old superchain standard tests

Additional context

The SuperchainERC20Bridge is a new predeploy.

Metadata

Closes #12322

* feat: add superchain erc20 bridge * fix: interfaces and versions

* refactor: use oz upgradeable erc20 as dependency * chore: update interfaces * fix: tests based on changes * refactor: remove op as dependency * feat: add check for supererc20 bridge on modifier * chore: update tests and interfaces * chore: update stack vars name on test * chore: remove empty gitmodules file * chore: update superchain weth errors

* test: add superchain erc20 bridge tests * test: add optimism superchain erc20 beacon tests * test: remove unnecessary test * test: tests fixes * test: tests fixes

* chore: update missing bridge on natspec * fix: natspecs --------- Co-authored-by: agusduha <[email protected]>

--------- Co-authored-by: agusduha <[email protected]>

* refactor: rename mint and burn functions on superchain erc20 * chore: rename optimism superchain erc20 to superchain erc20 * feat: create optimism superchain erc20 contract * chore: update natspec and errors * fix: superchain erc20 tests * refactor: make superchain erc20 abstract * refactor: move storage and erc20 metadata functions to implementation * chore: update interfaces * chore: update superchain erc20 events * fix: tests * fix: natspecs * fix: add semmver lock and snapshots * fix: remove unused imports * fix: natspecs --------- Co-authored-by: 0xDiscotech <[email protected]>

fix: update redesign with dev

* fix: semver natspec check failure * fix: ignore mock contracts in semver natspec script * fix: error message

* feat: add crosschain erc20 interface * fix: refactor interfaces

Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]>

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol

* fix: stop inheriting superchain interfaces * fix: move events and erros into the implementation * fix: make superchainERC20 inherits from crosschainERC20

tynes · 2024-10-04T21:17:30Z

What is the forge-std update from?

tynes · 2024-10-04T21:19:10Z

packages/contracts-bedrock/scripts/checks/semver-natspec/main.go

@@ -129,9 +129,14 @@ func run() error {
 return
 }

+ // Skip mock contracts
+ if strings.Contains(contractName, "_MockContract") {


Not a strong opinion but could just be open to making it mock, case insensitive. Tradeoff in explicit vs implicit

tynes · 2024-10-04T21:20:27Z

packages/contracts-bedrock/snapshots/abi/OptimismSuperchainERC20.json

 "type": "error"
 },
 {
 "inputs": [],
- "name": "OnlyBridge",
+ "name": "OnlySuperchainERC20Bridge",


We want to move away from the superchain specific naming so that this can be an ethereum wide standard in theory

This error is being used in the SuperchainERC20 that is the OP implementation of ICrosschainERC20, the neutral namings are set on that interface so that it can be used in other ethereum chains

tynes · 2024-10-04T21:22:30Z

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol

+/// @notice The SuperchainERC20Bridge allows for the bridging of ERC20 tokens to make them fungible across the
+/// Superchain. It builds on top of the L2ToL2CrossDomainMessenger for both replay protection and domain
+/// binding.
+contract SuperchainERC20Bridge {


Sorry for the naming nits, I know that I proposed this one, curious if you have any thoughts on the following issue: ethereum-optimism/specs#399

We agree on changing the name, we notice that asset is often used

so to be more consistent, do u prefer SuperchainTokenBridge or SuperchainAssetBridge?

I have a slight preference for SuperchainTokenBridge unless you think that there are non token assets?

tynes · 2024-10-04T21:24:15Z

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol

+ function relayERC20(address _token, address _from, address _to, uint256 _amount) external {
+ if (msg.sender != MESSENGER) revert CallerNotL2ToL2CrossDomainMessenger();
+
+ if (IL2ToL2CrossDomainMessenger(MESSENGER).crossDomainMessageSender() != address(this)) {


Would you find any value in adding a L2ToL2CrossDomainMessenger.context() function that returns both the source + sender + other contextual information? I notice we do this double call thing everywhere, it could be a simplification

It is a useful change, but we will be doing it in another PR to reduce the changes scope

tynes · 2024-10-04T21:25:34Z

packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20Factory.sol

+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import { IOptimismERC20Factory } from "./IOptimismERC20Factory.sol";


nit on import style, even tho eventually we should go back to relative paths so external projects can import this project without it breaking their remappings

…licts fix: crosschain merge conflicts

codecov · 2024-10-08T14:06:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.04%. Comparing base (18732db) to head (71389b6).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #12321      +/-   ##
===========================================
- Coverage    64.23%   64.04%   -0.19%     
===========================================
  Files           52       52              
  Lines         4353     4353              
===========================================
- Hits          2796     2788       -8     
- Misses        1381     1390       +9     
+ Partials       176      175       -1

Flag	Coverage Δ
cannon-go-tests	`64.04% <ø> (-0.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Dexaran · 2024-10-08T22:13:05Z

I would like to make a security disclosure.

ERC-20 standard contains a security flaw which resulted in $80,000,000 worth of tokens being lost due to lack of error handling implementation within the logic of transfer function.

Here is the full description: #12322 (comment)

As you are about to build your technology stack on top of the ERC-20 standard your users will be affected and are guaranteed to lose funds similar to how it happened on Ethereum due to the above described issue, unless you fix it.

skeletor-spaceman · 2024-10-09T01:52:18Z

Hi @Dexaran ! I'll echo what I replied on the other issue: #12322 (comment)
This is not a valid security disclosure, it does not affect the standard as is. The erc20 code will remain unchanged.

mds1

Just a skim of the diff with a few small comments

mds1 · 2024-10-09T01:11:53Z

packages/contracts-bedrock/scripts/checks/semver-natspec/main.go

+ // Skip mock contracts
+ if strings.Contains(contractName, "_mock") {
+ return
+ }


Looks like we already have MockL2ToL2CrossDomainMessenger and this PR adds SuperchainERC20Implementation_mock. Can we stick with the existing Mock* precedent?

mds1 · 2024-10-09T01:57:01Z

packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20.sol

- /// @notice Allows the L2StandardBridge to mint tokens.
- /// @param _to Address to mint tokens to.
- /// @param _amount Amount of tokens to mint.
+interface IOptimismSuperchainERC20 is ISuperchainERC20 {


What is the SuperchainERC20 interface compared to the CrosschainERC20 interface?

It has specific errors of the abstract implementation SuperchainERC20 that extends ICrosschainERC20. We extend it here so that the OptimismSuperchainERC20 has the correct ABI.

Hmm I thought I heard that SuperchainERC20 was being renamed to CrosschainERC20, and therefore I'd only expect to see CrosschainERC20 throughout this PR. I may be out of the loop there though, so just want to better understand the relationship between the two

mds1 · 2024-10-09T01:59:03Z

packages/contracts-bedrock/src/L2/interfaces/ICrosschainERC20.sol

+ /// @notice Mint tokens through a crosschain transfer.
+ /// @param _to Address to mint tokens to.
+ /// @param _amount Amount of tokens to mint.
+ function __crosschainMint(address _to, uint256 _amount) external;


What does the double underscore convention mean? The only other place I see this in the codebase (aside from the __constructor__ dummy sigs) is internal initializer methods such as __ERC721Bridge_init, but these are not initializers

It is to make it clear that are not supposed to be called by anyone else

It was proposed in this thread https://discord.com/channels/1244729134312198194/1285980446857101423/1289362724135899209

Asked a follow up question to learn more in discord here

mds1 · 2024-10-09T02:01:55Z

packages/contracts-bedrock/src/L2/SuperchainTokenBridge.sol

+/// binding.
+contract SuperchainTokenBridge {
+ /// @notice Thrown when attempting to perform an operation and the account is the zero address.
+ error ZeroAddress();


Consider an Errors.sol to avoid redeclaring errors like this in each file

mds1 · 2024-10-09T02:04:36Z

packages/contracts-bedrock/src/L2/SuperchainTokenBridge.sol

+ /// @notice Thrown when attempting to relay a message and the function caller (msg.sender) is not
+ /// L2ToL2CrossDomainMessenger.
+ error CallerNotL2ToL2CrossDomainMessenger();


This looks analagous to the Unauthorized eror we use elsewhere in the codebase. Consider using that to minimize the amount of new errors for a simpler ABI. Either the Unauthorized() or the Unauthorized(expected, actual) signatures that we use are ok with me

We discussed this with @skeletor-spaceman, @0xng, and @gotzenx, and we believe that having explicit error messages offers more advantages than disadvantages. However, we're open to changing them if it's absolutely necessary for the PR.

I don't feel strongly, I just want to be careful about exploding the amount of custom errors vs. grouping related ones and using params to get specific. For example you could even do Unauthorized(string) to revert with Unauthorized("Caller is not L2ToL2CrossDomainMessenger")

Returning a string defeats the purpose of using a custom error no? At least with regards to the bytecode size

I definitely am a minimalist and prefer a minimal API but if you are very opinionated about very specific custom errors, its ok to ship

Returning a string defeats the purpose of using a custom error no? At least with regards to the bytecode size

Ultimately depends on what we want to optimize for. The tradeoff is roughly:

Bespoke errors for each case: Biggest ABI, but full rationale is part of the ABI for consumers

Unauthorized(address actual, address expected) types of errors: Small ABI, but ABI alone doesn't tell consumer what each address semantically represents from the ABI (i.e. CallerNotL2ToL2CrossDomainMessenger tells me I expected the L2ToL2CrossDomainMessenger, but I have to know that the expected: 0x... address is the L2ToL2CrossDomainMessenger address

Unauthorized(string): Same ABI size, more flexibility in communicating to users what went wrong, larger bytecode and gas due to string usage

Unauthorized(bytes32): Like the prior bullet, but limited string size, so smaller/cheaper

Personally I also like smaller interfaces (for functions, errors, and events) as it means smaller surface areas (less attack surface when talking about functions), and for me that makes it easier to fit full context in my head when reasoning + less room for mistakes when choosing which to use.

Ultimately whatever we decide should be upstreamed to the Style Guide to avoid this bikeshed in the future 😅

mds1 · 2024-10-09T02:05:02Z

packages/contracts-bedrock/src/L2/SuperchainWETH.sol

+ /// @custom:semver 1.0.0-beta.7
+ string public constant version = "1.0.0-beta.7";


Looks like we skipped beta.6 and went straight to 7

Suggested change

/// @custom:semver 1.0.0-beta.7

string public constant version = "1.0.0-beta.7";

/// @custom:semver 1.0.0-beta.6

string public constant version = "1.0.0-beta.6";

* fix: refactor common errors * fix: remove unused version

…ory-merge-conflicts fix: superchain erc20 factory merge conflicts

agusduha and others added 15 commits September 25, 2024 15:20

feat: add superchain erc20 bridge (#61)

ae024a5

* feat: add superchain erc20 bridge * fix: interfaces and versions

test: add superchain erc20 bridge tests (#65)

6aaf905

* test: add superchain erc20 bridge tests * test: add optimism superchain erc20 beacon tests * test: remove unnecessary test * test: tests fixes * test: tests fixes

chore: update missing bridge on natspec (#69)

11fda67

* chore: update missing bridge on natspec * fix: natspecs --------- Co-authored-by: agusduha <[email protected]>

fix: remove superchain erc20 base (#70)

7a69a7e

refactor: update isuperchainweth (#71)

07c9c77

--------- Co-authored-by: agusduha <[email protected]>

fix: refactor zero check (#76)

2e507b2

Merge branch 'develop' into fix/redesign-merge-conflicts

7fcaf6c

fix: pre pr

619f23c

Merge pull request #77 from defi-wonderland/fix/redesign-merge-conflicts

1981789

fix: update redesign with dev

fix: semver natspec check failure (#79)

ae019b8

* fix: semver natspec check failure * fix: ignore mock contracts in semver natspec script * fix: error message

feat: add crosschain erc20 interface (#80)

a5aca61

* feat: add crosschain erc20 interface * fix: refactor interfaces

fix: superchain bridge natspec (#83)

83757ea

fix: superchain weth natspec (#84)

904a0ce

Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]>

agusduha requested a review from a team as a code owner October 4, 2024 14:11

agusduha requested a review from maurelian October 4, 2024 14:11

github-advanced-security bot found potential problems Oct 4, 2024

View reviewed changes

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol Fixed Show fixed Hide fixed

agusduha mentioned this pull request Oct 4, 2024

feat: Add ICrosschainERC20 standard defi-wonderland/optimism#82

Closed

fix: stop inheriting superchain interfaces (#85)

3ebb1d1

* fix: stop inheriting superchain interfaces * fix: move events and erros into the implementation * fix: make superchainERC20 inherits from crosschainERC20

tynes reviewed Oct 4, 2024

View reviewed changes

agusduha and others added 4 commits October 7, 2024 15:31

fix: superchain bridge rename (#86)

1b4317e

fix: fee vault compiler error (#87)

10038c3

Merge branch 'develop' into fix/crosschain-merge-conflicts

9f49b43

fix: remove unused imports

ab1e074

Merge pull request #88 from defi-wonderland/fix/crosschain-merge-conf…

a79bf4e

…licts fix: crosschain merge conflicts

mds1 reviewed Oct 9, 2024

View reviewed changes

agusduha added 2 commits October 9, 2024 16:16

fix: refactor common errors (#90)

af9694e

* fix: refactor common errors * fix: remove unused version

fix: reuse unauthorized error (#92)

6c46f9b

tremarkley mentioned this pull request Oct 10, 2024

Add SuperchainTokenBridge contract to forked supersim ethereum-optimism/supersim#198

Open

agusduha and others added 4 commits October 11, 2024 10:27

Merge branch 'develop' into fix/superchain-erc20-factory-merge-conflicts

736adf3

fix: superchain erc20 factory conflicts

061b1ff

Merge pull request #93 from defi-wonderland/fix/superchain-erc20-fact…

97fb300

…ory-merge-conflicts fix: superchain erc20 factory merge conflicts

fix: rename crosschain functions (#94)

71389b6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

agusduha commented Oct 4, 2024 •

edited

Loading

tynes commented Oct 4, 2024

tynes Oct 4, 2024

tynes Oct 4, 2024

agusduha Oct 7, 2024 •

edited

Loading

tynes Oct 4, 2024

agusduha Oct 7, 2024

tynes Oct 7, 2024

tynes Oct 4, 2024 •

edited

Loading

agusduha Oct 7, 2024

tynes Oct 4, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

Dexaran commented Oct 8, 2024

skeletor-spaceman commented Oct 9, 2024

mds1 left a comment

mds1 Oct 9, 2024

mds1 Oct 9, 2024

agusduha Oct 9, 2024

mds1 Oct 9, 2024

mds1 Oct 9, 2024

agusduha Oct 9, 2024 •

edited

Loading

mds1 Oct 9, 2024

mds1 Oct 9, 2024

mds1 Oct 9, 2024

agusduha Oct 9, 2024

mds1 Oct 9, 2024

tynes Oct 9, 2024 •

edited

Loading

tynes Oct 9, 2024

mds1 Oct 10, 2024

mds1 Oct 9, 2024

		/// @custom:semver 1.0.0-beta.7
		string public constant version = "1.0.0-beta.7";

feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

Are you sure you want to change the base?

feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

Conversation

agusduha commented Oct 4, 2024 • edited Loading

tynes commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agusduha Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

Dexaran commented Oct 8, 2024

skeletor-spaceman commented Oct 9, 2024

mds1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agusduha Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agusduha commented Oct 4, 2024 •

edited

Loading

agusduha Oct 7, 2024 •

edited

Loading

tynes Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

agusduha Oct 9, 2024 •

edited

Loading

tynes Oct 9, 2024 •

edited

Loading