Make gateways upgradable

.gitmodules

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

README.md

@@ -9,6 +9,8 @@ The Arbitrum Token Bridge is a [custom Arbitrum bridge](https://docs.arbitrum.io
 - `L1TokenGateway.sol` - L1 side of the bridge. Transfers the deposited tokens into an escrow contract. Transfer them back to the user upon receiving a withdrawal message from the `L2TokenGateway`.
 - `L2TokenGateway.sol` - L2 side of the bridge. Mints new L2 tokens after receiving a deposit message from `L1TokenGateway`. Burns L2 tokens when withdrawing them to L1.
 
+The `L1TokenGateway` and `L2TokenGateway` contracts use the ERC-1822 UUPS pattern for upgradeability and the ERC-1967 proxy storage slots standard. It is important that the `TokenGatewayDeploy` 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.
@@ -29,8 +31,14 @@ To withdraw her tokens back to L1, Alice calls `outboundTransfer()` on the `L2To
 
 ## Upgrades
 
+### Upgrade the bridge implementation(s)
+
+`L1TokenGateway` and/or `L2TokenGateway` 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 `L1TokenGateway` and `L2TokenGateway` 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.
 
@@ -45,6 +53,13 @@ To migrate a single token to a new bridge, follow the steps below:
 
 Note that step 3 is required because unregistering the token on `L2TokenGateway` not only removes the ability to initiate new L2 to L1 transfers but also causes the finalization of pending L1 to L2 transfers to revert. This is a point of difference with the implementation of the Arbitrum generic-custom gateway, where a missing L2 token triggers a withdrawal of the tokens back to L1 instead of a revert.
 
+## 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/L1TokenGatewayInstance.sol

@@ -0,0 +1,22 @@
+// SPDX-FileCopyrightText: © 2024 Dai Foundation <www.daifoundation.org>
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+pragma solidity >=0.8.0;
+
+struct L1TokenGatewayInstance {
+    address gateway;
+    address gatewayImp;
+}

deploy/L2TokenGatewayInstance.sol

@@ -18,5 +18,6 @@ pragma solidity >=0.8.0;
 
 struct L2TokenGatewayInstance {
     address gateway;
+    address gatewayImp;
     address spell;
 }

deploy/L2TokenGatewaySpell.sol

@@ -20,6 +20,9 @@ interface L2TokenGatewayLike {
     function isOpen() external view returns (uint256);
     function counterpartGateway() external view returns (address);
     function l2Router() 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;
@@ -39,6 +42,7 @@ contract L2TokenGatewaySpell {
         l2Gateway = L2TokenGatewayLike(l2Gateway_);
     }
 
+    function upgradeToAndCall(address newImp, bytes memory data) external { l2Gateway.upgradeToAndCall(newImp, data); }
     function rely(address usr) external { l2Gateway.rely(usr); }
     function deny(address usr) external { l2Gateway.deny(usr); }
     function close() external { l2Gateway.close(); }
@@ -60,6 +64,7 @@ contract L2TokenGatewaySpell {
 
     function init(
         address l2Gateway_,
+        address l2GatewayImp,
         address counterpartGateway,
         address l2Router,
         address[] calldata l1Tokens,
@@ -68,6 +73,8 @@ contract L2TokenGatewaySpell {
     ) external {
         // sanity checks
         require(address(l2Gateway) == l2Gateway_, "L2TokenGatewaySpell/l2-gateway-mismatch");
+        require(keccak256(bytes(l2Gateway.version())) == keccak256("1"), "L2TokenGatewaySpell/version-does-not-match");
+        require(l2Gateway.getImplementation() == l2GatewayImp, "L2TokenGatewaySpell/imp-does-not-match");
         require(l2Gateway.isOpen() == 1, "L2TokenGatewaySpell/not-open");
         require(l2Gateway.counterpartGateway() == counterpartGateway, "L2TokenGatewaySpell/counterpart-gateway-mismatch");
         require(l2Gateway.l2Router() == l2Router, "L2TokenGatewaySpell/l2-router-mismatch");

deploy/TokenGatewayDeploy.sol

@@ -18,6 +18,8 @@ pragma solidity >=0.8.0;
 
 import { ScriptTools } from "dss-test/ScriptTools.sol";
 
+import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import { L1TokenGatewayInstance } from "./L1TokenGatewayInstance.sol";
 import { L2TokenGatewayInstance } from "./L2TokenGatewayInstance.sol";
 import { L2TokenGatewaySpell } from "./L2TokenGatewaySpell.sol";
 import { L1TokenGateway } from "src/L1TokenGateway.sol";
@@ -30,9 +32,10 @@ library TokenGatewayDeploy {
         address l2Gateway,
         address l1Router,
         address inbox
-    ) internal returns (address l1Gateway) {
-        l1Gateway = address(new L1TokenGateway(l2Gateway, l1Router, inbox));
-        ScriptTools.switchOwner(l1Gateway, deployer, owner);
+    ) internal returns (L1TokenGatewayInstance memory l1GatewayInstance) {
+        l1GatewayInstance.gatewayImp = address(new L1TokenGateway(l2Gateway, l1Router, inbox));
+        l1GatewayInstance.gateway = address(new ERC1967Proxy(l1GatewayInstance.gatewayImp, abi.encodeCall(L1TokenGateway.initialize, ())));
+        ScriptTools.switchOwner(l1GatewayInstance.gateway, deployer, owner);
     }
 
     function deployL2Gateway(
@@ -41,7 +44,8 @@ library TokenGatewayDeploy {
         address l1Gateway,
         address l2Router
     ) internal returns (L2TokenGatewayInstance memory l2GatewayInstance) {
-        l2GatewayInstance.gateway = address(new L2TokenGateway(l1Gateway, l2Router));
+        l2GatewayInstance.gatewayImp = address(new L2TokenGateway(l1Gateway, l2Router));
+        l2GatewayInstance.gateway = address(new ERC1967Proxy(l2GatewayInstance.gatewayImp, abi.encodeCall(L2TokenGateway.initialize, ())));
         l2GatewayInstance.spell = address(new L2TokenGatewaySpell(l2GatewayInstance.gateway));
         ScriptTools.switchOwner(l2GatewayInstance.gateway, deployer, owner);
     }

deploy/TokenGatewayInit.sol

@@ -18,6 +18,7 @@ pragma solidity >=0.8.0;
 
 import { DssInstance } from "dss-test/MCD.sol";
 import { L2TokenGatewayInstance } from "./L2TokenGatewayInstance.sol";
+import { L1TokenGatewayInstance } from "./L1TokenGatewayInstance.sol";
 import { L2TokenGatewaySpell } from "./L2TokenGatewaySpell.sol";
 
 interface L1TokenGatewayLike {
@@ -26,6 +27,8 @@ interface L1TokenGatewayLike {
     function counterpartGateway() external view returns (address);
     function l1Router() external view returns (address);
     function inbox() 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;
 }
@@ -67,16 +70,18 @@ struct GatewaysConfig {
 library TokenGatewayInit {
     function initGateways(
         DssInstance memory            dss,
-        address                       l1Gateway_,
+        L1TokenGatewayInstance memory l1GatewayInstance,
         L2TokenGatewayInstance memory l2GatewayInstance,
         GatewaysConfig memory         cfg
     ) internal {
-        L1TokenGatewayLike l1Gateway = L1TokenGatewayLike(l1Gateway_);
+        L1TokenGatewayLike l1Gateway = L1TokenGatewayLike(l1GatewayInstance.gateway);
         L1RelayLike       l1GovRelay = L1RelayLike(dss.chainlog.getAddress("ARBITRUM_GOV_RELAY"));
         EscrowLike            escrow = EscrowLike(dss.chainlog.getAddress("ARBITRUM_ESCROW"));
         L1RouterLike        l1Router = L1RouterLike(cfg.l1Router);
 
         // sanity checks
+        require(keccak256(bytes(l1Gateway.version())) == keccak256("1"), "TokenGatewayInit/version-does-not-match");
+        require(l1Gateway.getImplementation() == l1GatewayInstance.gatewayImp, "TokenGatewayInit/imp-does-not-match");
         require(l1Gateway.isOpen() == 1, "TokenGatewayInit/not-open");
         require(l1Gateway.counterpartGateway() == l2GatewayInstance.gateway, "TokenGatewayInit/counterpart-gateway-mismatch");
         require(l1Gateway.l1Router() == cfg.l1Router, "TokenGatewayInit/l1-router-mismatch");
@@ -100,14 +105,15 @@ library TokenGatewayInit {
             require(l1Gateway.l1ToL2Token(l1Token) == address(0), "TokenGatewayInit/existing-l1-token");
 
             l1Gateway.registerToken(l1Token, l2Token);
-            escrow.approve(l1Token, l1Gateway_, type(uint256).max);
+            escrow.approve(l1Token, l1GatewayInstance.gateway, type(uint256).max);
         }
 
         l1GovRelay.relay({
             target:            l2GatewayInstance.spell,
             targetData:        abi.encodeCall(L2TokenGatewaySpell.init, (
                 l2GatewayInstance.gateway,
-                l1Gateway_,
+                l2GatewayInstance.gatewayImp,
+                l1GatewayInstance.gateway,
                 l1Router.counterpartGateway(),
                 cfg.l1Tokens,
                 cfg.l2Tokens,
@@ -119,6 +125,7 @@ library TokenGatewayInit {
             maxSubmissionCost: cfg.xchainMsg.maxSubmissionCost
         });
 
-        dss.chainlog.setAddress("ARBITRUM_TOKEN_BRIDGE", l1Gateway_);
+        dss.chainlog.setAddress("ARBITRUM_TOKEN_BRIDGE",     l1GatewayInstance.gateway);
+        dss.chainlog.setAddress("ARBITRUM_TOKEN_BRIDGE_IMP", l1GatewayInstance.gatewayImp);
     }
 }

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

@@ -21,7 +21,7 @@ import "forge-std/Script.sol";
 
 import { ScriptTools } from "dss-test/ScriptTools.sol";
 import { Domain } from "dss-test/domains/Domain.sol";
-import { TokenGatewayDeploy, L2TokenGatewayInstance } from "deploy/TokenGatewayDeploy.sol";
+import { TokenGatewayDeploy, L1TokenGatewayInstance, L2TokenGatewayInstance } from "deploy/TokenGatewayDeploy.sol";
 import { ChainLog } from "deploy/mocks/ChainLog.sol";
 import { L1Escrow } from "deploy/mocks/L1Escrow.sol";
 import { L1GovernanceRelay } from "deploy/mocks/L1GovernanceRelay.sol";
@@ -118,19 +118,19 @@ contract Deploy is Script {
         // L1 deployment
 
         l2Domain.selectFork();
-        address l2Gateway = vm.computeCreateAddress(l2Deployer, vm.getNonce(l2Deployer));
+        address l2Gateway = vm.computeCreateAddress(l2Deployer, vm.getNonce(l2Deployer) + 1);
 
         l1Domain.selectFork();
         vm.startBroadcast(l1PrivKey);
-        address l1Gateway = TokenGatewayDeploy.deployL1Gateway(l1Deployer, owner, l2Gateway, l1Router, inbox);
+        L1TokenGatewayInstance memory l1GatewayInstance = TokenGatewayDeploy.deployL1Gateway(l1Deployer, owner, l2Gateway, l1Router, inbox);
         vm.stopBroadcast();
         address l2Router = L1RouterLike(l1Router).counterpartGateway();
 
         // L2 deployment
 
         l2Domain.selectFork();
         vm.startBroadcast(l2PrivKey);
-        L2TokenGatewayInstance memory l2GatewayInstance = TokenGatewayDeploy.deployL2Gateway(l2Deployer, l2GovRelay, l1Gateway, l2Router);
+        L2TokenGatewayInstance memory l2GatewayInstance = TokenGatewayDeploy.deployL2Gateway(l2Deployer, l2GovRelay, l1GatewayInstance.gateway, l2Router);
         require(l2GatewayInstance.gateway == l2Gateway, "l2Gateway address mismatch");
         vm.stopBroadcast();
 
@@ -144,8 +144,10 @@ contract Deploy is Script {
         ScriptTools.exportContract("deployed", "escrow", escrow);
         ScriptTools.exportContract("deployed", "l1GovRelay", l1GovRelay);
         ScriptTools.exportContract("deployed", "l2GovRelay", l2GovRelay);
-        ScriptTools.exportContract("deployed", "l1Gateway", l1Gateway);
+        ScriptTools.exportContract("deployed", "l1Gateway", l1GatewayInstance.gateway);
+        ScriptTools.exportContract("deployed", "l1GatewayImp", l1GatewayInstance.gatewayImp);
         ScriptTools.exportContract("deployed", "l2Gateway", l2Gateway);
+        ScriptTools.exportContract("deployed", "l2GatewayImp", l2GatewayInstance.gatewayImp);
         ScriptTools.exportContract("deployed", "l2GatewaySpell", l2GatewayInstance.spell);
         ScriptTools.exportContracts("deployed", "l1Tokens", l1Tokens);
         ScriptTools.exportContracts("deployed", "l2Tokens", l2Tokens);

script/Init.s.sol

@@ -23,6 +23,7 @@ import { ScriptTools } from "dss-test/ScriptTools.sol";
 import { Domain } from "dss-test/domains/Domain.sol";
 import { MCD, DssInstance } from "dss-test/MCD.sol";
 import { TokenGatewayInit, GatewaysConfig, MessageParams } from "deploy/TokenGatewayInit.sol";
+import { L1TokenGatewayInstance } from "deploy/L1TokenGatewayInstance.sol";
 import { L2TokenGatewayInstance } from "deploy/L2TokenGatewayInstance.sol";
 import { L2TokenGatewaySpell } from "deploy/L2TokenGatewaySpell.sol";
 import { L2GovernanceRelay } from "deploy/mocks/L2GovernanceRelay.sol";
@@ -64,6 +65,7 @@ contract Init is Script {
             deps.readAddress(".l2GatewaySpell"), 
             abi.encodeCall(L2TokenGatewaySpell.init, (
                 deps.readAddress(".l2Gateway"),
+                deps.readAddress(".l2GatewayImp"),
                 l1Gateway,
                 deps.readAddress(".l2Router"),
                 cfg.l1Tokens,
@@ -77,9 +79,14 @@ contract Init is Script {
             maxSubmissionCost: retryable.getSubmissionFee(initCalldata) * 250 / 100
         });
 
+        L1TokenGatewayInstance memory l1GatewayInstance = L1TokenGatewayInstance({
+            gateway:    deps.readAddress(".l1Gateway"),
+            gatewayImp: deps.readAddress(".l1GatewayImp")
+        });
         L2TokenGatewayInstance memory l2GatewayInstance = L2TokenGatewayInstance({
-            spell:   deps.readAddress(".l2GatewaySpell"),
-            gateway: deps.readAddress(".l2Gateway")
+            spell:      deps.readAddress(".l2GatewaySpell"),
+            gateway:    deps.readAddress(".l2Gateway"),
+            gatewayImp: deps.readAddress(".l2GatewayImp")
         });
 
         vm.startBroadcast(l1PrivKey);
@@ -89,7 +96,7 @@ contract Init is Script {
             require(success, "l1GovRelay topup failed");
         }
 
-        TokenGatewayInit.initGateways(dss, l1Gateway, l2GatewayInstance, cfg);
+        TokenGatewayInit.initGateways(dss, l1GatewayInstance, l2GatewayInstance, cfg);
         vm.stopBroadcast();
     }
 }

src/L1TokenGateway.sol

@@ -17,6 +17,7 @@
 
 pragma solidity ^0.8.21;
 
+import { UUPSUpgradeable, ERC1967Utils } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 import { ITokenGateway } from "src/arbitrum/ITokenGateway.sol";
 import { IL1ArbitrumGateway } from "src/arbitrum/IL1ArbitrumGateway.sol";
 import { ICustomGateway } from "src/arbitrum/ICustomGateway.sol";
@@ -27,19 +28,20 @@ interface TokenLike {
     function transferFrom(address, address, uint256) external;
 }
 
-contract L1TokenGateway is ITokenGateway, IL1ArbitrumGateway, ICustomGateway, ERC165, L1ArbitrumMessenger {
+contract L1TokenGateway is UUPSUpgradeable, ITokenGateway, IL1ArbitrumGateway, ICustomGateway, ERC165, L1ArbitrumMessenger {
     // --- 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 counterpartGateway;
     address public immutable l1Router;
     address public immutable inbox;
+    string public constant version = "1";
 
     // --- events ---
 
@@ -87,14 +89,29 @@ contract L1TokenGateway is ITokenGateway, IL1ArbitrumGateway, ICustomGateway, ER
         address _l1Router,
         address _inbox
     ) {
+        _disableInitializers(); // Avoid initializing in the context of the implementation
+
         counterpartGateway = _counterpartGateway;
         l1Router = _l1Router;
         inbox = _inbox;
+    }
+
+    // --- 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 {