From 0f935505c0dc74ce3db2a9998320a56119321814 Mon Sep 17 00:00:00 2001 From: telome <130504305+telome@users.noreply.github.com> Date: Tue, 8 Oct 2024 07:40:14 +0400 Subject: [PATCH] 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 | 6 ++ README.md | 15 ++++ deploy/L1TokenBridgeInstance.sol | 1 + deploy/L2TokenBridgeInstance.sol | 1 + deploy/L2TokenBridgeSpell.sol | 7 ++ deploy/TokenBridgeDeploy.sol | 8 +- deploy/TokenBridgeInit.sol | 13 ++- lib/openzeppelin-contracts-upgradeable | 1 + lib/openzeppelin-foundry-upgrades | 1 + remappings.txt | 3 + script/Deploy.s.sol | 4 +- script/Init.s.sol | 33 ++++---- script/input/1/config.json | 3 +- script/input/11155111/config.json | 3 +- src/L1TokenBridge.sol | 24 +++++- src/L2TokenBridge.sol | 26 +++++- test/Integration.t.sol | 67 +++++++++++++--- test/L1TokenBridge.t.sol | 106 +++++++++++++++++++++---- test/L2TokenBridge.t.sol | 106 +++++++++++++++++++++---- test/mocks/L1TokenBridgeV2Mock.sol | 50 ++++++++++++ test/mocks/L2TokenBridgeV2Mock.sol | 50 ++++++++++++ 21 files changed, 458 insertions(+), 70 deletions(-) create mode 160000 lib/openzeppelin-contracts-upgradeable create mode 160000 lib/openzeppelin-foundry-upgrades create mode 100644 remappings.txt create mode 100644 test/mocks/L1TokenBridgeV2Mock.sol create mode 100644 test/mocks/L2TokenBridgeV2Mock.sol diff --git a/.gitmodules b/.gitmodules index a2df3f1..69a4d3e 100644 --- a/.gitmodules +++ b/.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 diff --git a/README.md b/README.md index d1123a0..09d2f7f 100644 --- a/README.md +++ b/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 diff --git a/deploy/L1TokenBridgeInstance.sol b/deploy/L1TokenBridgeInstance.sol index 1ed06a9..044a93b 100644 --- a/deploy/L1TokenBridgeInstance.sol +++ b/deploy/L1TokenBridgeInstance.sol @@ -20,4 +20,5 @@ struct L1TokenBridgeInstance { address govRelay; address escrow; address bridge; + address bridgeImp; } diff --git a/deploy/L2TokenBridgeInstance.sol b/deploy/L2TokenBridgeInstance.sol index a8d965b..4305e6d 100644 --- a/deploy/L2TokenBridgeInstance.sol +++ b/deploy/L2TokenBridgeInstance.sol @@ -19,5 +19,6 @@ pragma solidity >=0.8.0; struct L2TokenBridgeInstance { address govRelay; address bridge; + address bridgeImp; address spell; } diff --git a/deploy/L2TokenBridgeSpell.sol b/deploy/L2TokenBridgeSpell.sol index 57986cf..c3e125f 100644 --- a/deploy/L2TokenBridgeSpell.sol +++ b/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"); diff --git a/deploy/TokenBridgeDeploy.sol b/deploy/TokenBridgeDeploy.sol index 034584c..80dfcaa 100644 --- a/deploy/TokenBridgeDeploy.sol +++ b/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); } diff --git a/deploy/TokenBridgeInit.sol b/deploy/TokenBridgeInit.sol index e4e68f6..920894c 100644 --- a/deploy/TokenBridgeInit.sol +++ b/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); } } diff --git a/lib/openzeppelin-contracts-upgradeable b/lib/openzeppelin-contracts-upgradeable new file mode 160000 index 0000000..723f8ca --- /dev/null +++ b/lib/openzeppelin-contracts-upgradeable @@ -0,0 +1 @@ +Subproject commit 723f8cab09cdae1aca9ec9cc1cfa040c2d4b06c1 diff --git a/lib/openzeppelin-foundry-upgrades b/lib/openzeppelin-foundry-upgrades new file mode 160000 index 0000000..332bd33 --- /dev/null +++ b/lib/openzeppelin-foundry-upgrades @@ -0,0 +1 @@ +Subproject commit 332bd3306242e09520df2685b2edb99ebd7f5d37 diff --git a/remappings.txt b/remappings.txt new file mode 100644 index 0000000..7761e9f --- /dev/null +++ b/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/ \ No newline at end of file diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index 69f4480..7fa4c2c 100644 --- a/script/Deploy.s.sol +++ b/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); diff --git a/script/Init.s.sol b/script/Init.s.sol index 0ef5fc1..e519ec9 100644 --- a/script/Init.s.sol +++ b/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); diff --git a/script/input/1/config.json b/script/input/1/config.json index ffec488..23fb2d7 100644 --- a/script/input/1/config.json +++ b/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" } } } diff --git a/script/input/11155111/config.json b/script/input/11155111/config.json index c61e2c7..44d501a 100644 --- a/script/input/11155111/config.json +++ b/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" } } } diff --git a/src/L1TokenBridge.sol b/src/L1TokenBridge.sol index 43a55c8..49c2756 100644 --- a/src/L1TokenBridge.sol +++ b/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 { diff --git a/src/L2TokenBridge.sol b/src/L2TokenBridge.sol index beb960a..d5de9e5 100644 --- a/src/L2TokenBridge.sol +++ b/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), diff --git a/test/Integration.t.sol b/test/Integration.t.sol index 8520e4c..1328f86 100644 --- a/test/Integration.t.sol +++ b/test/Integration.t.sol @@ -28,7 +28,10 @@ import { L2TokenBridgeInstance } from "deploy/L2TokenBridgeInstance.sol"; import { TokenBridgeInit, BridgesConfig } from "deploy/TokenBridgeInit.sol"; import { L1TokenBridge } from "src/L1TokenBridge.sol"; import { L2TokenBridge } from "src/L2TokenBridge.sol"; +import { L1GovernanceRelay } from "src/L1GovernanceRelay.sol"; import { GemMock } from "test/mocks/GemMock.sol"; +import { L1TokenBridgeV2Mock } from "test/mocks/L1TokenBridgeV2Mock.sol"; +import { L2TokenBridgeV2Mock } from "test/mocks/L2TokenBridgeV2Mock.sol"; interface SuperChainConfigLike { function guardian() external returns (address); @@ -59,6 +62,7 @@ contract IntegrationTest is DssTest { address l2GovRelay; GemMock l2Token; L2TokenBridge l2Bridge; + address l2Spell; address L2_MESSENGER; constructor() { @@ -81,8 +85,8 @@ contract IntegrationTest is DssTest { vm.label(L1_MESSENGER, "L1_MESSENGER"); vm.label(L2_MESSENGER, "L2_MESSENGER"); - address l1GovRelay_ = vm.computeCreateAddress(address(this), vm.getNonce(address(this)) + 3); // foundry increments a global nonce across domains - address l1Bridge_ = vm.computeCreateAddress(address(this), vm.getNonce(address(this)) + 5); + address l1GovRelay_ = vm.computeCreateAddress(address(this), vm.getNonce(address(this)) + 4); // foundry increments a global nonce across domains + address l1Bridge_ = vm.computeCreateAddress(address(this), vm.getNonce(address(this)) + 7); l2Domain.selectFork(); L2TokenBridgeInstance memory l2BridgeInstance = TokenBridgeDeploy.deployL2({ deployer: address(this), @@ -92,7 +96,10 @@ contract IntegrationTest is DssTest { }); l2GovRelay = l2BridgeInstance.govRelay; l2Bridge = L2TokenBridge(l2BridgeInstance.bridge); - assertEq(address(L2TokenBridgeSpell(l2BridgeInstance.spell).l2Bridge()), address(l2Bridge)); + l2Spell = l2BridgeInstance.spell; + assertEq(address(L2TokenBridgeSpell(l2Spell).l2Bridge()), address(l2Bridge)); + assertEq(l2Bridge.version(), "1"); + assertEq(l2Bridge.getImplementation(), l2BridgeInstance.bridgeImp); l1Domain.selectFork(); L1TokenBridgeInstance memory l1BridgeInstance = TokenBridgeDeploy.deployL1({ @@ -107,6 +114,8 @@ contract IntegrationTest is DssTest { l1Bridge = L1TokenBridge(l1BridgeInstance.bridge); assertEq(l1GovRelay, l1GovRelay_); assertEq(address(l1Bridge), l1Bridge_); + assertEq(l1Bridge.version(), "1"); + assertEq(l1Bridge.getImplementation(), l1BridgeInstance.bridgeImp); l1Token = new GemMock(100 ether); vm.label(address(l1Token), "l1Token"); @@ -124,15 +133,16 @@ contract IntegrationTest is DssTest { uint256[] memory maxWithdraws = new uint256[](1); maxWithdraws[0] = 10_000_000 ether; BridgesConfig memory cfg = BridgesConfig({ - l1Messenger: L1_MESSENGER, - l2Messenger: L2_MESSENGER, - l1Tokens: l1Tokens, - l2Tokens: l2Tokens, - maxWithdraws: maxWithdraws, - minGasLimit: 1_000_000, - govRelayCLKey: "BASE_GOV_RELAY", - escrowCLKey: "BASE_ESCROW", - l1BridgeCLKey: "BASE_TOKEN_BRIDGE" + l1Messenger: L1_MESSENGER, + l2Messenger: L2_MESSENGER, + l1Tokens: l1Tokens, + l2Tokens: l2Tokens, + maxWithdraws: maxWithdraws, + minGasLimit: 1_000_000, + govRelayCLKey: "BASE_GOV_RELAY", + escrowCLKey: "BASE_ESCROW", + l1BridgeCLKey: "BASE_TOKEN_BRIDGE", + l1BridgeImpCLKey: "BASE_TOKEN_BRIDGE_IMP" }); l1Domain.selectFork(); @@ -146,6 +156,7 @@ contract IntegrationTest is DssTest { assertEq(dss.chainlog.getAddress("BASE_GOV_RELAY"), l1GovRelay); assertEq(dss.chainlog.getAddress("BASE_ESCROW"), escrow); assertEq(dss.chainlog.getAddress("BASE_TOKEN_BRIDGE"), l1Bridge_); + assertEq(dss.chainlog.getAddress("BASE_TOKEN_BRIDGE_IMP"), l1BridgeInstance.bridgeImp); l2Domain.relayFromHost(true); @@ -225,4 +236,36 @@ contract IntegrationTest is DssTest { vm.expectRevert("CrossDomainMessenger: paused"); l2Domain.relayToHost(true); } + + function testUpgrade() public { + l2Domain.selectFork(); + address newL2Imp = address(new L2TokenBridgeV2Mock()); + l1Domain.selectFork(); + address newL1Imp = address(new L1TokenBridgeV2Mock()); + + vm.startPrank(PAUSE_PROXY); + l1Bridge.upgradeToAndCall(newL1Imp, abi.encodeCall(L1TokenBridgeV2Mock.reinitialize, ())); + vm.stopPrank(); + + assertEq(l1Bridge.getImplementation(), newL1Imp); + assertEq(l1Bridge.version(), "2"); + assertEq(l1Bridge.wards(PAUSE_PROXY), 1); // still a ward + + vm.startPrank(PAUSE_PROXY); + L1GovernanceRelay(l1GovRelay).relay({ + target: l2Spell, + targetData: abi.encodeCall(L2TokenBridgeSpell.upgradeToAndCall, ( + newL2Imp, + abi.encodeCall(L2TokenBridgeV2Mock.reinitialize, ()) + )), + minGasLimit: 100_000 + }); + vm.stopPrank(); + + l2Domain.relayFromHost(true); + + assertEq(l2Bridge.getImplementation(), newL2Imp); + assertEq(l2Bridge.version(), "2"); + assertEq(l2Bridge.wards(l2GovRelay), 1); // still a ward + } } diff --git a/test/L1TokenBridge.t.sol b/test/L1TokenBridge.t.sol index 6100867..df130f7 100644 --- a/test/L1TokenBridge.t.sol +++ b/test/L1TokenBridge.t.sol @@ -18,10 +18,13 @@ pragma solidity ^0.8.21; import "dss-test/DssTest.sol"; - +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import { Upgrades, Options } from "openzeppelin-foundry-upgrades/Upgrades.sol"; import { L1TokenBridge } from "src/L1TokenBridge.sol"; import { GemMock } from "test/mocks/GemMock.sol"; import { MessengerMock } from "test/mocks/MessengerMock.sol"; +import { L1TokenBridgeV2Mock } from "test/mocks/L1TokenBridgeV2Mock.sol"; contract L1TokenBridgeTest is DssTest { @@ -50,6 +53,7 @@ contract L1TokenBridgeTest is DssTest { uint256 messageNonce, uint256 gasLimit ); + event UpgradedTo(string version); GemMock l1Token; address l2Token = address(0x222); @@ -57,11 +61,27 @@ contract L1TokenBridgeTest is DssTest { address escrow = address(0xeee); address otherBridge = address(0xccc); MessengerMock messenger; + bool validate; function setUp() public { + validate = vm.envOr("VALIDATE", false); + messenger = new MessengerMock(); messenger.setXDomainMessageSender(otherBridge); - bridge = new L1TokenBridge(otherBridge, address(messenger)); + + L1TokenBridge imp = new L1TokenBridge(otherBridge, address(messenger)); + assertEq(imp.otherBridge(), otherBridge); + assertEq(address(imp.messenger()), address(messenger)); + + vm.expectEmit(true, true, true, true); + emit Rely(address(this)); + bridge = L1TokenBridge(address(new ERC1967Proxy(address(imp), abi.encodeCall(L1TokenBridge.initialize, ())))); + assertEq(bridge.getImplementation(), address(imp)); + assertEq(bridge.wards(address(this)), 1); + assertEq(bridge.isOpen(), 1); + assertEq(bridge.otherBridge(), otherBridge); + assertEq(address(bridge.messenger()), address(messenger)); + bridge.file("escrow", escrow); l1Token = new GemMock(1_000_000 ether); l1Token.transfer(address(0xe0a), 500_000 ether); @@ -69,17 +89,6 @@ contract L1TokenBridgeTest is DssTest { bridge.registerToken(address(l1Token), l2Token); } - function testConstructor() public { - vm.expectEmit(true, true, true, true); - emit Rely(address(this)); - L1TokenBridge b = new L1TokenBridge(address(111), address(222)); - - assertEq(b.isOpen(), 1); - assertEq(b.otherBridge(), address(111)); - assertEq(address(b.messenger()), address(222)); - assertEq(b.wards(address(this)), 1); - } - function testAuth() public { checkAuth(address(bridge), "L1TokenBridge"); } @@ -89,7 +98,8 @@ contract L1TokenBridgeTest is DssTest { checkModifier(address(bridge), string(abi.encodePacked("L1TokenBridge", "/not-authorized")), [ bridge.close.selector, - bridge.registerToken.selector + bridge.registerToken.selector, + bridge.upgradeToAndCall.selector ]); } @@ -193,4 +203,72 @@ contract L1TokenBridgeTest is DssTest { assertEq(l1Token.balanceOf(escrow), 0); assertEq(l1Token.balanceOf(address(0xced)), 100 ether); } + + function testDeployWithUpgradesLib() public { + Options memory opts; + if (!validate) { + opts.unsafeSkipAllChecks = true; + } else { + opts.unsafeAllow = 'state-variable-immutable,constructor'; + } + opts.constructorData = abi.encode(otherBridge, address(messenger)); + + vm.expectEmit(true, true, true, true); + emit Rely(address(this)); + address proxy = Upgrades.deployUUPSProxy( + "out/L1TokenBridge.sol/L1TokenBridge.json", + abi.encodeCall(L1TokenBridge.initialize, ()), + opts + ); + assertEq(L1TokenBridge(proxy).version(), "1"); + assertEq(L1TokenBridge(proxy).wards(address(this)), 1); + } + + function testUpgrade() public { + address newImpl = address(new L1TokenBridgeV2Mock()); + vm.expectEmit(true, true, true, true); + emit UpgradedTo("2"); + bridge.upgradeToAndCall(newImpl, abi.encodeCall(L1TokenBridgeV2Mock.reinitialize, ())); + + assertEq(bridge.getImplementation(), newImpl); + assertEq(bridge.version(), "2"); + assertEq(bridge.wards(address(this)), 1); // still a ward + } + + function testUpgradeWithUpgradesLib() public { + address implementation1 = bridge.getImplementation(); + + Options memory opts; + if (!validate) { + opts.unsafeSkipAllChecks = true; + } else { + opts.referenceContract = "out/L1TokenBridge.sol/L1TokenBridge.json"; + opts.unsafeAllow = 'constructor'; + } + + vm.expectEmit(true, true, true, true); + emit UpgradedTo("2"); + Upgrades.upgradeProxy( + address(bridge), + "out/L1TokenBridgeV2Mock.sol/L1TokenBridgeV2Mock.json", + abi.encodeCall(L1TokenBridgeV2Mock.reinitialize, ()), + opts + ); + + address implementation2 = bridge.getImplementation(); + assertTrue(implementation1 != implementation2); + assertEq(bridge.version(), "2"); + assertEq(bridge.wards(address(this)), 1); // still a ward + } + + function testInitializeAgain() public { + vm.expectRevert(Initializable.InvalidInitialization.selector); + bridge.initialize(); + } + + function testInitializeDirectly() public { + address implementation = bridge.getImplementation(); + vm.expectRevert(Initializable.InvalidInitialization.selector); + L1TokenBridge(implementation).initialize(); + } } diff --git a/test/L2TokenBridge.t.sol b/test/L2TokenBridge.t.sol index 67b2b85..f71a57c 100644 --- a/test/L2TokenBridge.t.sol +++ b/test/L2TokenBridge.t.sol @@ -18,10 +18,13 @@ pragma solidity ^0.8.21; import "dss-test/DssTest.sol"; - +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import { Upgrades, Options } from "openzeppelin-foundry-upgrades/Upgrades.sol"; import { L2TokenBridge } from "src/L2TokenBridge.sol"; import { GemMock } from "test/mocks/GemMock.sol"; import { MessengerMock } from "test/mocks/MessengerMock.sol"; +import { L2TokenBridgeV2Mock } from "test/mocks/L2TokenBridgeV2Mock.sol"; contract L2TokenBridgeTest is DssTest { @@ -51,6 +54,7 @@ contract L2TokenBridgeTest is DssTest { uint256 messageNonce, uint256 gasLimit ); + event UpgradedTo(string version); GemMock l2Token; address l1Token = address(0x111); @@ -58,11 +62,27 @@ contract L2TokenBridgeTest is DssTest { address otherBridge = address(0xccc); address l2Router = address(0xbbb); MessengerMock messenger; + bool validate; function setUp() public { + validate = vm.envOr("VALIDATE", false); + messenger = new MessengerMock(); messenger.setXDomainMessageSender(otherBridge); - bridge = new L2TokenBridge(otherBridge, address(messenger)); + + L2TokenBridge imp = new L2TokenBridge(otherBridge, address(messenger)); + assertEq(imp.otherBridge(), otherBridge); + assertEq(address(imp.messenger()), address(messenger)); + + vm.expectEmit(true, true, true, true); + emit Rely(address(this)); + bridge = L2TokenBridge(address(new ERC1967Proxy(address(imp), abi.encodeCall(L2TokenBridge.initialize, ())))); + assertEq(bridge.getImplementation(), address(imp)); + assertEq(bridge.wards(address(this)), 1); + assertEq(bridge.isOpen(), 1); + assertEq(bridge.otherBridge(), otherBridge); + assertEq(address(bridge.messenger()), address(messenger)); + l2Token = new GemMock(1_000_000 ether); l2Token.transfer(address(0xe0a), 500_000 ether); l2Token.rely(address(bridge)); @@ -70,17 +90,6 @@ contract L2TokenBridgeTest is DssTest { bridge.setMaxWithdraw(address(l2Token), 1_000_000 ether); } - function testConstructor() public { - vm.expectEmit(true, true, true, true); - emit Rely(address(this)); - L2TokenBridge b = new L2TokenBridge(address(111), address(222)); - - assertEq(b.isOpen(), 1); - assertEq(b.otherBridge(), address(111)); - assertEq(address(b.messenger()), address(222)); - assertEq(b.wards(address(this)), 1); - } - function testAuth() public { checkAuth(address(bridge), "L2TokenBridge"); } @@ -91,7 +100,8 @@ contract L2TokenBridgeTest is DssTest { checkModifier(address(bridge), string(abi.encodePacked("L2TokenBridge", "/not-authorized")), [ bridge.close.selector, bridge.registerToken.selector, - bridge.setMaxWithdraw.selector + bridge.setMaxWithdraw.selector, + bridge.upgradeToAndCall.selector ]); } @@ -206,4 +216,72 @@ contract L2TokenBridgeTest is DssTest { assertEq(l2Token.balanceOf(address(0xced)), balanceBefore + 100 ether); assertEq(l2Token.totalSupply(), supplyBefore + 100 ether); } + + function testDeployWithUpgradesLib() public { + Options memory opts; + if (!validate) { + opts.unsafeSkipAllChecks = true; + } else { + opts.unsafeAllow = 'state-variable-immutable,constructor'; + } + opts.constructorData = abi.encode(otherBridge, address(messenger)); + + vm.expectEmit(true, true, true, true); + emit Rely(address(this)); + address proxy = Upgrades.deployUUPSProxy( + "out/L2TokenBridge.sol/L2TokenBridge.json", + abi.encodeCall(L2TokenBridge.initialize, ()), + opts + ); + assertEq(L2TokenBridge(proxy).version(), "1"); + assertEq(L2TokenBridge(proxy).wards(address(this)), 1); + } + + function testUpgrade() public { + address newImpl = address(new L2TokenBridgeV2Mock()); + vm.expectEmit(true, true, true, true); + emit UpgradedTo("2"); + bridge.upgradeToAndCall(newImpl, abi.encodeCall(L2TokenBridgeV2Mock.reinitialize, ())); + + assertEq(bridge.getImplementation(), newImpl); + assertEq(bridge.version(), "2"); + assertEq(bridge.wards(address(this)), 1); // still a ward + } + + function testUpgradeWithUpgradesLib() public { + address implementation1 = bridge.getImplementation(); + + Options memory opts; + if (!validate) { + opts.unsafeSkipAllChecks = true; + } else { + opts.referenceContract = "out/L2TokenBridge.sol/L2TokenBridge.json"; + opts.unsafeAllow = 'constructor'; + } + + vm.expectEmit(true, true, true, true); + emit UpgradedTo("2"); + Upgrades.upgradeProxy( + address(bridge), + "out/L2TokenBridgeV2Mock.sol/L2TokenBridgeV2Mock.json", + abi.encodeCall(L2TokenBridgeV2Mock.reinitialize, ()), + opts + ); + + address implementation2 = bridge.getImplementation(); + assertTrue(implementation1 != implementation2); + assertEq(bridge.version(), "2"); + assertEq(bridge.wards(address(this)), 1); // still a ward + } + + function testInitializeAgain() public { + vm.expectRevert(Initializable.InvalidInitialization.selector); + bridge.initialize(); + } + + function testInitializeDirectly() public { + address implementation = bridge.getImplementation(); + vm.expectRevert(Initializable.InvalidInitialization.selector); + L2TokenBridge(implementation).initialize(); + } } diff --git a/test/mocks/L1TokenBridgeV2Mock.sol b/test/mocks/L1TokenBridgeV2Mock.sol new file mode 100644 index 0000000..6eca2fe --- /dev/null +++ b/test/mocks/L1TokenBridgeV2Mock.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Copyright (C) 2024 Dai Foundation +// +// 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 . + +pragma solidity ^0.8.21; + +import { UUPSUpgradeable, ERC1967Utils } from "src/L1TokenBridge.sol"; + +contract L1TokenBridgeV2Mock is UUPSUpgradeable { + mapping(address => uint256) public wards; + mapping(address => address) public l1ToL2Token; + uint256 public isOpen; + address public escrow; + + string public constant version = "2"; + + event UpgradedTo(string version); + + modifier auth { + require(wards[msg.sender] == 1, "L1TokenBridge/not-authorized"); + _; + } + + constructor() { + _disableInitializers(); // Avoid initializing in the context of the implementation + } + + function reinitialize() reinitializer(2) external { + emit UpgradedTo(version); + } + + function _authorizeUpgrade(address newImplementation) internal override auth {} + + function getImplementation() external view returns (address) { + return ERC1967Utils.getImplementation(); + } +} diff --git a/test/mocks/L2TokenBridgeV2Mock.sol b/test/mocks/L2TokenBridgeV2Mock.sol new file mode 100644 index 0000000..9bd8e81 --- /dev/null +++ b/test/mocks/L2TokenBridgeV2Mock.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Copyright (C) 2024 Dai Foundation +// +// 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 . + +pragma solidity ^0.8.21; + +import { UUPSUpgradeable, ERC1967Utils } from "src/L2TokenBridge.sol"; + +contract L2TokenBridgeV2Mock is UUPSUpgradeable { + mapping(address => uint256) public wards; + mapping(address => address) public l1ToL2Token; + mapping(address => uint256) public maxWithdraws; + uint256 public isOpen; + + string public constant version = "2"; + + event UpgradedTo(string version); + + modifier auth { + require(wards[msg.sender] == 1, "L2TokenBridge/not-authorized"); + _; + } + + constructor() { + _disableInitializers(); // Avoid initializing in the context of the implementation + } + + function reinitialize() reinitializer(2) external { + emit UpgradedTo(version); + } + + function _authorizeUpgrade(address newImplementation) internal override auth {} + + function getImplementation() external view returns (address) { + return ERC1967Utils.getImplementation(); + } +}