Merge pull request #4932 from ethereum-optimism/fix/is-correct-token-pair

contracts-bedrock: fix standard bridge token pair checking

packages/contracts-bedrock/.gas-snapshot

@@ -174,16 +174,18 @@ L2OutputOracleUpgradeable_Test:test_initValuesOnProxy_succeeds() (gas: 26208)
 L2OutputOracleUpgradeable_Test:test_initializeImpl_alreadyInitialized_reverts() (gas: 15149)
 L2OutputOracleUpgradeable_Test:test_initializeProxy_alreadyInitialized_reverts() (gas: 20175)
 L2OutputOracleUpgradeable_Test:test_upgrading_succeeds() (gas: 180481)
-L2StandardBridge_BridgeERC20To_Test:test_bridgeERC20To_succeeds() (gas: 387833)
-L2StandardBridge_BridgeERC20To_Test:test_withdrawTo_withdrawingERC20_succeeds() (gas: 388064)
-L2StandardBridge_BridgeERC20_Test:test_bridgeERC20_succeeds() (gas: 383536)
+L2StandardBridge_BridgeERC20To_Test:test_bridgeERC20To_succeeds() (gas: 389773)
+L2StandardBridge_BridgeERC20To_Test:test_withdrawTo_withdrawingERC20_succeeds() (gas: 390006)
+L2StandardBridge_BridgeERC20_Test:test_bridgeERC20_succeeds() (gas: 385280)
+L2StandardBridge_BridgeERC20_Test:test_bridgeLegacyERC20_succeeds() (gas: 393552)
+L2StandardBridge_BridgeERC20_Test:test_withdrawLegacyERC20_succeeds() (gas: 393878)
 L2StandardBridge_BridgeERC20_Test:test_withdraw_notEOA_reverts() (gas: 251758)
-L2StandardBridge_BridgeERC20_Test:test_withdraw_withdrawingERC20_succeeds() (gas: 383722)
+L2StandardBridge_BridgeERC20_Test:test_withdraw_withdrawingERC20_succeeds() (gas: 385508)
 L2StandardBridge_Bridge_Test:test_finalizeBridgeETH_incorrectValue_reverts() (gas: 23843)
 L2StandardBridge_Bridge_Test:test_finalizeBridgeETH_sendToMessenger_reverts() (gas: 23982)
 L2StandardBridge_Bridge_Test:test_finalizeBridgeETH_sendToSelf_reverts() (gas: 23870)
-L2StandardBridge_Bridge_Test:test_finalizeDeposit_depositingERC20_succeeds() (gas: 91013)
-L2StandardBridge_Bridge_Test:test_finalizeDeposit_depositingETH_succeeds() (gas: 89889)
+L2StandardBridge_Bridge_Test:test_finalizeDeposit_depositingERC20_succeeds() (gas: 93824)
+L2StandardBridge_Bridge_Test:test_finalizeDeposit_depositingETH_succeeds() (gas: 92700)
 L2StandardBridge_FinalizeBridgeETH_Test:test_finalizeBridgeETH_succeeds() (gas: 43155)
 L2StandardBridge_Test:test_initialize_succeeds() (gas: 24292)
 L2StandardBridge_Test:test_receive_succeeds() (gas: 174011)
@@ -412,6 +414,8 @@ SequencerFeeVault_Test:test_minWithdrawalAmount_succeeds() (gas: 5442)
 SequencerFeeVault_Test:test_receive_succeeds() (gas: 17373)
 SequencerFeeVault_Test:test_withdraw_notEnough_reverts() (gas: 9331)
 SequencerFeeVault_Test:test_withdraw_succeeds() (gas: 163228)
+StandardBridge_Stateless_Test:test_isCorrectTokenPair_succeeds() (gas: 49936)
+StandardBridge_Stateless_Test:test_isOptimismMintableERC20_succeeds() (gas: 33072)
 SystemConfig_Initialize_TestFail:test_initialize_lowGasLimit_reverts() (gas: 62012)
 SystemConfig_Setters_TestFail:test_setBatcherHash_notOwner_reverts() (gas: 10612)
 SystemConfig_Setters_TestFail:test_setGasConfig_notOwner_reverts() (gas: 10555)

packages/contracts-bedrock/contracts/legacy/LegacyMintableERC20.sol

+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.15;
+
+import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+import { ILegacyMintableERC20 } from "../universal/OptimismMintableERC20.sol";
+
+/**
+ * @title LegacyMintableERC20
+ * @notice The legacy implementation of the OptimismMintableERC20. This
+ *         contract is deprecated and should no longer be used.
+ */
+contract LegacyMintableERC20 is ILegacyMintableERC20, ERC20 {
+    /**
+     * @notice Emitted when the token is minted by the bridge.
+     */
+    event Mint(address indexed _account, uint256 _amount);
+
+    /**
+     * @notice Emitted when a token is burned by the bridge.
+     */
+    event Burn(address indexed _account, uint256 _amount);
+
+    /**
+     * @notice The token on the remote domain.
+     */
+    address public l1Token;
+
+    /**
+     * @notice The local bridge.
+     */
+    address public l2Bridge;
+
+    /**
+     * @param _l2Bridge Address of the L2 standard bridge.
+     * @param _l1Token Address of the corresponding L1 token.
+     * @param _name ERC20 name.
+     * @param _symbol ERC20 symbol.
+     */
+    constructor(
+        address _l2Bridge,
+        address _l1Token,
+        string memory _name,
+        string memory _symbol
+    ) ERC20(_name, _symbol) {
+        l1Token = _l1Token;
+        l2Bridge = _l2Bridge;
+    }
+
+    /**
+     * @notice Modifier that requires the contract was called by the bridge.
+     */
+    modifier onlyL2Bridge() {
+        require(msg.sender == l2Bridge, "Only L2 Bridge can mint and burn");
+        _;
+    }
+
+    /**
+     * @notice EIP165 implementation.
+     */
+    function supportsInterface(bytes4 _interfaceId) public pure returns (bool) {
+        bytes4 firstSupportedInterface = bytes4(keccak256("supportsInterface(bytes4)")); // ERC165
+        bytes4 secondSupportedInterface = ILegacyMintableERC20.l1Token.selector ^
+            ILegacyMintableERC20.mint.selector ^
+            ILegacyMintableERC20.burn.selector;
+        return _interfaceId == firstSupportedInterface || _interfaceId == secondSupportedInterface;
+    }
+
+    /**
+     * @notice Only the bridge can mint tokens.
+     * @param _to     The account receiving tokens.
+     * @param _amount The amount of tokens to receive.
+     */
+    function mint(address _to, uint256 _amount) public virtual onlyL2Bridge {
+        _mint(_to, _amount);
+
+        emit Mint(_to, _amount);
+    }
+
+    /**
+     * @notice Only the bridge can burn tokens.
+     * @param _from   The account having tokens burnt.
+     * @param _amount The amount of tokens being burnt.
+     */
+    function burn(address _from, uint256 _amount) public virtual onlyL2Bridge {
+        _burn(_from, _amount);
+
+        emit Burn(_from, _amount);
+    }
+}

packages/contracts-bedrock/contracts/test/CommonTest.t.sol

@@ -27,6 +27,7 @@ import { AddressManager } from "../legacy/AddressManager.sol";
 import { L1ChugSplashProxy } from "../legacy/L1ChugSplashProxy.sol";
 import { IL1ChugSplashDeployer } from "../legacy/L1ChugSplashProxy.sol";
 import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
+import { LegacyMintableERC20 } from "../legacy/LegacyMintableERC20.sol";
 
 contract CommonTest is Test {
     address alice = address(128);
@@ -276,6 +277,7 @@ contract Bridge_Initializer is Messenger_Initializer {
     ERC20 L1Token;
     ERC20 BadL1Token;
     OptimismMintableERC20 L2Token;
+    LegacyMintableERC20 LegacyL2Token;
     ERC20 NativeL2Token;
     ERC20 BadL2Token;
     OptimismMintableERC20 RemoteL1Token;
@@ -398,6 +400,14 @@ contract Bridge_Initializer is Messenger_Initializer {
 
         L1Token = new ERC20("Native L1 Token", "L1T");
 
+        LegacyL2Token = new LegacyMintableERC20({
+            _l2Bridge: address(L2Bridge),
+            _l1Token: address(L1Token),
+            _name: string.concat("LegacyL2-", L1Token.name()),
+            _symbol: string.concat("LegacyL2-", L1Token.symbol())
+        });
+        vm.label(address(LegacyL2Token), "LegacyMintableERC20");
+
         // Deploy the L2 ERC20 now
         L2Token = OptimismMintableERC20(
             L2TokenFactory.createStandardL2Token(

packages/contracts-bedrock/contracts/test/L2StandardBridge.t.sol

@@ -155,15 +155,15 @@ contract L2StandardBridge_Test is Bridge_Initializer {
 contract PreBridgeERC20 is Bridge_Initializer {
     // withdraw and BridgeERC20 should behave the same when transferring ERC20 tokens
     // so they should share the same setup and expectEmit calls
-    function _preBridgeERC20(bool isLegacy) internal {
+    function _preBridgeERC20(bool _isLegacy, address _l2Token) internal {
         // Alice has 100 L2Token
-        deal(address(L2Token), alice, 100, true);
-        assertEq(L2Token.balanceOf(alice), 100);
+        deal(_l2Token, alice, 100, true);
+        assertEq(ERC20(_l2Token).balanceOf(alice), 100);
         uint256 nonce = L2Messenger.messageNonce();
         bytes memory message = abi.encodeWithSelector(
             StandardBridge.finalizeBridgeERC20.selector,
             address(L1Token),
-            address(L2Token),
+            _l2Token,
             alice,
             alice,
             100,
@@ -190,23 +190,17 @@ contract PreBridgeERC20 is Bridge_Initializer {
             })
         );
 
-        if (isLegacy) {
+        if (_isLegacy) {
             vm.expectCall(
                 address(L2Bridge),
-                abi.encodeWithSelector(
-                    L2Bridge.withdraw.selector,
-                    address(L2Token),
-                    100,
-                    1000,
-                    hex""
-                )
+                abi.encodeWithSelector(L2Bridge.withdraw.selector, _l2Token, 100, 1000, hex"")
             );
         } else {
             vm.expectCall(
                 address(L2Bridge),
                 abi.encodeWithSelector(
                     L2Bridge.bridgeERC20.selector,
-                    address(L2Token),
+                    _l2Token,
                     address(L1Token),
                     100,
                     1000,
@@ -237,15 +231,15 @@ contract PreBridgeERC20 is Bridge_Initializer {
 
         // The L2Bridge should burn the tokens
         vm.expectCall(
-            address(L2Token),
+            _l2Token,
             abi.encodeWithSelector(OptimismMintableERC20.burn.selector, alice, 100)
         );
 
         vm.expectEmit(true, true, true, true);
-        emit WithdrawalInitiated(address(L1Token), address(L2Token), alice, alice, 100, hex"");
+        emit WithdrawalInitiated(address(L1Token), _l2Token, alice, alice, 100, hex"");
 
         vm.expectEmit(true, true, true, true);
-        emit ERC20BridgeInitiated(address(L2Token), address(L1Token), alice, alice, 100, hex"");
+        emit ERC20BridgeInitiated(_l2Token, address(L1Token), alice, alice, 100, hex"");
 
         vm.expectEmit(true, true, true, true);
         emit MessagePassed(
@@ -276,7 +270,7 @@ contract L2StandardBridge_BridgeERC20_Test is PreBridgeERC20 {
     // - emits WithdrawalInitiated
     // - calls Withdrawer.initiateWithdrawal
     function test_withdraw_withdrawingERC20_succeeds() external {
-        _preBridgeERC20({ isLegacy: true });
+        _preBridgeERC20({ _isLegacy: true, _l2Token: address(L2Token) });
         L2Bridge.withdraw(address(L2Token), 100, 1000, hex"");
 
         assertEq(L2Token.balanceOf(alice), 0);
@@ -287,12 +281,26 @@ contract L2StandardBridge_BridgeERC20_Test is PreBridgeERC20 {
     // - emits WithdrawalInitiated
     // - calls Withdrawer.initiateWithdrawal
     function test_bridgeERC20_succeeds() external {
-        _preBridgeERC20({ isLegacy: false });
+        _preBridgeERC20({ _isLegacy: false, _l2Token: address(L2Token) });
         L2Bridge.bridgeERC20(address(L2Token), address(L1Token), 100, 1000, hex"");
 
         assertEq(L2Token.balanceOf(alice), 0);
     }
 
+    function test_withdrawLegacyERC20_succeeds() external {
+        _preBridgeERC20({ _isLegacy: true, _l2Token: address(LegacyL2Token) });
+        L2Bridge.withdraw(address(LegacyL2Token), 100, 1000, hex"");
+
+        assertEq(L2Token.balanceOf(alice), 0);
+    }
+
+    function test_bridgeLegacyERC20_succeeds() external {
+        _preBridgeERC20({ _isLegacy: false, _l2Token: address(LegacyL2Token) });
+        L2Bridge.bridgeERC20(address(LegacyL2Token), address(L1Token), 100, 1000, hex"");
+
+        assertEq(L2Token.balanceOf(alice), 0);
+    }
+
     function test_withdraw_notEOA_reverts() external {
         // This contract has 100 L2Token
         deal(address(L2Token), address(this), 100, true);
@@ -305,14 +313,14 @@ contract L2StandardBridge_BridgeERC20_Test is PreBridgeERC20 {
 contract PreBridgeERC20To is Bridge_Initializer {
     // withdrawTo and BridgeERC20To should behave the same when transferring ERC20 tokens
     // so they should share the same setup and expectEmit calls
-    function _preBridgeERC20To(bool isLegacy) internal {
-        deal(address(L2Token), alice, 100, true);
-        assertEq(L2Token.balanceOf(alice), 100);
+    function _preBridgeERC20To(bool _isLegacy, address _l2Token) internal {
+        deal(_l2Token, alice, 100, true);
+        assertEq(ERC20(L2Token).balanceOf(alice), 100);
         uint256 nonce = L2Messenger.messageNonce();
         bytes memory message = abi.encodeWithSelector(
             StandardBridge.finalizeBridgeERC20.selector,
             address(L1Token),
-            address(L2Token),
+            _l2Token,
             alice,
             bob,
             100,
@@ -340,10 +348,10 @@ contract PreBridgeERC20To is Bridge_Initializer {
         );
 
         vm.expectEmit(true, true, true, true, address(L2Bridge));
-        emit WithdrawalInitiated(address(L1Token), address(L2Token), alice, bob, 100, hex"");
+        emit WithdrawalInitiated(address(L1Token), _l2Token, alice, bob, 100, hex"");
 
         vm.expectEmit(true, true, true, true, address(L2Bridge));
-        emit ERC20BridgeInitiated(address(L2Token), address(L1Token), alice, bob, 100, hex"");
+        emit ERC20BridgeInitiated(_l2Token, address(L1Token), alice, bob, 100, hex"");
 
         vm.expectEmit(true, true, true, true, address(messagePasser));
         emit MessagePassed(
@@ -364,12 +372,12 @@ contract PreBridgeERC20To is Bridge_Initializer {
         vm.expectEmit(true, true, true, true, address(L2Messenger));
         emit SentMessageExtension1(address(L2Bridge), 0);
 
-        if (isLegacy) {
+        if (_isLegacy) {
             vm.expectCall(
                 address(L2Bridge),
                 abi.encodeWithSelector(
                     L2Bridge.withdrawTo.selector,
-                    address(L2Token),
+                    _l2Token,
                     bob,
                     100,
                     1000,
@@ -381,7 +389,7 @@ contract PreBridgeERC20To is Bridge_Initializer {
                 address(L2Bridge),
                 abi.encodeWithSelector(
                     L2Bridge.bridgeERC20To.selector,
-                    address(L2Token),
+                    _l2Token,
                     address(L1Token),
                     bob,
                     100,
@@ -427,7 +435,7 @@ contract L2StandardBridge_BridgeERC20To_Test is PreBridgeERC20To {
     // - emits WithdrawalInitiated w/ correct recipient
     // - calls Withdrawer.initiateWithdrawal
     function test_withdrawTo_withdrawingERC20_succeeds() external {
-        _preBridgeERC20To({ isLegacy: true });
+        _preBridgeERC20To({ _isLegacy: true, _l2Token: address(L2Token) });
         L2Bridge.withdrawTo(address(L2Token), bob, 100, 1000, hex"");
 
         assertEq(L2Token.balanceOf(alice), 0);
@@ -438,7 +446,7 @@ contract L2StandardBridge_BridgeERC20To_Test is PreBridgeERC20To {
     // - emits WithdrawalInitiated w/ correct recipient
     // - calls Withdrawer.initiateWithdrawal
     function test_bridgeERC20To_succeeds() external {
-        _preBridgeERC20To({ isLegacy: false });
+        _preBridgeERC20To({ _isLegacy: false, _l2Token: address(L2Token) });
         L2Bridge.bridgeERC20To(address(L2Token), address(L1Token), bob, 100, 1000, hex"");
         assertEq(L2Token.balanceOf(alice), 0);
     }

packages/contracts-bedrock/contracts/test/StandardBridge.t.sol

+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.15;
+
+import { StandardBridge } from "../universal/StandardBridge.sol";
+import { CommonTest } from "./CommonTest.t.sol";
+import {
+    OptimismMintableERC20,
+    ILegacyMintableERC20
+} from "../universal/OptimismMintableERC20.sol";
+import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+/**
+ * @title StandardBridgeTester
+ * @notice Simple wrapper around the StandardBridge contract that exposes
+ *         internal functions so they can be more easily tested directly.
+ */
+contract StandardBridgeTester is StandardBridge {
+    constructor(address payable _messenger, address payable _otherBridge)
+        StandardBridge(_messenger, _otherBridge)
+    {}
+
+    function isOptimismMintableERC20(address _token) external view returns (bool) {
+        return _isOptimismMintableERC20(_token);
+    }
+
+    function isCorrectTokenPair(address _mintableToken, address _otherToken)
+        external
+        view
+        returns (bool)
+    {
+        return _isCorrectTokenPair(_mintableToken, _otherToken);
+    }
+
+    receive() external payable override {}
+}
+
+/**
+ * @title LegacyMintable
+ * @notice Simple implementation of the legacy OptimismMintableERC20.
+ */
+contract LegacyMintable is ERC20, ILegacyMintableERC20 {
+    constructor(string memory _name, string memory _ticker) ERC20(_name, _ticker) {}
+
+    function l1Token() external view returns (address) {
+        return address(0);
+    }
+
+    function mint(address _to, uint256 _amount) external pure {}
+
+    function burn(address _from, uint256 _amount) external pure {}
+
+    /**
+     * @notice Implements ERC165. This implementation should not be changed as
+     *         it is how the actual legacy optimism mintable token does the
+     *         check. Allows for testing against code that is has been deployed,
+     *         assuming different compiler version is no problem.
+     */
+    function supportsInterface(bytes4 _interfaceId) external view returns (bool) {
+        bytes4 firstSupportedInterface = bytes4(keccak256("supportsInterface(bytes4)")); // ERC165
+        bytes4 secondSupportedInterface = ILegacyMintableERC20.l1Token.selector ^
+            ILegacyMintableERC20.mint.selector ^
+            ILegacyMintableERC20.burn.selector;
+        return _interfaceId == firstSupportedInterface || _interfaceId == secondSupportedInterface;
+    }
+}
+
+/**
+ * @title StandardBridge_Stateless_Test
+ * @notice Tests internal functions that require no existing state or contract
+ *         interactions with the messenger.
+ */
+contract StandardBridge_Stateless_Test is CommonTest {
+    StandardBridgeTester internal bridge;
+    OptimismMintableERC20 internal mintable;
+    ERC20 internal erc20;
+    LegacyMintable internal legacy;
+
+    function setUp() public override {
+        super.setUp();
+
+        bridge = new StandardBridgeTester({
+            _messenger: payable(address(0)),
+            _otherBridge: payable(address(0))
+        });
+
+        mintable = new OptimismMintableERC20({
+            _bridge: address(0),
+            _remoteToken: address(0),
+            _name: "Stonks",
+            _symbol: "STONK"
+        });
+
+        erc20 = new ERC20("Altcoin", "ALT");
+        legacy = new LegacyMintable("Legacy", "LEG");
+    }
+
+    /**
+     * @notice Test coverage for identifying OptimismMintableERC20 tokens.
+     *         This function should return true for both modern and legacy
+     *         OptimismMintableERC20 tokens and false for any accounts that
+     *         do not implement the interface.
+     */
+    function test_isOptimismMintableERC20_succeeds() external {
+        // Both the modern and legacy mintable tokens should return true
+        assertTrue(bridge.isOptimismMintableERC20(address(mintable)));
+        assertTrue(bridge.isOptimismMintableERC20(address(legacy)));
+        // A regular ERC20 should return false
+        assertFalse(bridge.isOptimismMintableERC20(address(erc20)));
+        // Non existent contracts should return false and not revert
+        assertEq(address(0x20).code.length, 0);
+        assertFalse(bridge.isOptimismMintableERC20(address(0x20)));
+    }
+
+    /**
+     * @notice Test coverage of isCorrectTokenPair under different types of
+     *         tokens.
+     */
+    function test_isCorrectTokenPair_succeeds() external {
+        // Modern + known to be correct remote token
+        assertTrue(bridge.isCorrectTokenPair(address(mintable), mintable.remoteToken()));
+        // Modern + known to be correct l1Token (legacy interface)
+        assertTrue(bridge.isCorrectTokenPair(address(mintable), mintable.l1Token()));
+        // Modern + known to be incorrect remote token
+        assertTrue(mintable.remoteToken() != address(0x20));
+        assertFalse(bridge.isCorrectTokenPair(address(mintable), address(0x20)));
+        // Legacy + known to be correct l1Token
+        assertTrue(bridge.isCorrectTokenPair(address(legacy), legacy.l1Token()));
+        // Legacy + known to be incorrect l1Token
+        assertTrue(legacy.l1Token() != address(0x20));
+        assertFalse(bridge.isCorrectTokenPair(address(legacy), address(0x20)));
+        // A token that doesn't support either modern or legacy interface
+        // will revert
+        vm.expectRevert();
+        bridge.isCorrectTokenPair(address(erc20), address(1));
+    }
+}

packages/contracts-bedrock/contracts/universal/IOptimismMintableERC20.sol

@@ -10,7 +10,7 @@ import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol
  *         OptimismMintableERC20.
  */
 interface IOptimismMintableERC20 is IERC165 {
-    function remoteToken() external returns (address);
+    function remoteToken() external view returns (address);
 
     function bridge() external returns (address);
 
@@ -26,7 +26,7 @@ interface IOptimismMintableERC20 is IERC165 {
  *         on the OptimismMintableERC20 contract for backwards compatibility.
  */
 interface ILegacyMintableERC20 is IERC165 {
-    function l1Token() external returns (address);
+    function l1Token() external view returns (address);
 
     function mint(address _to, uint256 _amount) external;
 

packages/contracts-bedrock/contracts/universal/StandardBridge.sol

@@ -458,6 +458,8 @@ abstract contract StandardBridge {
 
     /**
      * @notice Checks if the "other token" is the correct pair token for the OptimismMintableERC20.
+     *         Calls can be saved in the future by combining this logic with
+     *         `_isOptimismMintableERC20`.
      *
      * @param _mintableToken OptimismMintableERC20 to check against.
      * @param _otherToken    Pair token to check.
@@ -469,7 +471,13 @@ abstract contract StandardBridge {
         view
         returns (bool)
     {
-        return _otherToken == OptimismMintableERC20(_mintableToken).l1Token();
+        if (
+            ERC165Checker.supportsInterface(_mintableToken, type(ILegacyMintableERC20).interfaceId)
+        ) {
+            return _otherToken == ILegacyMintableERC20(_mintableToken).l1Token();
+        } else {
+            return _otherToken == IOptimismMintableERC20(_mintableToken).remoteToken();
+        }
     }
 
     /** @notice Emits the ETHBridgeInitiated event and if necessary the appropriate legacy event