diff --git a/packages/contracts-bedrock/snapshots/abi/SuperchainConfig.json b/packages/contracts-bedrock/snapshots/abi/SuperchainConfig.json index 4efd65d9bba..6d63bb81c86 100644 --- a/packages/contracts-bedrock/snapshots/abi/SuperchainConfig.json +++ b/packages/contracts-bedrock/snapshots/abi/SuperchainConfig.json @@ -276,6 +276,11 @@ "name": "ChainAlreadyAdded", "type": "error" }, + { + "inputs": [], + "name": "ChainAlreadyHasDependencies", + "type": "error" + }, { "inputs": [], "name": "Unauthorized", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index ec6549c6ec6..b3296d0a5d7 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -44,8 +44,8 @@ "sourceCodeHash": "0x26b7450f991fe197a94861cf618df04feb695dbbba884ffe93f96defa63e5bd7" }, "src/L1/SuperchainConfig.sol": { - "initCodeHash": "0x14e3ce78307d6709b29199de4380620003cbfe311a56b201ab2767c17e9e2091", - "sourceCodeHash": "0xff091439483e0669cf39c27d16ad0ef6c624d28829d079fe2c5c2c836a2db748" + "initCodeHash": "0x5cd6b119f03beea59e2ba29ea92694e6e5cbcbb1a83a02557ffbe332ce687ab6", + "sourceCodeHash": "0x1e191c7746b936427b8391c68a42b542eab0821c005a4a6a517bbe855278cced" }, "src/L1/SystemConfig.sol": { "initCodeHash": "0x0eda38e2fb2687a324289f04ec8ad0d2afe51f45219d074740fb4a0e24ea6569", diff --git a/packages/contracts-bedrock/src/L1/SuperchainConfig.sol b/packages/contracts-bedrock/src/L1/SuperchainConfig.sol index ad811120bb6..d990ea04112 100644 --- a/packages/contracts-bedrock/src/L1/SuperchainConfig.sol +++ b/packages/contracts-bedrock/src/L1/SuperchainConfig.sol @@ -50,6 +50,9 @@ contract SuperchainConfig is Initializable, ISemver { /// @param portal The address of the OptimismPortal contract. event ChainAdded(uint256 indexed chainId, address indexed systemConfig, address indexed portal); + /// @notice Thrown when the input chain's system config already contains dependencies on its set. + error ChainAlreadyHasDependencies(); + /// @notice Thrown when the input chain is already added to the dependency set. error ChainAlreadyAdded(); @@ -122,10 +125,11 @@ contract SuperchainConfig is Initializable, ISemver { /// Adds the new chain as a dependency for all existing chains in the dependency set, and vice versa. It /// also stores the SystemConfig address of it, and authorizes the OptimismPortal on the SharedLockbox. /// @param _chainId The chain ID. - /// @param _systemConfig The address of the SystemConfig contract. + /// @param _systemConfig The SystemConfig contract address of the chain to add. function addChain(uint256 _chainId, address _systemConfig) external { // TODO: Updater role TBD, using guardian for now. if (msg.sender != guardian()) revert Unauthorized(); + if (ISystemConfigInterop(_systemConfig).dependencyCounter() != 0) revert ChainAlreadyHasDependencies(); // Add to the dependency set and check it is not already added (`add()` returns false if it already exists) if (!_dependencySet.add(_chainId)) revert ChainAlreadyAdded(); diff --git a/packages/contracts-bedrock/src/L1/interfaces/ISuperchainConfig.sol b/packages/contracts-bedrock/src/L1/interfaces/ISuperchainConfig.sol index 24a65f9a470..efc0c598c5b 100644 --- a/packages/contracts-bedrock/src/L1/interfaces/ISuperchainConfig.sol +++ b/packages/contracts-bedrock/src/L1/interfaces/ISuperchainConfig.sol @@ -15,8 +15,9 @@ interface ISuperchainConfig is IDependencySet { event Unpaused(); event ChainAdded(uint256 indexed chainId, address indexed systemConfig, address indexed portal); - error ChainAlreadyAdded(); error Unauthorized(); + error ChainAlreadyHasDependencies(); + error ChainAlreadyAdded(); function GUARDIAN_SLOT() external view returns (bytes32); function PAUSED_SLOT() external view returns (bytes32); diff --git a/packages/contracts-bedrock/test/L1/LiquidityMigrator.t.sol b/packages/contracts-bedrock/test/L1/LiquidityMigrator.t.sol index a06aab1e11f..a793fab462b 100644 --- a/packages/contracts-bedrock/test/L1/LiquidityMigrator.t.sol +++ b/packages/contracts-bedrock/test/L1/LiquidityMigrator.t.sol @@ -10,6 +10,7 @@ contract LiquidityMigratorTest is CommonTest { LiquidityMigrator public migrator; function setUp() public virtual override { + super.enableInterop(); super.setUp(); migrator = new LiquidityMigrator(address(sharedLockbox)); } diff --git a/packages/contracts-bedrock/test/L1/SharedLockbox.t.sol b/packages/contracts-bedrock/test/L1/SharedLockbox.t.sol index 5a1bddcb97c..b1203e4e36f 100644 --- a/packages/contracts-bedrock/test/L1/SharedLockbox.t.sol +++ b/packages/contracts-bedrock/test/L1/SharedLockbox.t.sol @@ -18,6 +18,11 @@ contract SharedLockboxTest is CommonTest { event PortalAuthorized(address indexed portal); + function setUp() public virtual override { + super.enableInterop(); + super.setUp(); + } + /// @notice Tests it reverts when the caller is not an authorized portal. function test_lockETH_unauthorizedPortal_reverts(address _caller) public { vm.assume(!sharedLockbox.authorizedPortals(_caller)); diff --git a/packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol b/packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol index 6a39fd64207..4ac95f5f7ff 100644 --- a/packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol +++ b/packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol @@ -137,6 +137,11 @@ contract SuperchainConfig_Unpause_Test is CommonTest { contract SuperchainConfig_AddChain_Test is CommonTest { event ChainAdded(uint256 indexed chainId, address indexed systemConfig, address indexed portal); + function setUp() public virtual override { + super.enableInterop(); + super.setUp(); + } + function _mockAndExpect(address _target, bytes memory _calldata, bytes memory _returnData) internal { vm.mockCall(_target, _calldata, _returnData); vm.expectCall(_target, _calldata); @@ -151,11 +156,31 @@ contract SuperchainConfig_AddChain_Test is CommonTest { superchainConfig.addChain(_chainId, _systemConfig); } + /// @notice Tests that `addChain` reverts when the input chain already contains dependencies on its set. + function test_addChain_alreadyHasDependencies_reverts(uint256 _chainId, address _systemConfig) external { + // Mock the number of dependencies to be greater than 0. + uint256 numberOfDependencies = 1; + _mockAndExpect( + _systemConfig, + abi.encodeWithSelector(ISystemConfigInterop.dependencyCounter.selector), + abi.encode(numberOfDependencies) + ); + + vm.startPrank(superchainConfig.guardian()); + vm.expectRevert(ISuperchainConfig.ChainAlreadyHasDependencies.selector); + superchainConfig.addChain(_chainId, _systemConfig); + } + /// @notice Tests that `addChain` reverts when the chain is already in the dependency set. function test_addChain_chainAlreadyExists_reverts(uint256 _chainId, address _systemConfig) external { SuperchainConfigForTest superchainConfig = new SuperchainConfigForTest(address(sharedLockbox)); superchainConfig.forTest_addChainOnDependencySet(_chainId); + // Mock the call over `dependencyCounter` to return 0 and avoid a previous revert + _mockAndExpect( + _systemConfig, abi.encodeWithSelector(ISystemConfigInterop.dependencyCounter.selector), abi.encode(0) + ); + vm.startPrank(superchainConfig.guardian()); vm.expectRevert(SuperchainConfig.ChainAlreadyAdded.selector); superchainConfig.addChain(_chainId, _systemConfig); @@ -175,9 +200,7 @@ contract SuperchainConfig_AddChain_Test is CommonTest { vm.expectCall(address(systemConfig), abi.encodeWithSelector(ISystemConfigInterop.optimismPortal.selector)); // Mock and expect the call to authorize the portal on the SharedLockbox with the `_portal` address - _mockAndExpect( - address(sharedLockbox), abi.encodeWithSelector(ISharedLockbox.authorizePortal.selector, _portal), "" - ); + vm.expectCall(address(sharedLockbox), abi.encodeWithSelector(ISharedLockbox.authorizePortal.selector, _portal)); // Expect the `addDependency` function call to not be called since the dependency set is empty uint64 zeroCalls = 0; @@ -219,6 +242,13 @@ contract SuperchainConfig_AddChain_Test is CommonTest { superchainConfigForTest.forTest_addChainAndSystemConfig(chainIdTwo, systemConfigTwo); superchainConfigForTest.forTest_addChainAndSystemConfig(chainIdThree, systemConfigThree); + // Mock and expect the call to `dependencyCounter` to return 0 for the new chain + _mockAndExpect( + address(systemConfig), + abi.encodeWithSelector(ISystemConfigInterop.dependencyCounter.selector), + abi.encode(0) + ); + // Mock and expect the calls when looping through the first chain of the dependency set _mockAndExpect( systemConfigOne, abi.encodeWithSelector(ISystemConfigInterop.addDependency.selector, _chainId), ""