diff --git a/src/tasks/MultisigTask.sol b/src/tasks/MultisigTask.sol index 8dda82414..b154c4a89 100644 --- a/src/tasks/MultisigTask.sol +++ b/src/tasks/MultisigTask.sol @@ -7,7 +7,6 @@ import {Script} from "forge-std/Script.sol"; import {VmSafe} from "forge-std/Vm.sol"; import {Test} from "forge-std/Test.sol"; import {StdStyle} from "forge-std/StdStyle.sol"; -import {stdToml} from "forge-std/StdToml.sol"; import {IMulticall3} from "forge-std/interfaces/IMulticall3.sol"; import {Signatures} from "@base-contracts/script/universal/Signatures.sol"; @@ -21,6 +20,7 @@ import {StateOverrideManager} from "src/tasks/StateOverrideManager.sol"; import {Utils} from "src/libraries/Utils.sol"; import {MultisigTaskPrinter} from "src/libraries/MultisigTaskPrinter.sol"; import {TaskManager} from "src/tasks/TaskManager.sol"; +import {SuperchainAddressRegistry} from "src/SuperchainAddressRegistry.sol"; import {Solarray} from "lib/optimism/packages/contracts-bedrock/scripts/libraries/Solarray.sol"; type AddressRegistry is address; @@ -30,7 +30,6 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage using AccountAccessParser for VmSafe.AccountAccess[]; using AccountAccessParser for VmSafe.AccountAccess; using StdStyle for string; - using stdToml for string; /// @notice Maximum gas limit for transactions, 15M gas is ~90% of the limit uint256 internal constant MAX_GAS_LIMIT = 15_000_000; @@ -50,12 +49,16 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage /// @notice struct to store the addresses that are expected to have balance changes EnumerableSet.AddressSet internal _allowedBalanceChanges; - /// @notice (account => slot => allowed) storage slots exempted from the - /// `isLikelyAddressThatShouldHaveCode` check. Used for packed slots where the - /// low 160 bits are not an independent address (e.g. SystemConfig slot 0x6c - /// packs `superchainConfig` with `minBaseFee`). Populated from the task's - /// `[storageCodeExceptions]` config section. - mapping(address => mapping(bytes32 => bool)) internal _storageCodeExceptions; + /// @notice Known fixed storage slots in upgradeable L1 contracts that pack an address with other fields. + bytes32 private constant PACKED_SLOT_ZERO = bytes32(uint256(0)); + bytes32 private constant OPTIMISM_PORTAL_SUPERCHAIN_CONFIG_SLOT = bytes32(uint256(53)); + bytes32 private constant OPTIMISM_PORTAL_ETH_LOCKBOX_SLOT = bytes32(uint256(63)); + bytes32 private constant SYSTEM_CONFIG_SUPERCHAIN_CONFIG_SLOT = bytes32(uint256(108)); + + /// @notice Byte offsets where the address starts within known packed storage slots. + uint256 private constant PACKED_ADDRESS_OFFSET_ZERO = 0; + uint256 private constant PACKED_ADDRESS_OFFSET_ONE = 1; + uint256 private constant PACKED_ADDRESS_OFFSET_TWO = 2; /// @notice Snapshot of the chain state before the tasks transaction is executed. uint256 private _preExecutionSnapshot; @@ -478,23 +481,27 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage if (!storageAccess.isWrite) continue; // Skip SLOADs. uint256 value = uint256(storageAccess.newValue); address account = storageAccess.account; - // Skip the code check for slots explicitly whitelisted via the task's - // `[storageCodeExceptions]` config (e.g. packed slots whose low 160 bits - // are not an independent address). - if (!_storageCodeExceptions[account][storageAccess.slot]) { - if (Utils.isLikelyAddressThatShouldHaveCode(value, codeExceptions)) { - // Log account, slot, and value if there is no code. - // forgefmt: disable-start - string memory err = string.concat("Likely address in storage has no code\n", " account: ", vm.toString(account), "\n slot: ", vm.toString(storageAccess.slot), "\n value: ", vm.toString(bytes32(value))); - // forgefmt: disable-end - require(address(uint160(value)).code.length != 0, err); - } else { - // Log account, slot, and value if there is code. - // forgefmt: disable-start - string memory err = string.concat("Likely address in storage has unexpected code\n", " account: ", vm.toString(account), "\n slot: ", vm.toString(storageAccess.slot), "\n value: ", vm.toString(bytes32(value))); - // forgefmt: disable-end - require(address(uint160(value)).code.length == 0, err); - } + + // Most storage writes are checked as full 32-byte values. Some Bedrock L1 contracts + // pack an address with small fields in the same slot, which makes the full value too + // large for the address heuristic. For known registry-identified slots, extract the + // address bytes and run the same code/EOA validation below. Adjacent bytes in these + // slots are known non-address fields like Initializable flags, spacers, or minBaseFee. + (bool isPackedAddrSlot, address packedAddr) = _getPackedStorageAddr(account, storageAccess.slot, value); + value = isPackedAddrSlot ? uint256(uint160(packedAddr)) : value; + + if (Utils.isLikelyAddressThatShouldHaveCode(value, codeExceptions)) { + // Log account, slot, and value if there is no code. + // forgefmt: disable-start + string memory err = string.concat("Likely address in storage has no code\n", " account: ", vm.toString(account), "\n slot: ", vm.toString(storageAccess.slot), "\n value: ", vm.toString(bytes32(value))); + // forgefmt: disable-end + require(address(uint160(value)).code.length != 0, err); + } else { + // Log account, slot, and value if there is code. + // forgefmt: disable-start + string memory err = string.concat("Likely address in storage has unexpected code\n", " account: ", vm.toString(account), "\n slot: ", vm.toString(storageAccess.slot), "\n value: ", vm.toString(bytes32(value))); + // forgefmt: disable-end + require(address(uint160(value)).code.length == 0, err); } require(account.code.length != 0, string.concat("Storage account has no code: ", vm.toString(account))); require(!storageAccess.reverted, string.concat("Storage access reverted: ", vm.toString(account))); @@ -507,6 +514,93 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage } } + /// @notice Returns the address stored in a known fixed packed storage slot. + /// @dev The slot numbers and byte offsets are hard-coded from the contracts-bedrock storage layout. + /// A slot number alone isn't enough to identify a packed-address slot, since many contracts write the + /// same slots (e.g. slot 0). We therefore also require `account` to match one of the contracts listed + /// below, looked up by name in the `SuperchainAddressRegistry`. Without this, any contract that wrote + /// one of these slots would have its lower bytes incorrectly checked as an address. + function _getPackedStorageAddr(address account, bytes32 slot, uint256 value) + internal + view + returns (bool isPackedAddrSlot_, address packedAddr_) + { + if (AddressRegistry.unwrap(addrRegistry) == address(0)) { + return (false, address(0)); + } + + if (slot == PACKED_SLOT_ZERO) { + // These contracts pack their first address field after Initializable flags in slot 0. + if (_isRegistryAddress(account, "AnchorStateRegistryProxy")) { + return (true, _extractPackedAddr(value, PACKED_ADDRESS_OFFSET_TWO)); + } + if (_isRegistryAddress(account, "EthLockboxProxy")) { + return (true, _extractPackedAddr(value, PACKED_ADDRESS_OFFSET_TWO)); + } + if (_isRegistryAddress(account, "SuperchainConfig")) { + return (true, _extractPackedAddr(value, PACKED_ADDRESS_OFFSET_TWO)); + } + } else if (slot == OPTIMISM_PORTAL_SUPERCHAIN_CONFIG_SLOT) { + // OptimismPortal2.superchainConfig shares slot 53 with a spacer byte. + if (_isRegistryAddress(account, "OptimismPortalProxy")) { + return (true, _extractPackedAddr(value, PACKED_ADDRESS_OFFSET_ONE)); + } + } else if (slot == OPTIMISM_PORTAL_ETH_LOCKBOX_SLOT) { + // OptimismPortal2.ethLockbox shares slot 63 with a spacer byte. + if (_isRegistryAddress(account, "OptimismPortalProxy")) { + return (true, _extractPackedAddr(value, PACKED_ADDRESS_OFFSET_ZERO)); + } + } else if (slot == SYSTEM_CONFIG_SUPERCHAIN_CONFIG_SLOT) { + // SystemConfig.superchainConfig shares slot 108 with minBaseFee. + if (_isRegistryAddress(account, "SystemConfigProxy")) { + return (true, _extractPackedAddr(value, PACKED_ADDRESS_OFFSET_ZERO)); + } + } + + return (false, address(0)); + } + + /// @notice Returns whether `account` is the registry address for `identifier`. + /// @dev Checks the sentinel chain first via `registry.get(identifier)` (the config `[addresses]` and + /// hardcoded entries, which take precedence), then each L2 chain via `getChains()` + `getAddress()`. + /// Returns false when `addrRegistry` is a `SimpleAddressRegistry` (it has no `getChains()`), since those + /// tasks never write the packed slots this guards. + function _isRegistryAddress(address account, string memory identifier) internal view returns (bool) { + SuperchainAddressRegistry registry = SuperchainAddressRegistry(AddressRegistry.unwrap(addrRegistry)); + // Config-defined and hardcoded addresses use the registry sentinel chain and take precedence + // over per-chain discovery in L2TaskBase. + try registry.get(identifier) returns (address expected) { + if (account == expected) return true; + } catch { + // The identifier may only exist as a per-chain address below. + } + + // Slot 0 is common, so do not reverse-lookup every slot-zero writer. Match only the + // configured L2-chain registry addresses that are known to pack address fields. + try registry.getChains() returns (SuperchainAddressRegistry.ChainInfo[] memory chains) { + for (uint256 i; i < chains.length; i++) { + try registry.getAddress(identifier, chains[i].chainId) returns (address expected) { + if (account == expected) return true; + } catch { + // Some identifiers are absent on older chains; absence means this account is not that identifier. + } + } + } catch { + // `addrRegistry` isn't always a `SuperchainAddressRegistry`, e.g. `SimpleTaskBase` tasks + // wrap a `SimpleAddressRegistry`, which has no `getChains()`, so the call reverts. + // Those tasks never write these packed slots, so we treat the account as not-a-match. + return false; + } + + return false; + } + + /// @notice Extracts a 20-byte address that begins at `offset` bytes from the low end of the slot. + function _extractPackedAddr(uint256 value, uint256 offset) internal pure returns (address) { + require(offset <= 12, "MultisigTask: packed address offset out of bounds"); + return address(uint160(value >> (offset * 8))); + } + /// @notice Helper function that returns whether or not the current context is a broadcast context. function _isBroadcastContext() internal view returns (bool) { return vm.isContext(VmSafe.ForgeContext.ScriptBroadcast) || vm.isContext(VmSafe.ForgeContext.ScriptResume); @@ -805,7 +899,6 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage returns (uint256[] memory allOriginalNonces_) { _setStateOverridesFromConfig(_taskConfigFilePath); // Sets global '_stateOverrides' variable. - _setStorageCodeExceptionsFromConfig(_taskConfigFilePath); // Sets '_storageCodeExceptions' mapping. allOriginalNonces_ = new uint256[](_allSafes.length); for (uint256 i = 0; i < _allSafes.length; i++) { allOriginalNonces_[i] = _getNonceOrOverride(_allSafes[i], _taskConfigFilePath); @@ -819,25 +912,6 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage _applyStateOverrides(); // Applies '_stateOverrides' to the current state. } - /// @notice Read storage code exceptions from a TOML config file. - /// Format (slots are exempted from the `isLikelyAddressThatShouldHaveCode` check): - /// [storageCodeExceptions] - /// "0xSystemConfigAddr" = ["0x...006c"] - function _setStorageCodeExceptionsFromConfig(string memory _taskConfigFilePath) private { - string memory toml = vm.readFile(_taskConfigFilePath); - string memory exceptionsKey = ".storageCodeExceptions"; - if (!toml.keyExists(exceptionsKey)) return; - - string[] memory accountStrings = vm.parseTomlKeys(toml, exceptionsKey); - for (uint256 i = 0; i < accountStrings.length; i++) { - address account = vm.parseAddress(accountStrings[i]); - bytes32[] memory slots = toml.readBytes32Array(string.concat(exceptionsKey, ".", accountStrings[i])); - for (uint256 j = 0; j < slots.length; j++) { - _storageCodeExceptions[account][slots[j]] = true; - } - } - } - /// ================================================== /// =============== Print Functions ================== /// ================================================== diff --git a/test/tasks/MultisigTask.t.sol b/test/tasks/MultisigTask.t.sol index c23bd7606..ee8d54992 100644 --- a/test/tasks/MultisigTask.t.sol +++ b/test/tasks/MultisigTask.t.sol @@ -9,7 +9,7 @@ import {IGnosisSafe, Enum} from "@base-contracts/script/universal/IGnosisSafe.so import {Vm} from "forge-std/Vm.sol"; import {Solarray} from "lib/optimism/packages/contracts-bedrock/scripts/libraries/Solarray.sol"; -import {MultisigTask} from "src/tasks/MultisigTask.sol"; +import {MultisigTask, AddressRegistry} from "src/tasks/MultisigTask.sol"; import {SuperchainAddressRegistry} from "src/SuperchainAddressRegistry.sol"; import {Action, TaskPayload} from "src/libraries/MultisigTypes.sol"; import {MockMultisigTask} from "test/tasks/mock/MockMultisigTask.sol"; @@ -19,6 +19,14 @@ import {HighGasMultisigTask} from "test/tasks/mock/HighGasMultisigTask.sol"; contract MultisigTaskUnitTest is Test { using stdStorage for StdStorage; + // Non-address bytes that the real Bedrock contracts pack next to the address in these slots, which + // `_packAddress` reproduces so extraction is tested against realistic junk. `0x0101` is OpenZeppelin + // `Initializable`'s `_initialized` (low byte) and `_initializing` (next byte); `0x01` is a one-byte + // spacer; `1 << 160` is the `_initialized` flag sitting just above an address held in the low 20 bytes. + uint256 internal constant INITIALIZABLE_FLAGS = 0x0101; + uint256 internal constant SPACER_BYTE = 0x01; + uint256 internal constant ADDRESS_FOLLOWED_BY_INITIALIZED_FLAG = uint256(1) << 160; + SuperchainAddressRegistry public addrRegistry; MultisigTask public task; string constant TESTING_DIRECTORY = "multisig-task-testing"; @@ -349,6 +357,152 @@ contract MultisigTaskUnitTest is Test { assertFalse(harness.wrapperIsValidAction(access, topLevelDepth, rootSafe)); } + function testPackedStorageAddressExtractionMatchesKnownSlots() public { + MockMultisigTask harness = _packedSlotHarness(); + + _assertPackedAddressExtraction(harness, "AnchorStateRegistryProxy", bytes32(uint256(0)), 2); + _assertPackedAddressExtraction(harness, "EthLockboxProxy", bytes32(uint256(0)), 2); + _assertPackedAddressExtraction(harness, "OptimismPortalProxy", bytes32(uint256(53)), 1); + _assertPackedAddressExtraction(harness, "OptimismPortalProxy", bytes32(uint256(63)), 0); + _assertPackedAddressExtraction(harness, "SuperchainConfig", bytes32(uint256(0)), 2); + _assertPackedAddressExtraction(harness, "SystemConfigProxy", bytes32(uint256(108)), 0); + } + + function testPackedStorageAddressExtractionIgnoresFilteredSlots() public { + MockMultisigTask harness = _packedSlotHarness(); + + _assertNotPackedAddressSlot(harness, "OptimismPortalProxy", bytes32(uint256(52))); + _assertNotPackedAddressSlot(harness, "L1CrossDomainMessengerProxy", bytes32(uint256(0))); + } + + function testPackedStorageAddressExtractionMatchesConfigAddressOverride() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerSentinelStorageAccount("SuperchainConfig"); + address expected = address(uint160(0x1111111111111111111111111111111111111110)); + + (bool isPackedAddressSlot, address packedAddress) = harness.wrapperGetPackedStorageAddress( + address(storageAccount), bytes32(uint256(0)), uint256(_packAddress(expected, 2)) + ); + + assertTrue(isPackedAddressSlot, "SuperchainConfig"); + assertEq(packedAddress, expected, "SuperchainConfig"); + } + + function testPackedStorageAddressExtractionRevertsForOutOfBoundsOffset() public { + MockMultisigTask harness = _packedSlotHarness(); + + vm.expectRevert("MultisigTask: packed address offset out of bounds"); + harness.wrapperExtractPackedAddress(0, 13); + } + + function testCheckStateDiffValidatesPackedAddressAtNonZeroOffset() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerStorageAccount("AnchorStateRegistryProxy"); + address guardian = address(new MockTarget()); + + harness.wrapperAddAllowedStorageAccess(address(storageAccount)); + harness.wrapperCheckStateDiff( + _singleStorageWrite(address(storageAccount), bytes32(uint256(0)), _packAddress(guardian, 2)) + ); + } + + function testCheckStateDiffValidatesPackedAddressAtOffsetZero() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerStorageAccount("SystemConfigProxy"); + address superchainConfig = address(new MockTarget()); + + harness.wrapperAddAllowedStorageAccess(address(storageAccount)); + harness.wrapperCheckStateDiff( + _singleStorageWrite(address(storageAccount), bytes32(uint256(108)), _packAddress(superchainConfig, 0)) + ); + } + + function testCheckStateDiffRevertsWhenPackedAddressHasNoCode() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerStorageAccount("AnchorStateRegistryProxy"); + address guardian = address(uint160(0x1111111111111111111111111111111111111111)); + + harness.wrapperAddAllowedStorageAccess(address(storageAccount)); + + bytes32 slot = bytes32(uint256(0)); + bytes32 packedValue = _packAddress(guardian, 2); + string memory err = string.concat( + "Likely address in storage has no code\n", + " account: ", + vm.toString(address(storageAccount)), + "\n slot: ", + vm.toString(slot), + "\n value: ", + vm.toString(bytes32(uint256(uint160(guardian)))) + ); + vm.expectRevert(bytes(err)); + harness.wrapperCheckStateDiff(_singleStorageWrite(address(storageAccount), slot, packedValue)); + } + + function testCheckStateDiffAllowsPackedAddressCodeException() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerStorageAccount("AnchorStateRegistryProxy"); + address guardian = address(uint160(0x1111111111111111111111111111111111111111)); + address[] memory codeExceptions = new address[](1); + codeExceptions[0] = guardian; + + harness.wrapperAddAllowedStorageAccess(address(storageAccount)); + harness.wrapperSetCodeExceptions(codeExceptions); + harness.wrapperCheckStateDiff( + _singleStorageWrite(address(storageAccount), bytes32(uint256(0)), _packAddress(guardian, 2)) + ); + } + + function testCheckStateDiffRevertsWhenPackedAddressCodeExceptionHasCode() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerStorageAccount("AnchorStateRegistryProxy"); + address guardian = address(new MockTarget()); + address[] memory codeExceptions = new address[](1); + codeExceptions[0] = guardian; + + harness.wrapperAddAllowedStorageAccess(address(storageAccount)); + harness.wrapperSetCodeExceptions(codeExceptions); + + bytes32 slot = bytes32(uint256(0)); + string memory err = string.concat( + "Likely address in storage has unexpected code\n", + " account: ", + vm.toString(address(storageAccount)), + "\n slot: ", + vm.toString(slot), + "\n value: ", + vm.toString(bytes32(uint256(uint160(guardian)))) + ); + vm.expectRevert(bytes(err)); + harness.wrapperCheckStateDiff(_singleStorageWrite(address(storageAccount), slot, _packAddress(guardian, 2))); + } + + function testCheckStateDiffDoesNotTreatUnregisteredNewContractAsKnownPackedSlot() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = new MockTarget(); + address systemConfig = address(uint160(0x1111111111111111111111111111111111111111)); + + harness.wrapperAddAllowedStorageAccess(address(0x1234)); + harness.wrapperCheckStateDiff( + _singleNewContractStorageWrite(address(storageAccount), bytes32(uint256(0)), _packAddress(systemConfig, 2)) + ); + } + + /// @notice A registered contract that isn't one of the known packed-slot contracts must use the + /// full-word check, even when it writes slot 0. `type(uint256).max` proves which path was taken: on the + /// full-word path it exceeds `type(uint160).max`, so it isn't address-shaped and passes; if the slot were + /// wrongly treated as packed, the extracted low 160 bits would be `uint160.max` — address-shaped with no + /// code — and the check would revert. No revert therefore means the packed path was not taken. + function testCheckStateDiffDoesNotTreatOtherRegisteredSlotZeroWritesAsKnownPackedSlot() public { + MockMultisigTask harness = _packedSlotHarness(); + MockTarget storageAccount = _registerStorageAccount("TargetContract"); + + harness.wrapperAddAllowedStorageAccess(address(storageAccount)); + harness.wrapperCheckStateDiff( + _singleStorageWrite(address(storageAccount), bytes32(uint256(0)), bytes32(type(uint256).max)) + ); + } + // Helper to create AccountAccess struct function createAccess(VmSafe.AccountAccessKind kind, address account, address accessor, uint64 depth) internal @@ -372,6 +526,108 @@ contract MultisigTaskUnitTest is Test { }); } + function _packedSlotHarness() internal returns (MockMultisigTask harness) { + harness = new MockMultisigTask(); + harness.wrapperSetAddressRegistry(AddressRegistry.wrap(address(addrRegistry))); + } + + function _assertPackedAddressExtraction( + MockMultisigTask harness, + string memory identifier, + bytes32 slot, + uint256 offset + ) internal { + MockTarget storageAccount = _registerStorageAccount(identifier); + address expected = address(uint160(0x1111111111111111111111111111111111111110)); + + (bool isPackedAddressSlot, address packedAddress) = harness.wrapperGetPackedStorageAddress( + address(storageAccount), slot, uint256(_packAddress(expected, offset)) + ); + + assertTrue(isPackedAddressSlot, identifier); + assertEq(packedAddress, expected, identifier); + } + + function _assertNotPackedAddressSlot(MockMultisigTask harness, string memory identifier, bytes32 slot) internal { + MockTarget storageAccount = _registerStorageAccount(identifier); + (bool isPackedAddressSlot, address packedAddress) = + harness.wrapperGetPackedStorageAddress(address(storageAccount), slot, uint256(_packAddress(address(1), 0))); + + assertFalse(isPackedAddressSlot, identifier); + assertEq(packedAddress, address(0), identifier); + } + + /// @notice Deploys a `MockTarget` and registers it under `identifier` on `chain`, so that + /// `_isRegistryAddress` resolves the mock when validating a packed storage slot. + function _registerStorageAccountOnChain(string memory identifier, SuperchainAddressRegistry.ChainInfo memory chain) + internal + returns (MockTarget storageAccount) + { + storageAccount = new MockTarget(); + string[] memory allowOverwrite = new string[](1); + allowOverwrite[0] = identifier; + addrRegistry.saveAddress(identifier, chain, address(storageAccount), allowOverwrite); + } + + /// @notice Registers the mock as a per-L2-chain address. This is the path `_isRegistryAddress` finds by + /// iterating `getChains()` and calling `getAddress(identifier, chainId)`. + function _registerStorageAccount(string memory identifier) internal returns (MockTarget storageAccount) { + return _registerStorageAccountOnChain(identifier, addrRegistry.getChains()[0]); + } + + /// @notice Registers the mock on the sentinel chain, i.e. the config `[addresses]` section. This is the + /// path `_isRegistryAddress` finds via `registry.get(identifier)`, which takes precedence over per-chain + /// discovery. + function _registerSentinelStorageAccount(string memory identifier) internal returns (MockTarget storageAccount) { + (uint256 chainId, string memory name) = addrRegistry.sentinelChain(); + return _registerStorageAccountOnChain( + identifier, SuperchainAddressRegistry.ChainInfo({chainId: chainId, name: name}) + ); + } + + /// @notice Builds an `AccountAccess[]` with a single storage write to `slot`, in the shape + /// `_checkStateDiff` consumes. + function _singleStorageWrite(address account, bytes32 slot, bytes32 newValue) + internal + pure + returns (VmSafe.AccountAccess[] memory accountAccesses) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = VmSafe.StorageAccess({ + account: account, + slot: slot, + isWrite: true, + previousValue: bytes32(0), + newValue: newValue, + reverted: false + }); + accountAccesses = new VmSafe.AccountAccess[](1); + accountAccesses[0] = createAccess(VmSafe.AccountAccessKind.Call, account, address(0x1234), 0); + accountAccesses[0].storageAccesses = storageAccesses; + } + + /// @notice Same as `_singleStorageWrite`, but marks the account as freshly deployed (a `Create` access + /// with code), so it exercises the new-contract allowance path in `_checkStateDiff`. + function _singleNewContractStorageWrite(address account, bytes32 slot, bytes32 newValue) + internal + pure + returns (VmSafe.AccountAccess[] memory accountAccesses) + { + accountAccesses = _singleStorageWrite(account, slot, newValue); + accountAccesses[0].kind = VmSafe.AccountAccessKind.Create; + accountAccesses[0].deployedCode = hex"01"; + } + + /// @notice Builds a packed slot value: `addr` placed `offset` bytes from the low end, with the remaining + /// bytes filled with realistic non-address junk (Initializable flags or a spacer byte). This mirrors how + /// the real Bedrock contracts pack these slots, so tests confirm extraction reads the address and ignores + /// the adjacent fields. + function _packAddress(address addr, uint256 offset) internal pure returns (bytes32) { + uint256 flags = + offset == 0 ? ADDRESS_FOLLOWED_BY_INITIALIZED_FLAG : offset == 1 ? SPACER_BYTE : INITIALIZABLE_FLAGS; + return bytes32((uint256(uint160(addr)) << (offset * 8)) | flags); + } + function createActions( address target, bytes memory data, diff --git a/test/tasks/NestedMultisigTask.t.sol b/test/tasks/NestedMultisigTask.t.sol index 0613bca44..50a8e72a6 100644 --- a/test/tasks/NestedMultisigTask.t.sol +++ b/test/tasks/NestedMultisigTask.t.sol @@ -266,91 +266,6 @@ contract NestedMultisigTaskTest is Test { multisigTask.simulate(configFilePath, Solarray.addresses(SECURITY_COUNCIL_CHILD_MULTISIG)); } - /// @notice OptimismPortalProxy address on OP Mainnet at block 23149345 (used by the - /// storage-code-exception tests below). - address constant OPTIMISM_PORTAL_PROXY = 0xbEb5Fc579115071764c7423A4f12eDde41f106Ed; - - /// @notice Base config for the SetEIP1967Implementation task used by the storage-code-exception - /// tests. It writes a no-code, address-shaped value to the EIP-1967 implementation slot, which - /// without a `[storageCodeExceptions]` entry trips the `isLikelyAddressThatShouldHaveCode` check - /// (see `testSimulateCodeExceptionsCheckReverts`). - string constant CODE_EXCEPTION_BASE_TOML = "l2chains = [{name = \"OP Mainnet\", chainId = 10}]\n" "\n" - "templateName = \"SetEIP1967Implementation\"\n contractIdentifier = \"OptimismPortalProxy\"\n newImplementation = \"0x0000000FFfFFfffFffFfFffFFFfffffFffFFffFf\"\n"; - - /// @notice The exact revert message produced by the code check for the task above at block 23149345. - function _codeExceptionRevertMessage() internal pure returns (string memory) { - return string.concat( - "Likely address in storage has no code\n", - " account: ", - vm.toString(OPTIMISM_PORTAL_PROXY), - "\n slot: ", - vm.toString(bytes32(Constants.PROXY_IMPLEMENTATION_ADDRESS)), - "\n value: ", - vm.toString(bytes32(0x0000000000000000000000000000000fffffffffffffffffffffffffffffffff)) - ); - } - - /// @notice A `[storageCodeExceptions]` entry for the (account, slot) being written exempts that - /// slot from the code check, so the otherwise-reverting task simulates cleanly. The slot is listed - /// alongside an unrelated slot to also exercise multi-slot array parsing. - function testStorageCodeExceptionWhitelistsSlot() public { - vm.createSelectFork("mainnet", 23149345); - multisigTask = new SetEIP1967Implementation(); - string memory toml = string.concat( - CODE_EXCEPTION_BASE_TOML, - "[storageCodeExceptions]\n", - vm.toString(OPTIMISM_PORTAL_PROXY), - " = [\"", - vm.toString(bytes32(Constants.PROXY_IMPLEMENTATION_ADDRESS)), - "\", \"0x0000000000000000000000000000000000000000000000000000000000000000\"]\n" - ); - string memory configFilePath = MultisigTaskTestHelper.createTempTomlFile(toml, TESTING_DIRECTORY, "007"); - - // Should NOT revert: the code check is skipped for the whitelisted slot. - (,,, address rootSafe) = - multisigTask.simulate(configFilePath, Solarray.addresses(SECURITY_COUNCIL_CHILD_MULTISIG)); - assertTrue(rootSafe != address(0), "Expected task to simulate without reverting"); - MultisigTaskTestHelper.removeFile(configFilePath); - } - - /// @notice A `[storageCodeExceptions]` entry for the correct account but a different slot does NOT - /// suppress the check on the slot actually being written: the exception is slot-specific. - function testStorageCodeExceptionWrongSlotStillReverts() public { - vm.createSelectFork("mainnet", 23149345); - multisigTask = new SetEIP1967Implementation(); - string memory toml = string.concat( - CODE_EXCEPTION_BASE_TOML, - "[storageCodeExceptions]\n", - vm.toString(OPTIMISM_PORTAL_PROXY), - " = [\"0x0000000000000000000000000000000000000000000000000000000000000001\"]\n" - ); - string memory configFilePath = MultisigTaskTestHelper.createTempTomlFile(toml, TESTING_DIRECTORY, "008"); - - vm.expectRevert(bytes(_codeExceptionRevertMessage())); - multisigTask.simulate(configFilePath, Solarray.addresses(SECURITY_COUNCIL_CHILD_MULTISIG)); - MultisigTaskTestHelper.removeFile(configFilePath); - } - - /// @notice A `[storageCodeExceptions]` entry for the correct slot but a different account does NOT - /// suppress the check: the exception is account-specific. - function testStorageCodeExceptionWrongAccountStillReverts() public { - vm.createSelectFork("mainnet", 23149345); - multisigTask = new SetEIP1967Implementation(); - string memory toml = string.concat( - CODE_EXCEPTION_BASE_TOML, - "[storageCodeExceptions]\n", - // Some unrelated account, with the correct slot. - "0x000000000000000000000000000000000000dEaD = [\"", - vm.toString(bytes32(Constants.PROXY_IMPLEMENTATION_ADDRESS)), - "\"]\n" - ); - string memory configFilePath = MultisigTaskTestHelper.createTempTomlFile(toml, TESTING_DIRECTORY, "009"); - - vm.expectRevert(bytes(_codeExceptionRevertMessage())); - multisigTask.simulate(configFilePath, Solarray.addresses(SECURITY_COUNCIL_CHILD_MULTISIG)); - MultisigTaskTestHelper.removeFile(configFilePath); - } - /// @notice Validate the data to sign for the child multisig. function _validateNestedDataToSign( address _childMultisig, diff --git a/test/tasks/mock/MockMultisigTask.sol b/test/tasks/mock/MockMultisigTask.sol index b29e0de03..22a7205d6 100644 --- a/test/tasks/mock/MockMultisigTask.sol +++ b/test/tasks/mock/MockMultisigTask.sol @@ -5,8 +5,10 @@ import {IProxyAdmin} from "@eth-optimism-bedrock/interfaces/universal/IProxyAdmi import {Constants} from "@eth-optimism-bedrock/src/libraries/Constants.sol"; import {IProxy} from "@eth-optimism-bedrock/interfaces/universal/IProxy.sol"; import {VmSafe} from "forge-std/Vm.sol"; +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {SuperchainAddressRegistry} from "src/SuperchainAddressRegistry.sol"; +import {AddressRegistry} from "src/tasks/MultisigTask.sol"; import {L2TaskBase} from "src/tasks/types/L2TaskBase.sol"; import {Action} from "src/libraries/MultisigTypes.sol"; import {MockTarget} from "test/tasks/mock/MockTarget.sol"; @@ -14,11 +16,15 @@ import {MockTarget} from "test/tasks/mock/MockTarget.sol"; /// Mock task that upgrades the L1ERC721BridgeProxy implementation /// to an example implementation address contract MockMultisigTask is L2TaskBase { + using EnumerableSet for EnumerableSet.AddressSet; + address public constant newImplementation = address(1000); /// @notice reference to the mock target contract MockTarget public mockTarget; + address[] private _mockCodeExceptions; + /// @notice Returns the safe address string identifier /// @return The string "SystemConfigOwner" function safeAddressString() public pure override returns (string memory) { @@ -70,7 +76,9 @@ contract MockMultisigTask is L2TaskBase { } /// @notice No code exceptions for this template. - function _getCodeExceptions() internal view virtual override returns (address[] memory) {} + function _getCodeExceptions() internal view virtual override returns (address[] memory) { + return _mockCodeExceptions; + } /// @notice Wrapper function to call the internal _isValidAction function. This is used to test the internal function. function wrapperIsValidAction(VmSafe.AccountAccess memory access, uint256 topLevelDepth, address rootSafe) @@ -80,4 +88,32 @@ contract MockMultisigTask is L2TaskBase { { return super._isValidAction(access, topLevelDepth, rootSafe); } + + function wrapperSetAddressRegistry(AddressRegistry _addrRegistry) public { + addrRegistry = _addrRegistry; + } + + function wrapperAddAllowedStorageAccess(address account) public { + _allowedStorageAccesses.add(account); + } + + function wrapperSetCodeExceptions(address[] memory codeExceptions) public { + _mockCodeExceptions = codeExceptions; + } + + function wrapperCheckStateDiff(VmSafe.AccountAccess[] memory accountAccesses) public view { + _checkStateDiff(accountAccesses); + } + + function wrapperGetPackedStorageAddress(address account, bytes32 slot, uint256 value) + public + view + returns (bool isPackedAddressSlot, address packedAddress) + { + return _getPackedStorageAddr(account, slot, value); + } + + function wrapperExtractPackedAddress(uint256 value, uint256 offset) public pure returns (address) { + return _extractPackedAddr(value, offset); + } }