diff --git a/packages/contracts-bedrock/interfaces/L2/IL2ToL2CrossDomainMessenger.sol b/packages/contracts-bedrock/interfaces/L2/IL2ToL2CrossDomainMessenger.sol index 58f07a9c92d..c6c2c293c7f 100644 --- a/packages/contracts-bedrock/interfaces/L2/IL2ToL2CrossDomainMessenger.sol +++ b/packages/contracts-bedrock/interfaces/L2/IL2ToL2CrossDomainMessenger.sol @@ -42,9 +42,6 @@ interface IL2ToL2CrossDomainMessenger { /// @notice Thrown when a call to the target contract during message relay fails. error TargetCallFailed(); - /// @notice Thrown when attempting to use a chain ID that is not in the dependency set. - error InvalidChainId(); - /// @notice Emitted whenever a message is sent to a destination /// @param destination Chain ID of the destination chain. /// @param target Target contract or wallet address. diff --git a/packages/contracts-bedrock/scripts/L2Genesis.s.sol b/packages/contracts-bedrock/scripts/L2Genesis.s.sol index 5a350a820c7..2357e763f4c 100644 --- a/packages/contracts-bedrock/scripts/L2Genesis.s.sol +++ b/packages/contracts-bedrock/scripts/L2Genesis.s.sol @@ -280,7 +280,6 @@ contract L2Genesis is Deployer { setOptimismSuperchainERC20Factory(); // 26 setOptimismSuperchainERC20Beacon(); // 27 setSuperchainTokenBridge(); // 28 - setDependencyManager(); // 29 } } diff --git a/packages/contracts-bedrock/snapshots/abi/L2ToL2CrossDomainMessenger.json b/packages/contracts-bedrock/snapshots/abi/L2ToL2CrossDomainMessenger.json index 3d381ec9ae5..e4cea3651b3 100644 --- a/packages/contracts-bedrock/snapshots/abi/L2ToL2CrossDomainMessenger.json +++ b/packages/contracts-bedrock/snapshots/abi/L2ToL2CrossDomainMessenger.json @@ -253,11 +253,6 @@ "name": "IdOriginNotL2ToL2CrossDomainMessenger", "type": "error" }, - { - "inputs": [], - "name": "InvalidChainId", - "type": "error" - }, { "inputs": [], "name": "MessageAlreadyRelayed", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 3baf488b20d..f15541a0556 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -40,8 +40,8 @@ "sourceCodeHash": "0xa98dfaa25f6594f702ca868a3bf54c6b00694d6a3d5601c04ac5db788d2b6e3c" }, "src/L1/SuperchainConfigInterop.sol": { - "initCodeHash": "0x061ace7ed28a2e9653085bb13802d58f8f80a047f88b2fe548721ff180930a09", - "sourceCodeHash": "0x28179045dd1c6c01846f3ec351102c47d7bf54e6ed87c929d67cdfce5426a130" + "initCodeHash": "0xc478f3a55eccdecf008bd3171eb0d80dca2f5b1ee2540594f264b62621a96cb1", + "sourceCodeHash": "0x3db38a6f7743e63cbeabc1ea598c70d29bfce67daef6871b486aea90691c7cd6" }, "src/L1/SystemConfig.sol": { "initCodeHash": "0x98c1049952199f55ae63e34ec61a839d43bde52b0892c482ae4246d0c088e826", @@ -100,8 +100,8 @@ "sourceCodeHash": "0xaef8ea36c5b78cd12e0e62811d51db627ccf0dfd2cc5479fb707a10ef0d42048" }, "src/L2/L2ToL2CrossDomainMessenger.sol": { - "initCodeHash": "0x86f1ff98c5ffe19a400c65a112b3e4f16242dcbcb635f636d63f25dbd3732d58", - "sourceCodeHash": "0x00237042c23a463b0aaa05fa95c819d246ca8fb8deb6a63510d85bd2a988fce1" + "initCodeHash": "0x115a00cfd2432d8b230302ec9a22f611ec23420aecd4b35d3d1d42041db820a9", + "sourceCodeHash": "0x067e879cd1acf2348e0729dd02395c064d24cf4571fdeade8bc2eefd8accec87" }, "src/L2/OptimismMintableERC721.sol": { "initCodeHash": "0xcfa6ad9997a422aef5a19a490a0a535bc870ee34b1f5258c2949eb3680f71e8a", diff --git a/packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol b/packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol index 1f023806c36..d770273e863 100644 --- a/packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol +++ b/packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol @@ -8,7 +8,6 @@ import { SuperchainConfig } from "src/L1/SuperchainConfig.sol"; import { Storage } from "src/libraries/Storage.sol"; import { EnumerableSet } from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import { Unauthorized } from "src/libraries/errors/CommonErrors.sol"; -import { Predeploys } from "src/libraries/Predeploys.sol"; // Interfaces import { ISystemConfig } from "interfaces/L1/ISystemConfig.sol"; @@ -120,16 +119,10 @@ contract SuperchainConfigInterop is SuperchainConfig { /// @param _systemConfig The SystemConfig contract address of the chain to add. function addDependency(uint256 _chainId, address _systemConfig) external { if (paused()) revert SuperchainPaused(); + if (msg.sender != clusterManager()) revert Unauthorized(); SuperchainConfigDependencies storage dependenciesStorage = _dependenciesStorage(); - if (msg.sender != clusterManager()) { - if (!dependenciesStorage.authorizedPortals[msg.sender]) revert Unauthorized(); - if (IOptimismPortalInterop(payable(msg.sender)).l2Sender() != Predeploys.DEPENDENCY_MANAGER) { - revert Unauthorized(); - } - } - if (dependenciesStorage.dependencySet.length() == type(uint8).max) revert DependencySetTooLarge(); // Add to the dependency set and check it is not already added (`add()` returns false if it already exists) diff --git a/packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol b/packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol index ae73b0de33d..4672e999cbb 100644 --- a/packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol +++ b/packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol @@ -9,7 +9,6 @@ import { TransientReentrancyAware } from "src/libraries/TransientContext.sol"; // Interfaces import { ISemver } from "interfaces/universal/ISemver.sol"; -import { IDependencySet } from "interfaces/L2/IDependencySet.sol"; import { ICrossL2Inbox, Identifier } from "interfaces/L2/ICrossL2Inbox.sol"; /// @notice Thrown when a non-written slot in transient storage is attempted to be read from. @@ -42,9 +41,6 @@ error ReentrantCall(); /// @notice Thrown when a call to the target contract during message relay fails. error TargetCallFailed(); -/// @notice Thrown when attempting to use a chain ID that is not in the dependency set. -error InvalidChainId(); - /// @custom:proxied true /// @custom:predeploy 0x4200000000000000000000000000000000000023 /// @title L2ToL2CrossDomainMessenger @@ -136,7 +132,6 @@ contract L2ToL2CrossDomainMessenger is ISemver, TransientReentrancyAware { if (_destination == block.chainid) revert MessageDestinationSameChain(); if (_target == Predeploys.CROSS_L2_INBOX) revert MessageTargetCrossL2Inbox(); if (_target == Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER) revert MessageTargetL2ToL2CrossDomainMessenger(); - if (!IDependencySet(Predeploys.DEPENDENCY_MANAGER).isInDependencySet(_destination)) revert InvalidChainId(); uint256 nonce = messageNonce(); emit SentMessage(_destination, _target, nonce, msg.sender, _message); diff --git a/packages/contracts-bedrock/src/libraries/Predeploys.sol b/packages/contracts-bedrock/src/libraries/Predeploys.sol index 8283eb8aca6..5385fbb33f8 100644 --- a/packages/contracts-bedrock/src/libraries/Predeploys.sol +++ b/packages/contracts-bedrock/src/libraries/Predeploys.sol @@ -163,7 +163,7 @@ library Predeploys { || (_useInterop && _addr == SUPERCHAIN_WETH) || (_useInterop && _addr == ETH_LIQUIDITY) || (_useInterop && _addr == OPTIMISM_SUPERCHAIN_ERC20_FACTORY) || (_useInterop && _addr == OPTIMISM_SUPERCHAIN_ERC20_BEACON) - || (_useInterop && _addr == SUPERCHAIN_TOKEN_BRIDGE) || (_useInterop && _addr == DEPENDENCY_MANAGER); + || (_useInterop && _addr == SUPERCHAIN_TOKEN_BRIDGE); } function isPredeployNamespace(address _addr) internal pure returns (bool) { diff --git a/packages/contracts-bedrock/test/L1/SuperchainConfigInterop.t.sol b/packages/contracts-bedrock/test/L1/SuperchainConfigInterop.t.sol index 9b9bebad968..6373780560a 100644 --- a/packages/contracts-bedrock/test/L1/SuperchainConfigInterop.t.sol +++ b/packages/contracts-bedrock/test/L1/SuperchainConfigInterop.t.sol @@ -173,71 +173,15 @@ contract SuperchainConfigInterop_AddDependency_Test is SuperchainConfigInterop_B assertTrue(_superchainConfigInterop().authorizedPortals(portal)); } - /// @notice Tests that `addDependency` successfully adds a chain to the dependency set through a portal call. - function test_addDependencyFromPortal_succeeds(uint256 _chainId, uint256 _chainId2) external { - vm.assume(!_superchainConfigInterop().isInDependencySet(_chainId)); - vm.assume(!_superchainConfigInterop().isInDependencySet(_chainId2)); - vm.assume(_chainId != _chainId2); - uint256 currentSize = _superchainConfigInterop().dependencySetSize(); - - // Add first an authorized portal - address authorizedPortal = _setUpPortal(_chainId); - vm.prank(_superchainConfigInterop().clusterManager()); - _superchainConfigInterop().addDependency(_chainId, address(systemConfig)); - - address portal2 = _setUpPortal(_chainId2); - - // Expect the DependencyAdded event to be emitted - vm.expectEmit(address(superchainConfig)); - emit DependencyAdded(_chainId2, address(systemConfig), portal2); - - // Mock the `authorizedPortal` to return the dependency manager predeploy as l2Sender - _mockAndExpect( - authorizedPortal, - abi.encodeCall(IOptimismPortalInterop.l2Sender, ()), - abi.encode(Predeploys.DEPENDENCY_MANAGER) - ); - - // Add the new chain to the dependency set from the `authorizedPortal` - vm.prank(authorizedPortal); - _superchainConfigInterop().addDependency(_chainId2, address(systemConfig)); - - // Check that the new chain is in the dependency set - assertTrue(_superchainConfigInterop().isInDependencySet(_chainId2)); - assertEq(_superchainConfigInterop().dependencySetSize(), currentSize + 2); - assertTrue(_superchainConfigInterop().authorizedPortals(portal2)); - } - /// @notice Tests that `addDependency` reverts when the caller is not the cluster manager or an authorized portal. - function test_addDependency_notClusterManagerOrPortal_reverts(address _caller, uint256 _chainId) external { + function test_addDependency_notClusterManager_reverts(address _caller, uint256 _chainId) external { vm.assume(_caller != _superchainConfigInterop().clusterManager()); - vm.assume(_caller != address(optimismPortal2)); vm.expectRevert(Unauthorized.selector); vm.prank(_caller); _superchainConfigInterop().addDependency(_chainId, address(systemConfig)); } - /// @notice Tests that `addDependency` reverts when the caller is an authorized portal but not the correct L2 - /// sender. - function test_addDependency_notCorrectL2Sender_reverts(uint256 _chainId, address _l2sender) external { - vm.assume(_chainId != block.chainid); - vm.assume(_l2sender != Predeploys.DEPENDENCY_MANAGER); - - address portal = _setUpPortal(_chainId); - - // Mock the `authorizedPortal` to return the dependency manager predeploy as l2Sender - _mockAndExpect(portal, abi.encodeCall(IOptimismPortalInterop.l2Sender, ()), abi.encode(_l2sender)); - - // Add first an authorized portal - vm.prank(_superchainConfigInterop().clusterManager()); - _superchainConfigInterop().addDependency(_chainId, address(systemConfig)); - - vm.expectRevert(Unauthorized.selector); - vm.prank(portal); - _superchainConfigInterop().addDependency(_chainId, address(systemConfig)); - } - /// @notice Tests that `addDependency` reverts when the portal has an invalid superchain config address. function test_addDependency_portalInvalidSuperchainConfig_reverts( uint256 _chainId, @@ -269,9 +213,13 @@ contract SuperchainConfigInterop_AddDependency_Test is SuperchainConfigInterop_B } /// @notice Tests that `addDependency` reverts when the portal is already authorized. - function test_addDependency_portalAlreadyAuthorized_reverts(uint256 _chainId) external { + function test_addDependency_portalAlreadyAuthorized_reverts(uint256 _chainId, uint256 _otherChainId) external { + // Bound chainId to be within uint128 range but not equal to block.chainid + _chainId = bound(_chainId, 1, type(uint128).max); + _otherChainId = bound(_otherChainId, 1, type(uint128).max); vm.assume(_chainId != block.chainid); - vm.assume(_chainId <= type(uint128).max); + vm.assume(_otherChainId != block.chainid); + vm.assume(_chainId != _otherChainId); _setUpPortal(_chainId); @@ -281,7 +229,7 @@ contract SuperchainConfigInterop_AddDependency_Test is SuperchainConfigInterop_B vm.prank(_superchainConfigInterop().clusterManager()); vm.expectRevert(SuperchainConfigInterop.PortalAlreadyAuthorized.selector); - _superchainConfigInterop().addDependency(_chainId + 1, address(systemConfig)); + _superchainConfigInterop().addDependency(_otherChainId, address(systemConfig)); } /// @notice Tests that `addDependency` reverts when the superchain is paused. diff --git a/packages/contracts-bedrock/test/L2/DependencyManager.t.sol b/packages/contracts-bedrock/test/L2/DependencyManager.t.sol index 0d4817db54a..ec756d0d049 100644 --- a/packages/contracts-bedrock/test/L2/DependencyManager.t.sol +++ b/packages/contracts-bedrock/test/L2/DependencyManager.t.sol @@ -13,6 +13,9 @@ contract DependencyManager_Base_Test is CommonTest { event DependencyAdded(uint256 indexed chainId, address indexed systemConfig, address indexed superchainConfig); function setUp() public virtual override { + // Skip the test until DependencyManager is integrated again + vm.skip(true); + super.enableInterop(); super.setUp(); } @@ -103,7 +106,7 @@ contract DependencyManager_AddDependency_Test is DependencyManager_Base_Test { contract DependencyManager_IsInDependencySet_Test is DependencyManager_Base_Test { /// @dev Tests that current chain is always in dependency set function testFuzz_isInDependencySet_currentChain_succeeds(uint256 _chainId) public { - vm.assume(_chainId <= type(uint8).max); + _chainId = bound(_chainId, 1, type(uint128).max); vm.chainId(_chainId); assertTrue(_dependencyManager().isInDependencySet(_chainId)); @@ -127,54 +130,79 @@ contract DependencyManager_IsInDependencySet_Test is DependencyManager_Base_Test } contract DependencyManager_DependencySet_Test is DependencyManager_Base_Test { + // Create a mapping to track used chainIds + mapping(uint256 => bool) usedChainIds; + /// @dev Tests that dependencySet returns correct values - function testFuzz_dependencySet_succeeds(uint256[] memory _chainIds) public { - vm.assume(_chainIds.length <= type(uint8).max); - - // Ensure chain IDs are unique and valid - for (uint256 i = 0; i < _chainIds.length; i++) { - _chainIds[i] = bound(_chainIds[i], 1, type(uint8).max); - vm.assume(_chainIds[i] != block.chainid); - for (uint256 j = 0; j < i; j++) { - vm.assume(_chainIds[i] != _chainIds[j]); + function testFuzz_dependencySet_succeeds(uint256[32] memory _chainIdsValues) public { + // Limit array size to prevent too many rejections + uint256[] memory chainIds = new uint256[](bound(_chainIdsValues.length, 1, 32)); + + // Loop over the values and add them to the array + for (uint256 i = 0; i < chainIds.length; i++) { + chainIds[i] = _chainIdsValues[i]; + } + + // Generate unique chain IDs more efficiently + for (uint256 i = 0; i < chainIds.length; i++) { + // Start with a bounded random value + uint256 chainId = bound(chainIds[i], 1, type(uint8).max); + + // If this chainId is already used or is the current chainId, + // increment until we find an unused one + while (usedChainIds[chainId] || chainId == block.chainid) { + chainId = (chainId % type(uint8).max) + 1; } + usedChainIds[chainId] = true; + chainIds[i] = chainId; } vm.startPrank(Constants.DEPOSITOR_ACCOUNT); // Add dependencies - for (uint256 i = 0; i < _chainIds.length; i++) { - _dependencyManager().addDependency(address(superchainConfig), _chainIds[i], address(systemConfig)); + for (uint256 i = 0; i < chainIds.length; i++) { + _dependencyManager().addDependency(address(superchainConfig), chainIds[i], address(systemConfig)); } uint256[] memory deps = _dependencyManager().dependencySet(); - assertEq(deps.length, _chainIds.length); + assertEq(deps.length, chainIds.length); // Verify each chain ID is in the dependency set - for (uint256 i = 0; i < _chainIds.length; i++) { - assertTrue(_dependencyManager().isInDependencySet(_chainIds[i])); + for (uint256 i = 0; i < chainIds.length; i++) { + assertTrue(_dependencyManager().isInDependencySet(chainIds[i])); } vm.stopPrank(); } /// @dev Tests that dependencySetSize returns correct value - function testFuzz_dependencySetSize_succeeds(uint256[] memory _chainIds) public { - vm.assume(_chainIds.length <= type(uint8).max); - - // Ensure chain IDs are unique and valid - for (uint256 i = 0; i < _chainIds.length; i++) { - _chainIds[i] = bound(_chainIds[i], 1, type(uint8).max); - vm.assume(_chainIds[i] != block.chainid); - for (uint256 j = 0; j < i; j++) { - vm.assume(_chainIds[i] != _chainIds[j]); + function testFuzz_dependencySetSize_succeeds(uint256[32] memory _chainIdsValues) public { + // Limit array size to prevent too many rejections + uint256[] memory chainIds = new uint256[](bound(_chainIdsValues.length, 1, 32)); + + // Loop over the values and add them to the array + for (uint256 i = 0; i < chainIds.length; i++) { + chainIds[i] = _chainIdsValues[i]; + } + + // Generate unique chain IDs more efficiently + for (uint256 i = 0; i < chainIds.length; i++) { + // Start with a bounded random value + uint256 chainId = bound(chainIds[i], 1, type(uint8).max); + + // If this chainId is already used or is the current chainId, + // increment until we find an unused one + while (usedChainIds[chainId] || chainId == block.chainid) { + chainId = (chainId % type(uint8).max) + 1; } + usedChainIds[chainId] = true; + chainIds[i] = chainId; } vm.startPrank(Constants.DEPOSITOR_ACCOUNT); - for (uint256 i = 0; i < _chainIds.length; i++) { - _dependencyManager().addDependency(address(superchainConfig), _chainIds[i], address(systemConfig)); + for (uint256 i = 0; i < chainIds.length; i++) { + _dependencyManager().addDependency(address(superchainConfig), chainIds[i], address(systemConfig)); assertEq(_dependencyManager().dependencySetSize(), i + 1); } diff --git a/packages/contracts-bedrock/test/L2/L2Genesis.t.sol b/packages/contracts-bedrock/test/L2/L2Genesis.t.sol index 3320be59cde..2398ac332df 100644 --- a/packages/contracts-bedrock/test/L2/L2Genesis.t.sol +++ b/packages/contracts-bedrock/test/L2/L2Genesis.t.sol @@ -140,7 +140,7 @@ contract L2GenesisTest is Test { assertEq(getCodeCount(_path, "Proxy.sol:Proxy"), Predeploys.PREDEPLOY_COUNT - 2); // 24 proxies have the implementation set if useInterop is true and 17 if useInterop is false - assertEq(getPredeployCountWithSlotSet(_path, Constants.PROXY_IMPLEMENTATION_ADDRESS), _useInterop ? 25 : 17); + assertEq(getPredeployCountWithSlotSet(_path, Constants.PROXY_IMPLEMENTATION_ADDRESS), _useInterop ? 24 : 17); // All proxies except 2 have the proxy 1967 admin slot set to the proxy admin assertEq( diff --git a/packages/contracts-bedrock/test/L2/L2ToL2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/test/L2/L2ToL2CrossDomainMessenger.t.sol index 510d5d529fa..55e43a4f782 100644 --- a/packages/contracts-bedrock/test/L2/L2ToL2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/test/L2/L2ToL2CrossDomainMessenger.t.sol @@ -26,7 +26,6 @@ import { // Interfaces import { ICrossL2Inbox, Identifier } from "interfaces/L2/ICrossL2Inbox.sol"; -import { IDependencySet } from "interfaces/L2/IDependencySet.sol"; /// @title L2ToL2CrossDomainMessengerWithModifiableTransientStorage /// @dev L2ToL2CrossDomainMessenger contract with methods to modify the transient storage. @@ -90,13 +89,6 @@ contract L2ToL2CrossDomainMessengerTest is Test { // Ensure that the target contract is not CrossL2Inbox or L2ToL2CrossDomainMessenger vm.assume(_target != Predeploys.CROSS_L2_INBOX && _target != Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER); - // Mock the call over the `isInDependencySet` function to return true - vm.mockCall( - Predeploys.DEPENDENCY_MANAGER, - abi.encodeCall(IDependencySet.isInDependencySet, (_destination)), - abi.encode(true) - ); - // Get the current message nonce uint256 messageNonce = l2ToL2CrossDomainMessenger.messageNonce();