Upgradable L1TokenBridge and L2TokenBridge (#7)

* Make bridges upgradable

* Fix spacing

* Add missing internal calls

* Address review comments

* Fix alignment

* Import only necessary types

* Remove extra space

---------

Co-authored-by: telome <>

.gitmodules

@@ -1,3 +1,9 @@
 [submodule "lib/dss-test"]
 	path = lib/dss-test
 	url = https://github.com/makerdao/dss-test
+[submodule "lib/openzeppelin-contracts-upgradeable"]
+	path = lib/openzeppelin-contracts-upgradeable
+	url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable
+[submodule "lib/openzeppelin-foundry-upgrades"]
+	path = lib/openzeppelin-foundry-upgrades
+	url = https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades

README.md

@@ -12,6 +12,8 @@ The OP Token Bridge is a [custom bridge](https://docs.optimism.io/builders/app-d
 - `L1GovernanceRelay.sol` - L1 side of the governance relay, which allows governance to exert admin control over the deployed L2 contracts.
 - `L2GovernanceRelay.sol` - L2 side of the governance relay.
 
+The `L1TokenBridge` and `L2TokenBridge` contracts use the ERC-1822 UUPS pattern for upgradeability and the ERC-1967 proxy storage slots standard. It is important that the `TokenBridgeDeploy` library sequences be used for deploying.
+
 ### External dependencies
 
 - The L2 implementations of the bridged tokens are not provided as part of this repository and are assumed to exist in external repositories. It is assumed that only simple, regular ERC20 tokens will be used with this bridge. In particular, the supported tokens are assumed to revert on failure (instead of returning false) and do not execute any hook on transfer.
@@ -28,8 +30,14 @@ To withdraw her tokens back to L1, Alice calls `bridgeERC20[To]()` on the `L2Tok
 
 ## Upgrades
 
+### Upgrade the bridge implementation(s)
+
+`L1TokenBridge` and/or `L2TokenBridge` implementations can be upgraded by calling the `upgradeToAndCall` function of their inherited `UUPSUpgradeable` parent. Special care must be taken to ensure any deposit or withdrawal that is in transit at the time of the upgrade will still be able to get confirmed on the destination side.
+
 ### Upgrade to a new bridge (and deprecate this bridge)
 
+As an alternative upgrade mechanism, a new bridge can be deployed to be used with the escrow.
+
 1. Deploy the new token bridge and connect it to the same escrow as the one used by this bridge. The old and new bridges can operate in parallel.
 2. Optionally, deprecate the old bridge by closing it. This involves calling `close()` on both the `L1TokenBridge` and `L2TokenBridge` so that no new outbound message can be sent to the other side of the bridge. After all cross-chain messages are done processing (can take ~1 week), the bridge is effectively closed and governance can consider revoking the approval to transfer funds from the escrow on L1 and the token minting rights on L2.
 
@@ -40,6 +48,13 @@ To migrate a single token to a new bridge, follow the steps below:
 1. Deploy the new token bridge and connect it to the same escrow as the one used by this bridge.
 2. Unregister the token on both `L1TokenBridge` and `L2TokenBridge`, so that no new outbound message can be sent to the other side of the bridge for that token.
 
+## Tests
+
+### OZ upgradeability validations
+
+The OZ validations can be run alongside the existing tests:  
+`VALIDATE=true forge test --ffi --build-info --extra-output storageLayout`
+
 ## Deployment
 
 ### Declare env variables

deploy/L1TokenBridgeInstance.sol

@@ -20,4 +20,5 @@ struct L1TokenBridgeInstance {
     address govRelay;
     address escrow;
     address bridge;
+    address bridgeImp;
 }

deploy/L2TokenBridgeInstance.sol

@@ -19,5 +19,6 @@ pragma solidity >=0.8.0;
 struct L2TokenBridgeInstance {
     address govRelay;
     address bridge;
+    address bridgeImp;
     address spell;
 }

deploy/L2TokenBridgeSpell.sol

@@ -25,6 +25,9 @@ interface L2TokenBridgeLike {
     function isOpen() external view returns (uint256);
     function otherBridge() external view returns (address);
     function messenger() external view returns (address);
+    function version() external view returns (string memory);
+    function getImplementation() external view returns (address);
+    function upgradeToAndCall(address, bytes memory) external;
     function rely(address) external;
     function deny(address) external;
     function close() external;
@@ -44,6 +47,7 @@ contract L2TokenBridgeSpell {
         l2Bridge = L2TokenBridgeLike(l2Bridge_);
     }
 
+    function upgradeToAndCall(address newImp, bytes memory data) external { l2Bridge.upgradeToAndCall(newImp, data); }
     function rely(address usr) external { l2Bridge.rely(usr); }
     function deny(address usr) external { l2Bridge.deny(usr); }
     function close() external { l2Bridge.close(); }
@@ -66,6 +70,7 @@ contract L2TokenBridgeSpell {
     function init(
         address l2GovRelay_,
         address l2Bridge_,
+        address l2BridgeImp,
         address l1GovRelay,
         address l1Bridge,
         address l2Messenger,
@@ -77,6 +82,8 @@ contract L2TokenBridgeSpell {
 
         // sanity checks
         require(address(l2Bridge) == l2Bridge_, "L2TokenBridgeSpell/l2-bridge-mismatch");
+        require(keccak256(bytes(l2Bridge.version())) == keccak256("1"), "L2TokenBridgeSpell/version-does-not-match");
+        require(l2Bridge.getImplementation() == l2BridgeImp, "L2TokenBridgeSpell/imp-does-not-match");
         require(l2Bridge.isOpen() == 1, "L2TokenBridgeSpell/not-open");
         require(l2Bridge.otherBridge() == l1Bridge, "L2TokenBridgeSpell/other-bridge-mismatch");
         require(l2Bridge.messenger() == l2Messenger, "L2TokenBridgeSpell/l2-bridge-messenger-mismatch");

deploy/TokenBridgeDeploy.sol

@@ -18,6 +18,7 @@ pragma solidity >=0.8.0;
 
 import { ScriptTools } from "dss-test/ScriptTools.sol";
 
+import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import { L1TokenBridgeInstance } from "./L1TokenBridgeInstance.sol";
 import { L2TokenBridgeInstance } from "./L2TokenBridgeInstance.sol";
 import { L2TokenBridgeSpell } from "./L2TokenBridgeSpell.sol";
@@ -37,7 +38,9 @@ library TokenBridgeDeploy {
     ) internal returns (L1TokenBridgeInstance memory l1BridgeInstance) {
         l1BridgeInstance.govRelay = address(new L1GovernanceRelay(l2GovRelay, l1Messenger));
         l1BridgeInstance.escrow = address(new Escrow());
-        l1BridgeInstance.bridge = address(new L1TokenBridge(l2Bridge, l1Messenger));
+        l1BridgeInstance.bridgeImp = address(new L1TokenBridge(l2Bridge, l1Messenger));
+        l1BridgeInstance.bridge = address(new ERC1967Proxy(l1BridgeInstance.bridgeImp, abi.encodeCall(L1TokenBridge.initialize, ())));
+
         ScriptTools.switchOwner(l1BridgeInstance.govRelay, deployer, owner);
         ScriptTools.switchOwner(l1BridgeInstance.escrow, deployer, owner);
         ScriptTools.switchOwner(l1BridgeInstance.bridge, deployer, owner);
@@ -50,7 +53,8 @@ library TokenBridgeDeploy {
         address l2Messenger
     ) internal returns (L2TokenBridgeInstance memory l2BridgeInstance) {
         l2BridgeInstance.govRelay = address(new L2GovernanceRelay(l1GovRelay, l2Messenger));
-        l2BridgeInstance.bridge = address(new L2TokenBridge(l1Bridge, l2Messenger));
+        l2BridgeInstance.bridgeImp = address(new L2TokenBridge(l1Bridge, l2Messenger));
+        l2BridgeInstance.bridge = address(new ERC1967Proxy(l2BridgeInstance.bridgeImp, abi.encodeCall(L2TokenBridge.initialize, ())));
         l2BridgeInstance.spell = address(new L2TokenBridgeSpell(l2BridgeInstance.bridge));
         ScriptTools.switchOwner(l2BridgeInstance.bridge, deployer, l2BridgeInstance.govRelay);
     }

deploy/TokenBridgeInit.sol

@@ -26,6 +26,8 @@ interface L1TokenBridgeLike {
     function isOpen() external view returns (uint256);
     function otherBridge() external view returns (address);
     function messenger() external view returns (address);
+    function version() external view returns (string memory);
+    function getImplementation() external view returns (address);
     function file(bytes32, address) external;
     function registerToken(address, address) external;
 }
@@ -54,6 +56,7 @@ struct BridgesConfig {
     bytes32 govRelayCLKey;
     bytes32 escrowCLKey;
     bytes32 l1BridgeCLKey;
+    bytes32 l1BridgeImpCLKey;
 }
 
 library TokenBridgeInit {
@@ -68,6 +71,8 @@ library TokenBridgeInit {
         L1TokenBridgeLike l1Bridge = L1TokenBridgeLike(l1BridgeInstance.bridge);
 
         // sanity checks
+        require(keccak256(bytes(l1Bridge.version())) == keccak256("1"), "TokenBridgeInit/version-does-not-match");
+        require(l1Bridge.getImplementation() == l1BridgeInstance.bridgeImp, "TokenBridgeInit/imp-does-not-match");
         require(l1Bridge.isOpen() == 1, "TokenBridgeInit/not-open");
         require(l1Bridge.otherBridge() == l2BridgeInstance.bridge, "TokenBridgeInit/other-bridge-mismatch");
         require(l1Bridge.messenger() == cfg.l1Messenger, "TokenBridgeInit/l1-bridge-messenger-mismatch");
@@ -95,6 +100,7 @@ library TokenBridgeInit {
             targetData:  abi.encodeCall(L2TokenBridgeSpell.init, (
                 l2BridgeInstance.govRelay,
                 l2BridgeInstance.bridge,
+                l2BridgeInstance.bridgeImp,
                 address(l1GovRelay),
                 address(l1Bridge),
                 cfg.l2Messenger,
@@ -105,8 +111,9 @@ library TokenBridgeInit {
             minGasLimit: cfg.minGasLimit
         });
 
-        dss.chainlog.setAddress(cfg.govRelayCLKey, address(l1GovRelay));
-        dss.chainlog.setAddress(cfg.escrowCLKey,   address(escrow));
-        dss.chainlog.setAddress(cfg.l1BridgeCLKey, address(l1Bridge));
+        dss.chainlog.setAddress(cfg.govRelayCLKey,    address(l1GovRelay));
+        dss.chainlog.setAddress(cfg.escrowCLKey,      address(escrow));
+        dss.chainlog.setAddress(cfg.l1BridgeCLKey,    address(l1Bridge));
+        dss.chainlog.setAddress(cfg.l1BridgeImpCLKey, l1BridgeInstance.bridgeImp);
     }
 }

remappings.txt

@@ -0,0 +1,3 @@
+@openzeppelin/contracts/=lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/
+@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/
+forge-std/=lib/dss-test/lib/forge-std/src/

script/Deploy.s.sol

@@ -49,7 +49,7 @@ contract Deploy is Script {
 
         l2Domain.selectFork();
         address l2GovRelay = vm.computeCreateAddress(l2Deployer, vm.getNonce(l2Deployer));
-        address l2Bridge = vm.computeCreateAddress(l2Deployer, vm.getNonce(l2Deployer) + 1);
+        address l2Bridge = vm.computeCreateAddress(l2Deployer, vm.getNonce(l2Deployer) + 2);
 
         // Deploy chainlog, L1 gov relay, escrow and L1 bridge
 
@@ -130,7 +130,9 @@ contract Deploy is Script {
         ScriptTools.exportContract("deployed", "l1GovRelay", l1GovRelay);
         ScriptTools.exportContract("deployed", "l2GovRelay", l2GovRelay);
         ScriptTools.exportContract("deployed", "l1Bridge", l1Bridge);
+        ScriptTools.exportContract("deployed", "l1BridgeImp", l1BridgeInstance.bridgeImp);
         ScriptTools.exportContract("deployed", "l2Bridge", l2Bridge);
+        ScriptTools.exportContract("deployed", "l2BridgeImp", l2BridgeInstance.bridgeImp);
         ScriptTools.exportContract("deployed", "l2BridgeSpell", l2BridgeInstance.spell);
         ScriptTools.exportContracts("deployed", "l1Tokens", l1Tokens);
         ScriptTools.exportContracts("deployed", "l2Tokens", l2Tokens);

script/Init.s.sol

@@ -47,28 +47,31 @@ contract Init is Script {
         DssInstance memory dss = MCD.loadFromChainlog(deps.readAddress(".chainlog"));
 
         BridgesConfig memory cfg; 
-        cfg.l1Messenger   = deps.readAddress(".l1Messenger");
-        cfg.l2Messenger   = deps.readAddress(".l2Messenger");
-        cfg.l1Tokens      = deps.readAddressArray(".l1Tokens");
-        cfg.l2Tokens      = deps.readAddressArray(".l2Tokens");
-        cfg.maxWithdraws  = new uint256[](cfg.l2Tokens.length);
-        cfg.minGasLimit   = 100_000;
-        cfg.govRelayCLKey = l2Domain.readConfigBytes32FromString("govRelayCLKey");
-        cfg.escrowCLKey   = l2Domain.readConfigBytes32FromString("escrowCLKey");
-        cfg.l1BridgeCLKey = l2Domain.readConfigBytes32FromString("l1BridgeCLKey");
+        cfg.l1Messenger      = deps.readAddress(".l1Messenger");
+        cfg.l2Messenger      = deps.readAddress(".l2Messenger");
+        cfg.l1Tokens         = deps.readAddressArray(".l1Tokens");
+        cfg.l2Tokens         = deps.readAddressArray(".l2Tokens");
+        cfg.maxWithdraws     = new uint256[](cfg.l2Tokens.length);
+        cfg.minGasLimit      = 100_000;
+        cfg.govRelayCLKey    = l2Domain.readConfigBytes32FromString("govRelayCLKey");
+        cfg.escrowCLKey      = l2Domain.readConfigBytes32FromString("escrowCLKey");
+        cfg.l1BridgeCLKey    = l2Domain.readConfigBytes32FromString("l1BridgeCLKey");
+        cfg.l1BridgeImpCLKey = l2Domain.readConfigBytes32FromString("l1BridgeImpCLKey");
         for (uint256 i; i < cfg.maxWithdraws.length; ++i) {
             cfg.maxWithdraws[i] = 10_000_000 ether;
         }
 
         L1TokenBridgeInstance memory l1BridgeInstance = L1TokenBridgeInstance({
-            govRelay: deps.readAddress(".l1GovRelay"),
-            escrow:   deps.readAddress(".escrow"),
-            bridge:   deps.readAddress(".l1Bridge")
+            govRelay:  deps.readAddress(".l1GovRelay"),
+            escrow:    deps.readAddress(".escrow"),
+            bridge:    deps.readAddress(".l1Bridge"),
+            bridgeImp: deps.readAddress(".l1BridgeImp")
         });
         L2TokenBridgeInstance memory l2BridgeInstance = L2TokenBridgeInstance({
-            govRelay: deps.readAddress(".l2GovRelay"),
-            spell:    deps.readAddress(".l2BridgeSpell"),
-            bridge:   deps.readAddress(".l2Bridge")
+            govRelay:  deps.readAddress(".l2GovRelay"),
+            spell:     deps.readAddress(".l2BridgeSpell"),
+            bridge:    deps.readAddress(".l2Bridge"),
+            bridgeImp: deps.readAddress(".l2BridgeImp")
         });
 
         vm.startBroadcast(l1PrivKey);

script/input/1/config.json

@@ -10,7 +10,8 @@
       "tokens": [],
       "govRelayCLKey": "BASE_GOV_RELAY",
       "escrowCLKey": "BASE_ESCROW",
-      "l1BridgeCLKey": "BASE_TOKEN_BRIDGE"
+      "l1BridgeCLKey": "BASE_TOKEN_BRIDGE",
+      "l1BridgeImpCLKey": "BASE_TOKEN_BRIDGE_IMP"
     }
   }
 }

script/input/11155111/config.json

@@ -6,7 +6,8 @@
       "l2Messenger": "0x4200000000000000000000000000000000000007",
       "govRelayCLKey": "BASE_GOV_RELAY",
       "escrowCLKey": "BASE_ESCROW",
-      "l1BridgeCLKey": "BASE_TOKEN_BRIDGE"
+      "l1BridgeCLKey": "BASE_TOKEN_BRIDGE",
+      "l1BridgeImpCLKey": "BASE_TOKEN_BRIDGE_IMP"
     }
   }
 }

src/L1TokenBridge.sol

@@ -17,6 +17,8 @@
 
 pragma solidity ^0.8.21;
 
+import { UUPSUpgradeable, ERC1967Utils } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+
 interface TokenLike {
     function transferFrom(address, address, uint256) external;
 }
@@ -26,18 +28,19 @@ interface CrossDomainMessengerLike {
     function sendMessage(address _target, bytes calldata _message, uint32 _minGasLimit) external payable;
 }
 
-contract L1TokenBridge {
+contract L1TokenBridge is UUPSUpgradeable {
     // --- storage variables ---
 
     mapping(address => uint256) public wards;
     mapping(address => address) public l1ToL2Token;
-    uint256 public isOpen = 1;
+    uint256 public isOpen;
     address public escrow;
 
-    // --- immutables ---
+    // --- immutables and const ---
 
     address public immutable otherBridge;
     CrossDomainMessengerLike public immutable messenger;
+    string public constant version = "1";
 
     // --- events ---
 
@@ -84,13 +87,28 @@ contract L1TokenBridge {
         address _otherBridge,
         address _messenger
     ) {
+        _disableInitializers(); // Avoid initializing in the context of the implementation
+
         otherBridge = _otherBridge;
         messenger = CrossDomainMessengerLike(_messenger);
+    }
+
+    // --- upgradability ---
 
+    function initialize() initializer external {
+        __UUPSUpgradeable_init();
+
+        isOpen = 1;
         wards[msg.sender] = 1;
         emit Rely(msg.sender);
     }
 
+    function _authorizeUpgrade(address newImplementation) internal override auth {}
+
+    function getImplementation() external view returns (address) {
+        return ERC1967Utils.getImplementation();
+    }
+
     // --- administration ---
 
     function rely(address usr) external auth {

src/L2TokenBridge.sol

@@ -17,6 +17,8 @@
 
 pragma solidity ^0.8.21;
 
+import { UUPSUpgradeable, ERC1967Utils } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+
 interface TokenLike {
     function mint(address, uint256) external;
     function burn(address, uint256) external;
@@ -27,18 +29,19 @@ interface CrossDomainMessengerLike {
     function sendMessage(address _target, bytes calldata _message, uint32 _minGasLimit) external payable;
 }
 
-contract L2TokenBridge {
+contract L2TokenBridge is UUPSUpgradeable {
     // --- storage variables ---
 
     mapping(address => uint256) public wards;
     mapping(address => address) public l1ToL2Token;
     mapping(address => uint256) public maxWithdraws;
-    uint256 public isOpen = 1;
+    uint256 public isOpen;
 
-    // --- immutables ---
+    // --- immutables and const ---
 
     address public immutable otherBridge;
     CrossDomainMessengerLike public immutable messenger;
+    string public constant version = "1";
 
     // --- events ---
 
@@ -85,13 +88,28 @@ contract L2TokenBridge {
         address _otherBridge,
         address _messenger
     ) {
+        _disableInitializers(); // Avoid initializing in the context of the implementation
+
         otherBridge = _otherBridge;
         messenger = CrossDomainMessengerLike(_messenger);
+    }
+
+    // --- upgradability ---
 
+    function initialize() initializer external {
+        __UUPSUpgradeable_init();
+
+        isOpen = 1;
         wards[msg.sender] = 1;
         emit Rely(msg.sender);
     }
 
+    function _authorizeUpgrade(address newImplementation) internal override auth {}
+
+    function getImplementation() external view returns (address) {
+        return ERC1967Utils.getImplementation();
+    }
+
     // --- administration ---
 
     function rely(address usr) external auth {
@@ -133,7 +151,7 @@ contract L2TokenBridge {
         require(_localToken != address(0) && l1ToL2Token[_remoteToken] == _localToken, "L2TokenBridge/invalid-token");
         require(_amount <= maxWithdraws[_localToken], "L2TokenBridge/amount-too-large");
 
-        TokenLike(_localToken).burn(msg.sender, _amount); // TODO: should l2Tokens allow authed burn?
+        TokenLike(_localToken).burn(msg.sender, _amount);
 
         messenger.sendMessage({
             _target: address(otherBridge),