diff --git a/.circleci/config.yml b/.circleci/config.yml index ae66c92da..347259fb9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -34,6 +34,8 @@ commands: - utils/checkout-with-mise - run: name: "simulate << parameters.task >>" + environment: + FOUNDRY_PROFILE: ci command: | just install cd tasks/<< parameters.task >> @@ -51,6 +53,8 @@ commands: - utils/checkout-with-mise - run: name: "simulate nested << parameters.task >>" + environment: + FOUNDRY_PROFILE: ci command: | just install cd tasks/<< parameters.task >> @@ -199,6 +203,7 @@ jobs: # Use L1_RPC_URL and L2_RPC_URL here. just_simulate_sc_rehearsal_1: + circleci_ip_ranges: true docker: - image: <> steps: @@ -216,6 +221,7 @@ jobs: just simulate # simulate again to make sure the json is still valid just_simulate_sc_rehearsal_2: + circleci_ip_ranges: true docker: - image: <> steps: @@ -234,12 +240,15 @@ jobs: just simulate # simulate again to make sure the json is still valid just_simulate_sc_rehearsal_4: + circleci_ip_ranges: true docker: - image: <> steps: - utils/checkout-with-mise - run: name: just simulate r4-jointly-upgrade + environment: + FOUNDRY_PROFILE: ci command: | just install cd security-council-rehearsals @@ -251,6 +260,7 @@ jobs: just simulate-council # simulate again to make sure the json is still valid just_simulate_ink_respected_game_type: + circleci_ip_ranges: true docker: - image: <> steps: @@ -258,6 +268,7 @@ jobs: task: "/eth/ink-002-set-respected-game-type" just_simulate_zora_002_fp_upgrade: + circleci_ip_ranges: true docker: - image: <> steps: @@ -289,12 +300,15 @@ jobs: forge fmt --check forge_test: + circleci_ip_ranges: true docker: - image: <> steps: - utils/checkout-with-mise - run: name: forge test + environment: + FOUNDRY_PROFILE: ci command: | forge --version forge test -vvv @@ -303,20 +317,18 @@ jobs: circleci_ip_ranges: true docker: - image: <> + environment: + FOUNDRY_PROFILE: ci steps: - utils/checkout-with-mise - run: name: monorepo integration tests - environment: - ETH_RPC_URL: << pipeline.parameters.l1_mainnet_rpc_url >> command: | (cd src/improvements && just monorepo-integration-test) - notify-failures-on-develop: mentions: "@security-team" - run: name: Print failed test traces - environment: - ETH_RPC_URL: << pipeline.parameters.l1_mainnet_rpc_url >> command: (cd src/improvements && just monorepo-integration-test rerun) when: on_fail diff --git a/foundry.toml b/foundry.toml index 404e5d17e..169ca483b 100644 --- a/foundry.toml +++ b/foundry.toml @@ -25,7 +25,11 @@ remappings = [ [profile.ci] deny_warnings = true -[rpc_endpoints] +[profile.ci.rpc_endpoints] +mainnet = "https://ci-mainnet-l1-archive.optimism.io" # Must have 'circleci_ip_ranges = true' in .circleci/config.yml +sepolia = "https://ci-sepolia-l1-archive.optimism.io" # Must have 'circleci_ip_ranges = true' in .circleci/config.yml + +[profile.default.rpc_endpoints] localhost = "http://127.0.0.1:8545" mainnet = "https://ethereum.publicnode.com" sepolia = "https://ethereum-sepolia-rpc.publicnode.com" diff --git a/src/improvements/justfile b/src/improvements/justfile index c8acbc1f8..68f376544 100644 --- a/src/improvements/justfile +++ b/src/improvements/justfile @@ -67,10 +67,21 @@ task COMMAND="" NETWORK="": monorepo-integration-test COMMAND="": #!/usr/bin/env bash set -euo pipefail + + # Set FOUNDRY_PROFILE, defaulting to 'default' if not already set. + export FOUNDRY_PROFILE="${FOUNDRY_PROFILE:-default}" + echo "Currently running with FOUNDRY_PROFILE: ${FOUNDRY_PROFILE}" + root_dir=$(git rev-parse --show-toplevel) - allocs_path=${root_dir}/lib/optimism/packages/contracts-bedrock/allocs.json + allocs_path="${root_dir}/lib/optimism/packages/contracts-bedrock/allocs.json" + + # Running this command with mainnet RPC URL. + ETH_RPC_URL=$(yq eval ".profile.\"${FOUNDRY_PROFILE}\".rpc_endpoints.mainnet" "${root_dir}/foundry.toml") + export ETH_RPC_URL + echo "Using mainnet RPC: ${ETH_RPC_URL}" + forge build - forge script ${root_dir}/src/improvements/tasks/Runner.sol:Runner --sig "run(string)" ${allocs_path} --rpc-url $ETH_RPC_URL --ffi + forge script ${root_dir}/src/improvements/tasks/Runner.sol:Runner --sig "run(string)" ${allocs_path} --ffi --rpc-url $ETH_RPC_URL export SUPERCHAIN_OPS_ALLOCS_PATH=./allocs.json cd ${root_dir}/lib/optimism/packages/contracts-bedrock/ # shellcheck disable=SC2194 diff --git a/src/improvements/script/create-template.sh b/src/improvements/script/create-template.sh index e9d7f525b..8db972d5f 100755 --- a/src/improvements/script/create-template.sh +++ b/src/improvements/script/create-template.sh @@ -17,7 +17,6 @@ create_template() { // SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; -import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; /// @title ${contract_name} /// @notice A template contract for configuring protocol parameters. @@ -26,7 +25,7 @@ contract ${contract_name} is MultisigTask { /// @notice TODO: Define the struct fields for your task configuration. struct TaskConfig { // TODO: Add members this template needs - // (e.g., chainId, gas, implementation, gameType, etc.) + // (e.g., chainId, gas, implementation, gameType, etc.) } /// @notice TODO: Update the mapping key/value types as needed. @@ -35,15 +34,15 @@ contract ${contract_name} is MultisigTask { /// @notice Returns the safe address string identifier. /// @return A string identifier. function safeAddressString() public pure override returns (string memory) { - // TODO: Return the actual safe address string identifier as defined in - /// Superchain-Registry's addresses.json. + require(false, "TODO: Return the actual safe address string identifier as defined in Superchain-Registry's addresses.json."); + // Superchain-Registry's addresses.json. // return "ProxyAdminOwner"; } /// @notice Specifies the storage write permissions required for this task. /// @return An array of strings representing the storage permissions. function _taskStorageWrites() internal pure override returns (string[] memory) { - // TODO: Populate this array with actual storage permission identifiers. + require(false, "TODO: Populate this array with actual storage permission identifiers."); // string[] memory storageWrites = new string[](1); // return storageWrites; } @@ -51,19 +50,19 @@ contract ${contract_name} is MultisigTask { /// @notice Sets up the template using configuration data from a file. /// @param taskConfigFilePath The path to the configuration file. function _templateSetup(string memory taskConfigFilePath) internal override { - // TODO: Parse the configuration file and populate the \`taskConfig\` mapping. + require(false, "TODO: Implement the logic to parse the configuration file and populate the \`taskConfig\` mapping."); } /// @notice Builds the action(s) for applying the configuration for a given chain ID. /// @param chainId The chain ID for which to build the configuration actions. function _build(uint256 chainId) internal override { - // TODO: Implement the logic to build the configuration action(s). + require(false, "TODO: Implement the logic to build the configuration action(s)."); } /// @notice Validates that the configuration has been applied correctly. /// @param chainId The chain ID to validate. function _validate(uint256 chainId) internal view override { - // TODO: Implement the logic to validate that the configuration was set as expected. + require(false, "TODO: Implement the logic to validate that the configuration was set as expected."); } } EOL diff --git a/src/improvements/single.just b/src/improvements/single.just index d3a83d537..b0db8b58f 100644 --- a/src/improvements/single.just +++ b/src/improvements/single.just @@ -26,7 +26,7 @@ simulate hdPath='0': forge build forge script ${script} \ --rpc-url ${rpcUrl} \ - --sig "simulateRun(string)" ${config} \ + --sig "simulateRun(string)" ${config} sign hdPath='0': #!/usr/bin/env bash diff --git a/src/improvements/tasks/MultisigTask.sol b/src/improvements/tasks/MultisigTask.sol index 614a673d4..21258c6e8 100644 --- a/src/improvements/tasks/MultisigTask.sol +++ b/src/improvements/tasks/MultisigTask.sol @@ -74,7 +74,7 @@ abstract contract MultisigTask is Test, Script { mapping(address => TransferInfo[]) private _taskTransfers; /// @notice state changes during task execution - mapping(address => StateInfo[]) private _stateInfos; + mapping(address => StateInfo[]) internal _stateInfos; /// @notice addresses involved in state changes or token transfers EnumerableSet.AddressSet private _taskTransferFromAddresses; @@ -152,15 +152,30 @@ abstract contract MultisigTask is Test, Script { /// prints the data to sign and the hash to approve which is used to sign with the eip712sign binary. /// For nested multisig, prints the data to sign and the hash to approve for each of the child multisigs. /// @param taskConfigFilePath The path to the task configuration file. - function simulateRun(string memory taskConfigFilePath) public { + function simulateRun(string memory taskConfigFilePath, bytes memory signatures) + public + returns (VmSafe.AccountAccess[] memory) + { /// sets safe to the safe specified by the current template from addresses.json _taskSetup(taskConfigFilePath); /// now execute task actions build(); - simulate(); - validate(); + VmSafe.AccountAccess[] memory accountAccesses = simulate(signatures); + validate(accountAccesses); print(); + + return accountAccesses; + } + + /// @notice Runs the task with the given configuration file path. + /// Sets the address registry, initializes and simulates the single multisig + /// as well as the nested multisig. For single multisig, + /// prints the data to sign and the hash to approve which is used to sign with the eip712sign binary. + /// For nested multisig, prints the data to sign and the hash to approve for each of the child multisigs. + /// @param taskConfigFilePath The path to the task configuration file. + function simulateRun(string memory taskConfigFilePath) public returns (VmSafe.AccountAccess[] memory) { + return simulateRun(taskConfigFilePath, ""); } /// @notice Executes the task with the given configuration file path and signatures. @@ -168,12 +183,15 @@ abstract contract MultisigTask is Test, Script { /// as well as the nested multisig. /// @param taskConfigFilePath The path to the task configuration file. /// @param signatures The signatures to execute the task. - function executeRun(string memory taskConfigFilePath, bytes memory signatures) public { - _taskSetup(taskConfigFilePath); - /// now execute task actions - build(); + function executeRun(string memory taskConfigFilePath, bytes memory signatures) + public + returns (VmSafe.AccountAccess[] memory) + { + /// perform all actions in simulateRun, then send the transaction on chain + VmSafe.AccountAccess[] memory accountAccesses = simulateRun(taskConfigFilePath, signatures); execute(signatures); - validate(); + + return accountAccesses; } /// @notice Child multisig of a nested multisig approves the task to be executed with the given @@ -221,6 +239,7 @@ abstract contract MultisigTask is Test, Script { config.safeAddressString = safeAddressString(); config.allowedStorageWriteAccesses = _taskStorageWrites(); + config.allowedStorageWriteAccesses.push(safeAddressString()); // set the AddressRegistry addrRegistry = _addrRegistry; @@ -327,31 +346,42 @@ abstract contract MultisigTask is Test, Script { } /// @notice simulate the task by approving from owners and then executing - function simulate() public { + function simulate(bytes memory _signatures) public returns (VmSafe.AccountAccess[] memory) { bytes memory data = getCalldata(); bytes32 hash = getHash(); + bytes memory signatures; // Approve the hash from each owner address[] memory owners = IGnosisSafe(parentMultisig).getOwners(); - for (uint256 i = 0; i < owners.length; i++) { - vm.prank(owners[i]); - IGnosisSafe(parentMultisig).approveHash(hash); + if (_signatures.length == 0) { + for (uint256 i = 0; i < owners.length; i++) { + vm.prank(owners[i]); + IGnosisSafe(parentMultisig).approveHash(hash); + } + /// gather signatures after approval hashes have been made + signatures = prepareSignatures(parentMultisig, hash); + } else { + signatures = Signatures.prepareSignatures(parentMultisig, hash, _signatures); } - bytes memory signatures = prepareSignatures(parentMultisig, hash); - bytes32 txHash = IGnosisSafe(parentMultisig).getTransactionHash( multicallTarget, 0, data, Enum.Operation.DelegateCall, 0, 0, 0, address(0), payable(address(0)), nonce ); require(hash == txHash, "MultisigTask: hash mismatch"); + vm.startStateDiffRecording(); + // Execute the transaction (bool success) = IGnosisSafe(parentMultisig).execTransaction( multicallTarget, 0, data, Enum.Operation.DelegateCall, 0, 0, 0, address(0), payable(address(0)), signatures ); + VmSafe.AccountAccess[] memory accountAccesses = vm.stopAndReturnStateDiff(); + require(success, "MultisigTask: simulateActions failed"); + + return accountAccesses; } /// @notice child multisig approves the task to be executed. @@ -390,7 +420,6 @@ abstract contract MultisigTask is Test, Script { (bool success) = IGnosisSafe(parentMultisig).execTransaction( multicallTarget, 0, data, Enum.Operation.DelegateCall, 0, 0, 0, address(0), payable(address(0)), signatures ); - require(success, "MultisigTask: execute failed"); } @@ -404,8 +433,11 @@ abstract contract MultisigTask is Test, Script { /// e.g. read state variables of the deployed contracts to make /// sure they are deployed and initialized correctly, or read /// states that are expected to have changed during the simulate step. - function validate() public view virtual { - // check that all state change addresses are in allowed storage accesses + function validate(VmSafe.AccountAccess[] memory accountAccesses) public virtual { + /// write all state changes to storage + _processStateDiffChanges(accountAccesses); + + /// check that all state change addresses are in allowed storage accesses for (uint256 i; i < _taskStateChangeAddresses.length(); i++) { address addr = _taskStateChangeAddresses.at(i); require( @@ -438,6 +470,9 @@ abstract contract MultisigTask is Test, Script { for (uint256 i = 0; i < chains.length; i++) { _validate(chains[i].chainId); } + + /// check that state diff is as expected + checkStateDiff(accountAccesses); } /// @notice get task actions @@ -723,6 +758,9 @@ abstract contract MultisigTask is Test, Script { } } + /// @notice overridden in templates + function getCodeExceptions() internal view virtual returns (address[] memory); + /// @notice helper function to prepare the signatures to be executed /// @param _safe The address of the parent multisig /// @param hash The hash to be approved @@ -813,8 +851,6 @@ abstract contract MultisigTask is Test, Script { vm.revertTo(_startSnapshot), "MultisigTask: failed to revert back to snapshot, unsafe state to run task" ); - _processStateDiffChanges(accountAccesses); - // there should be at least one account access require(accountAccesses.length > 0, "MultisigTask: no account accesses found"); @@ -891,7 +927,68 @@ abstract contract MultisigTask is Test, Script { /// check the state changes applied by the task. This function can check /// that only the nonce changed in the parent multisig when executing a task /// by checking the slot and address where the slot changed. - function checkStateDiff(VmSafe.AccountAccess[] memory) internal view virtual {} + function checkStateDiff(VmSafe.AccountAccess[] memory accountAccesses) internal view virtual { + console.log("Running assertions on the state diff"); + require(accountAccesses.length > 0, "No account accesses"); + address[] memory allowedAccesses = getAllowedStorageAccess(); + for (uint256 i; i < accountAccesses.length; i++) { + VmSafe.AccountAccess memory accountAccess = accountAccesses[i]; + // All touched accounts should have code, with the exception of precompiles. + bool isPrecompile = accountAccess.account >= address(0x1) && accountAccess.account <= address(0xa); + if (!isPrecompile) { + require( + accountAccess.account.code.length != 0, + string.concat("Account has no code: ", vm.toString(accountAccess.account)) + ); + } + require( + accountAccess.oldBalance == accountAccess.newBalance, + string.concat("Unexpected balance change: ", vm.toString(accountAccess.account)) + ); + require( + accountAccess.kind != VmSafe.AccountAccessKind.SelfDestruct, + string.concat("Self-destructed account: ", vm.toString(accountAccess.account)) + ); + for (uint256 j; j < accountAccess.storageAccesses.length; j++) { + VmSafe.StorageAccess memory storageAccess = accountAccess.storageAccesses[j]; + if (!storageAccess.isWrite) continue; // Skip SLOADs. + uint256 value = uint256(storageAccess.newValue); + address account = storageAccess.account; + if (isLikelyAddressThatShouldHaveCode(value)) { + // Log account, slot, and value if there is no code. + 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)) + ); + require(address(uint160(value)).code.length != 0, err); + } else { + // Log account, slot, and value if there is code. + 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)) + ); + 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))); + bool allowed; + for (uint256 k; k < allowedAccesses.length; k++) { + allowed = allowed || (account == allowedAccesses[k]); + } + require(allowed, string.concat("Unallowed Storage access: ", vm.toString(account))); + } + } + } /// @notice helper method to get transfers and state changes of task affected addresses function _processStateDiffChanges(VmSafe.AccountAccess[] memory accountAccesses) private { @@ -907,8 +1004,6 @@ abstract contract MultisigTask is Test, Script { // process state changes _processStateChanges(accountAccesses[i].storageAccesses); } - - checkStateDiff(accountAccesses); } /// @notice helper method to get eth transfers of task affected addresses @@ -987,4 +1082,29 @@ abstract contract MultisigTask is Test, Script { } } } + + /// @notice Checks that values have code on this chain. + /// This method is not storage-layout-aware and therefore is not perfect. It may return erroneous + /// results for cases like packed slots, and silently show that things are okay when they are not. + function isLikelyAddressThatShouldHaveCode(uint256 value) internal view virtual returns (bool) { + // If out of range (fairly arbitrary lower bound), return false. + if (value > type(uint160).max) return false; + if (value < uint256(uint160(0x00000000fFFFffffffFfFfFFffFfFffFFFfFffff))) return false; + // If the value is a L2 predeploy address it won't have code on this chain, so return false. + if ( + value >= uint256(uint160(0x4200000000000000000000000000000000000000)) + && value <= uint256(uint160(0x420000000000000000000000000000000000FffF)) + ) return false; + // Allow known EOAs. + address[] memory exceptions = getCodeExceptions(); + for (uint256 i; i < exceptions.length; i++) { + require( + exceptions[i] != address(0), + "getCodeExceptions includes the zero address, please make sure all entries are populated." + ); + if (address(uint160(value)) == exceptions[i]) return false; + } + // Otherwise, this value looks like an address that we'd expect to have code. + return true; + } } diff --git a/src/improvements/tasks/OPCMBaseTask.sol b/src/improvements/tasks/OPCMBaseTask.sol index 7786e88d7..69799175a 100644 --- a/src/improvements/tasks/OPCMBaseTask.sol +++ b/src/improvements/tasks/OPCMBaseTask.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; +import {VmSafe} from "forge-std/Vm.sol"; + import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; /// @notice base task for making calls to the Optimism Contracts Manager @@ -57,10 +59,15 @@ abstract contract OPCMBaseTask is MultisigTask { data = abi.encodeWithSignature("aggregate3((address,bool,bytes)[])", calls); } - function validate() public view override { + function validate(VmSafe.AccountAccess[] memory accesses) public override { (address[] memory targets,,) = getTaskActions(); require(targets.length == 1 && targets[0] == opcm(), "OPCMBaseTask: only OPCM is allowed as target"); - super.validate(); + super.validate(accesses); + require( + _stateInfos[parentMultisig].length == 1, + "OPCMBaseTask: only nonce should be updated on upgrade controller multisig" + ); + require(_stateInfos[opcm()].length == 0, "OPCMBaseTask: Storage writes are not allowed on OPCM"); } /// @notice get the OPCM address diff --git a/src/improvements/tasks/Runner.sol b/src/improvements/tasks/Runner.sol index b21df1cf9..6aca862b4 100644 --- a/src/improvements/tasks/Runner.sol +++ b/src/improvements/tasks/Runner.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; diff --git a/src/improvements/template/DisputeGameUpgradeTemplate.sol b/src/improvements/template/DisputeGameUpgradeTemplate.sol index f534624b1..e8dc5703d 100644 --- a/src/improvements/template/DisputeGameUpgradeTemplate.sol +++ b/src/improvements/template/DisputeGameUpgradeTemplate.sol @@ -1,10 +1,11 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {IDisputeGameFactory, IDisputeGame} from "@eth-optimism-bedrock/interfaces/dispute/IDisputeGameFactory.sol"; + import "@eth-optimism-bedrock/src/dispute/lib/Types.sol"; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; -import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; /// @title DisputeGameUpgradeTemplate /// @notice Template contract for upgrading dispute game implementations @@ -75,4 +76,7 @@ contract DisputeGameUpgradeTemplate is MultisigTask { ); } } + + /// @notice no code exceptions for this template + function getCodeExceptions() internal view virtual override returns (address[] memory) {} } diff --git a/src/improvements/template/GasConfigTemplate.sol b/src/improvements/template/GasConfigTemplate.sol index 12ee76e5c..11daf941b 100644 --- a/src/improvements/template/GasConfigTemplate.sol +++ b/src/improvements/template/GasConfigTemplate.sol @@ -1,9 +1,9 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {SystemConfig} from "@eth-optimism-bedrock/src/L1/SystemConfig.sol"; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; -import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; /// @title GasConfigTemplate /// @notice Template contract for configuring gas limits @@ -66,4 +66,7 @@ contract GasConfigTemplate is MultisigTask { assertEq(systemConfig.gasLimit(), gasLimits[chainId], "l2 gas limit not set"); } } + + /// @notice no code exceptions for this template + function getCodeExceptions() internal view virtual override returns (address[] memory) {} } diff --git a/src/improvements/template/SetGameTypeTemplate.sol b/src/improvements/template/SetGameTypeTemplate.sol index 35033b3d9..1a1ce383d 100644 --- a/src/improvements/template/SetGameTypeTemplate.sol +++ b/src/improvements/template/SetGameTypeTemplate.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import { @@ -6,7 +7,6 @@ import { import {LibGameType} from "@eth-optimism-bedrock/src/dispute/lib/LibUDT.sol"; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; -import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; /// @title SetGameTypeTemplate /// @notice Template contract for setting game types in the Optimism system @@ -79,4 +79,7 @@ contract SetGameTypeTemplate is MultisigTask { ); } } + + /// @notice no code exceptions for this template + function getCodeExceptions() internal view virtual override returns (address[] memory) {} } diff --git a/src/improvements/template/TestOPCMUpgradeVxyz.sol b/src/improvements/template/TestOPCMUpgradeVxyz.sol index e6d711ef3..50461cc9a 100644 --- a/src/improvements/template/TestOPCMUpgradeVxyz.sol +++ b/src/improvements/template/TestOPCMUpgradeVxyz.sol @@ -1,4 +1,5 @@ -pragma solidity ^0.8.0; +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; import {OPCMBaseTask} from "../tasks/OPCMBaseTask.sol"; import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; @@ -74,4 +75,7 @@ contract TestOPCMUpgradeVxyz is OPCMBaseTask { /// for this dummy opcm there are no validations per l2 chain /// for a real OPCM instance, add the validations per l2chain function _validate(uint256 chainId) internal view override {} + + /// @notice no code exceptions for this template + function getCodeExceptions() internal view virtual override returns (address[] memory) {} } diff --git a/src/improvements/template/TransferOwnerTemplate.sol b/src/improvements/template/TransferOwnerTemplate.sol index a66328f4f..24ce78ca8 100644 --- a/src/improvements/template/TransferOwnerTemplate.sol +++ b/src/improvements/template/TransferOwnerTemplate.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {ProxyAdmin} from "@eth-optimism-bedrock/src/universal/ProxyAdmin.sol"; @@ -52,4 +53,7 @@ contract TransferOwnerTemplate is MultisigTask { assertEq(proxyAdmin.owner(), newOwner, "new owner not set correctly"); } + + /// @notice no code exceptions for this template + function getCodeExceptions() internal view virtual override returns (address[] memory) {} } diff --git a/src/libraries/StateDiffDecoder.sol b/src/libraries/AccountAccessParser.sol similarity index 61% rename from src/libraries/StateDiffDecoder.sol rename to src/libraries/AccountAccessParser.sol index 2e5b850e9..40ca291a7 100644 --- a/src/libraries/StateDiffDecoder.sol +++ b/src/libraries/AccountAccessParser.sol @@ -1,15 +1,44 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.15; +pragma solidity ^0.8.0; import {Vm, VmSafe} from "forge-std/Vm.sol"; import {console} from "forge-std/console.sol"; -import {LibString} from "@solady/utils/LibString.sol"; import {stdJson} from "forge-std/StdJson.sol"; +import {IERC20} from "forge-std/interfaces/IERC20.sol"; +import {LibString} from "@solady/utils/LibString.sol"; -library StateDiffDecoder { +/// @notice Parses account accesses into decoded transfers and state diffs. +/// The core methods intended to be part of the public interface are `decodeAndPrint`, `decode`, +/// `getUniqueWrites`, and `getStateDiffFor`. Example usage: +/// +/// ```solidity +/// contract MyContract { +/// using AccountAccessParser for VmSafe.AccountAccess[]; +/// +/// function myFunction(VmSafe.AccountAccess[] memory accountAccesses) public { +/// // Decode all state changes and ETH/ERC20 transfers and print to the terminal. +/// accountAccesses.decodeAndPrint(); +/// +/// // Get all decoded data without printing. +/// ( +/// AccountAccessParser.DecodedTransfer[] memory transfers, +/// AccountAccessParser.DecodedStateDiff[] memory diffs +/// ) = accountAccesses.decode(); +/// +/// // Get the state diff for a given account. +/// StateDiff[] memory diffs = accountAccesses.getStateDiffFor(myContract); +/// +/// // Get an array of all unique accounts that had state changes. +/// address[] memory accountsWithStateChanges = accountAccesses.getUniqueWrites(); +/// } +/// } +/// ``` +library AccountAccessParser { using LibString for string; using stdJson for string; + address internal constant ETHER = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; + address internal constant ZERO = address(0); address internal constant VM_ADDRESS = address(uint160(uint256(keccak256("hevm cheat code")))); Vm internal constant vm = Vm(VM_ADDRESS); @@ -35,6 +64,13 @@ library StateDiffDecoder { DecodedSlot decoded; } + struct DecodedTransfer { + address from; + address to; + uint256 value; + address tokenAddress; + } + // Temporary struct used during deduplication. struct TempStateChange { address who; @@ -54,7 +90,9 @@ library StateDiffDecoder { } // forgefmt: disable-start - bytes32 internal constant ERC1967_IMPL_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; + bytes32 internal constant ERC1967_IMPL_SLOT = bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1); + bytes32 internal constant PROXY_OWNER_ADDR_SLOT = bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1); + bytes32 internal constant UNSAFE_BLOCK_SIGNER_SLOT = keccak256("systemconfig.unsafeblocksigner"); bytes32 internal constant L1_CROSS_DOMAIN_MESSENGER_SLOT = bytes32(uint256(keccak256("systemconfig.l1crossdomainmessenger")) - 1); bytes32 internal constant L1_ERC_721_BRIDGE_SLOT = bytes32(uint256(keccak256("systemconfig.l1erc721bridge")) - 1); @@ -77,86 +115,212 @@ library StateDiffDecoder { bytes32 internal constant GAS_PAYING_TOKEN_SYMBOL_SLOT = bytes32(uint256(keccak256("opstack.gaspayingtokensymbol")) - 1); // forgefmt: disable-end - function decode(VmSafe.AccountAccess[] memory _accountAccesses) internal { - vm.pauseGasMetering(); - // First, determine the maximum possible number of storage writes. - uint256 maxEntries = 0; - for (uint256 i = 0; i < _accountAccesses.length; i++) { - for (uint256 j = 0; j < _accountAccesses[i].storageAccesses.length; j++) { - VmSafe.StorageAccess memory storageAccess = _accountAccesses[i].storageAccesses[j]; - if (storageAccess.isWrite && storageAccess.previousValue != storageAccess.newValue) { - maxEntries++; - } + modifier noGasMetering() { + // We use low-level staticcalls so we can keep cheatcodes as view functions. + (bool ok,) = address(vm).staticcall(abi.encodeWithSelector(VmSafe.pauseGasMetering.selector)); + require(ok, "pauseGasMetering failed"); + + _; + + (ok,) = address(vm).staticcall(abi.encodeWithSelector(VmSafe.resumeGasMetering.selector)); + require(ok, "resumeGasMetering failed"); + } + + // ============================================================== + // ======== Methods intended to be used as the interface ======== + // ============================================================== + + /// @notice Convenience function that wraps decode and print together. + function decodeAndPrint(VmSafe.AccountAccess[] memory _accesses) internal view { + (DecodedTransfer[] memory transfers, DecodedStateDiff[] memory stateDiffs) = decode(_accesses); + print(transfers, stateDiffs); + } + + /// @notice Decodes the provided AccountAccess array into decoded transfers and state diffs. + function decode(VmSafe.AccountAccess[] memory _accountAccesses) + internal + view + noGasMetering + returns (DecodedTransfer[] memory transfers, DecodedStateDiff[] memory stateDiffs) + { + // --- Transfers --- + // Allocate a temporary transfers array with maximum possible size (2 transfers per access). + uint256 n = _accountAccesses.length; + DecodedTransfer[] memory tempTransfers = new DecodedTransfer[](2 * n); + uint256 transferIndex = 0; + // Process each account access once for both ETH and ERC20 transfers. + for (uint256 i = 0; i < n; i++) { + DecodedTransfer memory ethTransfer = getETHTransfer(_accountAccesses[i]); + if (ethTransfer.value != 0) { + tempTransfers[transferIndex] = ethTransfer; + transferIndex++; + } + + DecodedTransfer memory erc20Transfer = getERC20Transfer(_accountAccesses[i]); + if (erc20Transfer.value != 0) { + tempTransfers[transferIndex] = erc20Transfer; + transferIndex++; } } - // Allocate a temporary array to deduplicate writes. - TempStateChange[] memory deduped = new TempStateChange[](maxEntries); - uint256 dedupCount = 0; + // Copy the valid transfers into an array of the correct length. + transfers = new DecodedTransfer[](transferIndex); + for (uint256 i = 0; i < transferIndex; i++) { + transfers[i] = tempTransfers[i]; + } - // Iterate in order so that the first write encountered holds the initial (old) value. - for (uint256 i = 0; i < _accountAccesses.length; i++) { - for (uint256 j = 0; j < _accountAccesses[i].storageAccesses.length; j++) { - VmSafe.StorageAccess memory storageAccess = _accountAccesses[i].storageAccesses[j]; + // --- State diffs --- + address[] memory uniqueAccounts = getUniqueWrites(_accountAccesses); + uint256 totalDiffCount = 0; + // Count the total number of net state diffs. + for (uint256 i = 0; i < uniqueAccounts.length; i++) { + StateDiff[] memory accountDiffs = getStateDiffFor(_accountAccesses, uniqueAccounts[i]); + totalDiffCount += accountDiffs.length; + } - // Only process writes where the value has actually changed. - if (storageAccess.isWrite && storageAccess.previousValue != storageAccess.newValue) { - bool found = false; + // Aggregate all the diffs and decode each one. + stateDiffs = new DecodedStateDiff[](totalDiffCount); + uint256 index = 0; + for (uint256 i = 0; i < uniqueAccounts.length; i++) { + StateDiff[] memory accountDiffs = getStateDiffFor(_accountAccesses, uniqueAccounts[i]); + for (uint256 j = 0; j < accountDiffs.length; j++) { + address who = uniqueAccounts[i]; + (uint256 l2ChainId, string memory contractName) = getContractInfo(who); + DecodedSlot memory decoded = + tryDecode(contractName, accountDiffs[j].slot, accountDiffs[j].oldValue, accountDiffs[j].newValue); + stateDiffs[index] = DecodedStateDiff({ + who: who, + l2ChainId: l2ChainId, + contractName: contractName, + raw: accountDiffs[j], + decoded: decoded + }); + index++; + } + } + } - // Check if we've already seen this address and slot pair. - for (uint256 k = 0; k < dedupCount; k++) { - if (deduped[k].who == storageAccess.account && deduped[k].slot == storageAccess.slot) { - // Update the new value to the latest value. - deduped[k].lastNew = storageAccess.newValue; - found = true; + /// @notice Extracts all unique storage writes (i.e. writes where the value has actually changed) + function getUniqueWrites(VmSafe.AccountAccess[] memory accesses) + internal + pure + returns (address[] memory uniqueAccounts) + { + // Temporary array sized to maximum possible length. + address[] memory temp = new address[](accesses.length); + uint256 count = 0; + for (uint256 i = 0; i < accesses.length; i++) { + if (!accesses[i].reverted) { + bool hasChangedWrite = false; + for (uint256 j = 0; j < accesses[i].storageAccesses.length; j++) { + VmSafe.StorageAccess memory sa = accesses[i].storageAccesses[j]; + if (sa.isWrite && !sa.reverted && sa.previousValue != sa.newValue) { + hasChangedWrite = true; + break; + } + } + if (hasChangedWrite) { + bool exists = false; + for (uint256 k = 0; k < count; k++) { + if (temp[k] == accesses[i].account) { + exists = true; break; } } + if (!exists) { + temp[count] = accesses[i].account; + count++; + } + } + } + } + uniqueAccounts = new address[](count); + for (uint256 i = 0; i < count; i++) { + uniqueAccounts[i] = temp[i]; + } + } - // If not found, record a new change. - if (!found) { - deduped[dedupCount] = TempStateChange({ - who: storageAccess.account, - slot: storageAccess.slot, - firstOld: storageAccess.previousValue, - lastNew: storageAccess.newValue - }); - dedupCount++; + /// @notice Extracts the net state diffs for a given account from the provided account accesses. + /// It deduplicates writes by slot and returns an array of StateDiff structs where each slot + /// appears only once and for each entry oldValue != newValue. + function getStateDiffFor(VmSafe.AccountAccess[] memory accesses, address who) + internal + pure + returns (StateDiff[] memory diffs) + { + // Over-allocate to the maximum possible number of diffs. + StateDiff[] memory temp = new StateDiff[](accesses.length); + uint256 diffCount = 0; + + for (uint256 i = 0; i < accesses.length; i++) { + if (!accesses[i].reverted) { + for (uint256 j = 0; j < accesses[i].storageAccesses.length; j++) { + VmSafe.StorageAccess memory sa = accesses[i].storageAccesses[j]; + if (sa.account == who && sa.isWrite && !sa.reverted && sa.previousValue != sa.newValue) { + // Check if we already recorded a diff for this slot. + bool found = false; + for (uint256 k = 0; k < diffCount; k++) { + if (temp[k].slot == sa.slot) { + // Update the new value. + temp[k].newValue = sa.newValue; + found = true; + break; + } + } + if (!found) { + temp[diffCount] = + StateDiff({slot: sa.slot, oldValue: sa.previousValue, newValue: sa.newValue}); + diffCount++; + } } } } } - // Filter out state changes where the first old value equals the final new value. - DecodedStateDiff[] memory tempStateDiffs = new DecodedStateDiff[](dedupCount); - uint256 filteredCount = 0; - for (uint256 i = 0; i < dedupCount; i++) { - if (deduped[i].firstOld == deduped[i].lastNew) { - continue; + // Filter out diffs where the net change is zero. + uint256 finalCount = 0; + for (uint256 i = 0; i < diffCount; i++) { + if (temp[i].oldValue != temp[i].newValue) { + temp[finalCount] = temp[i]; + finalCount++; } - address who = deduped[i].who; - (uint256 l2ChainId, string memory contractName) = getContractInfo(who); - DecodedSlot memory decoded = - tryDecode(contractName, deduped[i].slot, deduped[i].firstOld, deduped[i].lastNew); - tempStateDiffs[filteredCount] = DecodedStateDiff({ - who: who, - l2ChainId: l2ChainId, - contractName: contractName, - raw: StateDiff({slot: deduped[i].slot, oldValue: deduped[i].firstOld, newValue: deduped[i].lastNew}), - decoded: decoded - }); - filteredCount++; } - // Create the final array with the exact number of filtered entries. - DecodedStateDiff[] memory stateDiffs = new DecodedStateDiff[](filteredCount); - for (uint256 i = 0; i < filteredCount; i++) { - stateDiffs[i] = tempStateDiffs[i]; + // Allocate and copy the final array. + diffs = new StateDiff[](finalCount); + for (uint256 i = 0; i < finalCount; i++) { + diffs[i] = temp[i]; + } + } + + // ========================================= + // ======== Internal helper methods ======== + // ========================================= + + /// @notice Prints the decoded transfers and state diffs to the console. + function print(DecodedTransfer[] memory _transfers, DecodedStateDiff[] memory _stateDiffs) + internal + view + noGasMetering + { + console.log("\n----------------- Task Transfers -------------------"); + if (_transfers.length == 0) { + console.log("No ETH or ERC20 transfers."); + } else { + for (uint256 i = 0; i < _transfers.length; i++) { + DecodedTransfer memory transfer = _transfers[i]; + console.log("\n----- DecodedTransfer[%s] -----", i); + console.log("From: %s", transfer.from); + console.log("To: %s", transfer.to); + console.log("Value: %s", transfer.value); + console.log("Token Address: %s", transfer.tokenAddress); + } } - // Lastly, print the decoded state infos. - for (uint256 i = 0; i < stateDiffs.length; i++) { - DecodedStateDiff memory state = stateDiffs[i]; + console.log("\n----------------- Task State Changes -------------------"); + require(_stateDiffs.length > 0, "No state changes found, this is unexpected."); + for (uint256 i = 0; i < _stateDiffs.length; i++) { + DecodedStateDiff memory state = _stateDiffs[i]; console.log("\n----- DecodedStateDiff[%s] -----", i); console.log("Who: %s", state.who); console.log("Contract: %s", state.contractName); @@ -165,7 +329,6 @@ library StateDiffDecoder { console.log("Raw Old Value: %s", vm.toString(state.raw.oldValue)); console.log("Raw New Value: %s", vm.toString(state.raw.newValue)); - // Check if decoded.kind is empty. if (bytes(state.decoded.kind).length == 0) { console.log("\x1B[33m[WARN]\x1B[0m Slot was not decoded"); } else { @@ -176,9 +339,41 @@ library StateDiffDecoder { console.log("Detail: %s", state.decoded.detail); } } - vm.resumeGasMetering(); } + /// @notice Decodes an ETH transfer from an account access record, and returns an empty struct + /// if no transfer occurred. + function getETHTransfer(VmSafe.AccountAccess memory access) internal pure returns (DecodedTransfer memory) { + return access.value != 0 && !access.reverted + ? DecodedTransfer({from: access.accessor, to: access.account, value: access.value, tokenAddress: ETHER}) + : DecodedTransfer({from: ZERO, to: ZERO, value: 0, tokenAddress: ZERO}); + } + + /// @notice Decodes an ERC20 transfer from an account access record, and returns an empty struct + /// if no ERC20 transfer is detected. + function getERC20Transfer(VmSafe.AccountAccess memory access) internal pure returns (DecodedTransfer memory) { + bytes memory data = access.data; + if (data.length <= 4) return DecodedTransfer({from: ZERO, to: ZERO, value: 0, tokenAddress: ZERO}); + + bytes4 selector = bytes4(data); + bytes memory params = new bytes(data.length - 4); + for (uint256 j = 0; j < data.length - 4; j++) { + params[j] = data[j + 4]; + } + + bool reverted = access.reverted; + if (selector == IERC20.transfer.selector && !reverted) { + (address to, uint256 value) = abi.decode(params, (address, uint256)); + return DecodedTransfer({from: access.accessor, to: to, value: value, tokenAddress: access.account}); + } else if (selector == IERC20.transferFrom.selector && !reverted) { + (address from, address to, uint256 value) = abi.decode(params, (address, address, uint256)); + return DecodedTransfer({from: from, to: to, value: value, tokenAddress: access.account}); + } else { + return DecodedTransfer({from: ZERO, to: ZERO, value: 0, tokenAddress: ZERO}); + } + } + + /// @notice Given an address, returns the contract name and L2 chain ID for the contract. function getContractInfo(address _address) internal view @@ -222,6 +417,16 @@ library StateDiffDecoder { }); } + if (_slot == PROXY_OWNER_ADDR_SLOT) { + return DecodedSlot({ + kind: "address", + oldValue: toAddress(_oldValue), + newValue: toAddress(_newValue), + summary: "Proxy owner address", + detail: "Standard slot for storing the owner address in a Proxy contract." + }); + } + // SystemConfig. if (_slot == UNSAFE_BLOCK_SIGNER_SLOT) { return DecodedSlot({ diff --git a/src/libraries/StateDiffProcessor.sol b/src/libraries/StateDiffProcessor.sol deleted file mode 100644 index 4c2dcce25..000000000 --- a/src/libraries/StateDiffProcessor.sol +++ /dev/null @@ -1,140 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.15; - -import {VmSafe} from "forge-std/Vm.sol"; -import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; - -/// @notice Processes account accesses and returns transfers and state diffs. -/// The `process` function is the interface to be used by callers, and other -/// functions are internal helpers. -library StateDiffProcessor { - /// @notice Special constant representing ETH transfers. - address internal constant ETH_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; - - /// @notice The zero address. - address internal constant ZERO = address(0); - - /// @notice Struct to represent a token/ETH transfer. - struct TransferInfo { - address from; - address to; - uint256 value; - address tokenAddress; - } - - /// @notice Struct to represent a storage state change. - struct StateDiff { - address who; - bytes32 slot; - bytes32 oldValue; - bytes32 newValue; - } - - /// @notice Processes an array of VmSafe.AccountAccess records and return all transfers and - /// state diffs. - function process(VmSafe.AccountAccess[] memory accountAccesses) - internal - pure - returns (TransferInfo[] memory transfers, StateDiff[] memory stateDiffs) - { - uint256 totalTransfers = 0; - uint256 totalStateChanges = 0; - for (uint256 i = 0; i < accountAccesses.length; i++) { - TransferInfo memory ethTransfer = processEthTransfer(accountAccesses[i]); - if (ethTransfer.value != 0) { - totalTransfers++; - } - TransferInfo memory erc20Transfer = processERC20Transfer(accountAccesses[i]); - if (erc20Transfer.value != 0) { - totalTransfers++; - } - StateDiff[] memory states = processStateDiffs(accountAccesses[i].storageAccesses); - totalStateChanges += states.length; - } - - transfers = new TransferInfo[](totalTransfers); - stateDiffs = new StateDiff[](totalStateChanges); - uint256 transferIndex = 0; - uint256 stateIndex = 0; - for (uint256 i = 0; i < accountAccesses.length; i++) { - TransferInfo memory ethTransfer = processEthTransfer(accountAccesses[i]); - if (ethTransfer.value != 0) { - transfers[transferIndex] = ethTransfer; - transferIndex++; - } - TransferInfo memory erc20Transfer = processERC20Transfer(accountAccesses[i]); - if (erc20Transfer.value != 0) { - transfers[transferIndex] = erc20Transfer; - transferIndex++; - } - StateDiff[] memory states = processStateDiffs(accountAccesses[i].storageAccesses); - for (uint256 j = 0; j < states.length; j++) { - stateDiffs[stateIndex] = states[j]; - stateIndex++; - } - } - } - - /// @notice Processes an ETH transfer from an account access record. - /// Returns a TransferInfo struct containing transfer details. If no transfer - /// occurred, all fields are zero. - function processEthTransfer(VmSafe.AccountAccess memory access) internal pure returns (TransferInfo memory) { - return access.value != 0 - ? TransferInfo({from: access.accessor, to: access.account, value: access.value, tokenAddress: ETH_TOKEN}) - : TransferInfo({from: ZERO, to: ZERO, value: 0, tokenAddress: ZERO}); - } - - /// @notice Processes an ERC20 transfer from an account access record. - /// Returns a TransferInfo struct containing transfer details. If no valid - /// ERC20 transfer is detected, all fields are zero. - function processERC20Transfer(VmSafe.AccountAccess memory access) internal pure returns (TransferInfo memory) { - bytes memory data = access.data; - if (data.length <= 4) { - return TransferInfo({from: ZERO, to: ZERO, value: 0, tokenAddress: ZERO}); - } - - bytes4 selector = bytes4(data); - bytes memory params = new bytes(data.length - 4); - for (uint256 j = 0; j < data.length - 4; j++) { - params[j] = data[j + 4]; - } - - if (selector == IERC20.transfer.selector) { - (address to, uint256 value) = abi.decode(params, (address, uint256)); - return TransferInfo({from: access.accessor, to: to, value: value, tokenAddress: access.account}); - } else if (selector == IERC20.transferFrom.selector) { - (address from, address to, uint256 value) = abi.decode(params, (address, address, uint256)); - return TransferInfo({from: from, to: to, value: value, tokenAddress: access.account}); - } else { - return TransferInfo({from: ZERO, to: ZERO, value: 0, tokenAddress: ZERO}); - } - } - - /// @notice Processes storage changes from an array of storage access records. - function processStateDiffs(VmSafe.StorageAccess[] memory accesses) - internal - pure - returns (StateDiff[] memory diffs) - { - uint256 count = 0; - for (uint256 i = 0; i < accesses.length; i++) { - if (accesses[i].isWrite) { - count++; - } - } - - diffs = new StateDiff[](count); - uint256 diffCount = 0; - for (uint256 i = 0; i < accesses.length; i++) { - if (accesses[i].isWrite) { - diffs[diffCount] = StateDiff({ - who: accesses[i].account, - slot: accesses[i].slot, - oldValue: accesses[i].previousValue, - newValue: accesses[i].newValue - }); - diffCount++; - } - } - } -} diff --git a/tasks/sep/zora-002-fp-upgrade/NestedSignFromJson.s.sol b/tasks/sep/zora-002-fp-upgrade/NestedSignFromJson.s.sol index 4c75a1c80..aa1e4bc95 100644 --- a/tasks/sep/zora-002-fp-upgrade/NestedSignFromJson.s.sol +++ b/tasks/sep/zora-002-fp-upgrade/NestedSignFromJson.s.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.15; import {console2 as console} from "forge-std/console2.sol"; -import {Vm} from "forge-std/Vm.sol"; +import {Vm, VmSafe} from "forge-std/Vm.sol"; import {stdJson} from "forge-std/StdJson.sol"; import {Simulation} from "@base-contracts/script/universal/Simulation.sol"; import {LibString} from "@solady/utils/LibString.sol"; @@ -26,13 +26,14 @@ import {IL1ChugSplashProxy} from "@eth-optimism-bedrock/interfaces/legacy/IL1Chu import {IOptimismMintableERC20Factory} from "@eth-optimism-bedrock/interfaces/universal/IOptimismMintableERC20Factory.sol"; -import {StateDiffDecoder} from "src/libraries/StateDiffDecoder.sol"; +import {AccountAccessParser} from "src/libraries/AccountAccessParser.sol"; interface ISystemConfigLegacy is ISystemConfig { function l2OutputOracle() external view returns (address); } contract NestedSignFromJson is OriginalNestedSignFromJson, CouncilFoundationNestedSign, SuperchainRegistry { + using AccountAccessParser for VmSafe.AccountAccess[]; /// @notice Expected address for the AnchorStateRegistry proxy. IAnchorStateRegistry expectedAnchorStateRegistryProxy = IAnchorStateRegistry(vm.envAddress("EXPECTED_ANCHOR_STATE_REGISTRY_PROXY")); @@ -119,7 +120,7 @@ contract NestedSignFromJson is OriginalNestedSignFromJson, CouncilFoundationNest checkPermissionedDisputeGame(); console.log("All assertions passed!"); - StateDiffDecoder.decode(accesses); + accesses.decodeAndPrint(); } /// @notice Checks the input to the script. diff --git a/test/libraries/AccountAccessParser.t.sol b/test/libraries/AccountAccessParser.t.sol new file mode 100644 index 000000000..e87a086cc --- /dev/null +++ b/test/libraries/AccountAccessParser.t.sol @@ -0,0 +1,690 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.15; + +// Forge +import {Test} from "forge-std/Test.sol"; +import {VmSafe} from "forge-std/Vm.sol"; +import {IERC20} from "forge-std/interfaces/IERC20.sol"; + +// Libraries +import {AccountAccessParser} from "src/libraries/AccountAccessParser.sol"; + +contract AccountAccessParser_decodeAndPrint_Test is Test { + using AccountAccessParser for VmSafe.AccountAccess[]; + + bool constant isWrite = true; + bool constant reverted = true; + + bytes32 constant slot0 = bytes32(0); + + bytes32 constant val0 = bytes32(uint256(0)); + bytes32 constant val1 = bytes32(uint256(1)); + bytes32 constant val2 = bytes32(uint256(2)); + bytes32 constant val3 = bytes32(uint256(3)); + + address constant addr0 = address(0); + address constant addr1 = address(1); + address constant addr2 = address(2); + address constant addr3 = address(3); + address constant addr4 = address(4); + address constant addr5 = address(5); + address constant addr6 = address(6); + address constant addr7 = address(7); + address constant addr8 = address(8); + address constant addr9 = address(9); + address constant addr10 = address(10); + + function test_getUniqueWrites_succeeds() public pure { + // Test basic case - single account with single changed write + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr0, slot0, isWrite, val0, val1); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr0, storageAccesses); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 1, "10"); + assertEq(uniqueAccounts[0], addr0, "20"); + } + + // Test multiple writes to same account - should only appear once + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](2); + storageAccesses[0] = storageAccess(addr1, slot0, isWrite, val0, val1); + storageAccesses[1] = storageAccess(addr1, val1, isWrite, val0, val2); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, storageAccesses); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 1, "30"); + assertEq(uniqueAccounts[0], addr1, "40"); + } + + // Test writes with no changes - should not be included + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr2, slot0, isWrite, val1, val1); // Same value + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr2, storageAccesses); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 0, "50"); + } + + // Test reads - should not be included + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr3, slot0, !isWrite, val0, val1); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr3, storageAccesses); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 0, "60"); + } + + // Test reverted writes - should not be included + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr3, slot0, isWrite, val0, val1); + storageAccesses[0].reverted = reverted; + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr3, storageAccesses); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 0, "70"); + } + + // Test multiple accounts with mixed read/writes/reverts + { + VmSafe.StorageAccess[] memory storageAccesses1 = new VmSafe.StorageAccess[](3); + storageAccesses1[0] = storageAccess(addr4, slot0, isWrite, val0, val1); // Changed write + storageAccesses1[1] = storageAccess(addr4, val1, !isWrite, val0, val1); // Read + storageAccesses1[2] = storageAccess(addr4, val2, isWrite, val0, val2); // Reverted write + storageAccesses1[2].reverted = reverted; + + VmSafe.StorageAccess[] memory storageAccesses2 = new VmSafe.StorageAccess[](2); + storageAccesses2[0] = storageAccess(addr5, slot0, isWrite, val1, val1); // Unchanged write + storageAccesses2[1] = storageAccess(addr5, val1, isWrite, val0, val2); // Changed write + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](2); + accesses[0] = accountAccess(addr4, storageAccesses1); + accesses[1] = accountAccess(addr5, storageAccesses2); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 2, "80"); + assertEq(uniqueAccounts[0], addr4, "90"); + assertEq(uniqueAccounts[1], addr5, "100"); + } + + // Test empty storage accesses + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](0); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr6, storageAccesses); + + address[] memory uniqueAccounts = accesses.getUniqueWrites(); + assertEq(uniqueAccounts.length, 0, "110"); + } + } + + function test_getStateDiffFor_succeeds() public pure { + // Test single account with single changed write + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr1, slot0, isWrite, val0, val1); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr1); + assertEq(diffs.length, 1, "10"); + assertEq(diffs[0].slot, slot0, "20"); + assertEq(diffs[0].oldValue, val0, "30"); + assertEq(diffs[0].newValue, val1, "40"); + } + + // Test single account with multiple writes to same slot (should only keep last write) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](2); + storageAccesses[0] = storageAccess(addr2, slot0, isWrite, val0, val1); + storageAccesses[1] = storageAccess(addr2, slot0, isWrite, val1, val2); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr2, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr2); + assertEq(diffs.length, 1, "50"); + assertEq(diffs[0].slot, slot0, "60"); + assertEq(diffs[0].oldValue, val0, "70"); + assertEq(diffs[0].newValue, val2, "80"); + } + + // Test single account with unchanged write (should be excluded) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr3, slot0, isWrite, val1, val1); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr3, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr3); + assertEq(diffs.length, 0, "90"); + } + + // Test single account with reads (should be excluded) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr4, slot0, !isWrite, val0, val1); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr4, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr4); + assertEq(diffs.length, 0, "100"); + } + + // Test single account with reverted write (should be excluded) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr4, slot0, isWrite, val0, val1); + storageAccesses[0].reverted = reverted; + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr4, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr4); + assertEq(diffs.length, 0, "110"); + } + + // Test multiple accounts but only requesting one + { + VmSafe.StorageAccess[] memory storageAccesses1 = new VmSafe.StorageAccess[](1); + storageAccesses1[0] = storageAccess(addr5, slot0, isWrite, val0, val1); + VmSafe.StorageAccess[] memory storageAccesses2 = new VmSafe.StorageAccess[](1); + storageAccesses2[0] = storageAccess(addr6, slot0, isWrite, val0, val2); + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](2); + accesses[0] = accountAccess(addr5, storageAccesses1); + accesses[1] = accountAccess(addr6, storageAccesses2); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr5); + assertEq(diffs.length, 1, "120"); + assertEq(diffs[0].slot, slot0, "130"); + assertEq(diffs[0].oldValue, val0, "140"); + assertEq(diffs[0].newValue, val1, "150"); + } + + // Test requesting non-existent account + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr7, slot0, isWrite, val0, val1); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr7, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr8); + assertEq(diffs.length, 0, "160"); + } + + // Test empty storage accesses + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](0); + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr9, storageAccesses); + + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr9); + assertEq(diffs.length, 0, "170"); + } + + // Test empty accesses array + { + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](0); + AccountAccessParser.StateDiff[] memory diffs = accesses.getStateDiffFor(addr10); + assertEq(diffs.length, 0, "180"); + } + } + + function test_getETHTransfer_succeeds() public pure { + // Test successful ETH transfer + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.value = 100; + access.accessor = addr2; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getETHTransfer(access); + assertEq(transfer.from, addr2, "10"); + assertEq(transfer.to, addr1, "20"); + assertEq(transfer.value, 100, "30"); + assertEq(transfer.tokenAddress, AccountAccessParser.ETHER, "40"); + } + + // Test reverted ETH transfer (should return zero values) + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.value = 100; + access.accessor = addr2; + access.reverted = reverted; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getETHTransfer(access); + assertEq(transfer.from, addr0, "50"); + assertEq(transfer.to, addr0, "60"); + assertEq(transfer.value, 0, "70"); + assertEq(transfer.tokenAddress, addr0, "80"); + } + + // Test zero value transfer + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.value = 0; + access.accessor = addr2; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getETHTransfer(access); + assertEq(transfer.from, addr0, "90"); + assertEq(transfer.to, addr0, "100"); + assertEq(transfer.value, 0, "110"); + assertEq(transfer.tokenAddress, addr0, "120"); + } + + // Test transfer with max uint256 value + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.value = type(uint256).max; + access.accessor = addr2; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getETHTransfer(access); + assertEq(transfer.from, addr2, "130"); + assertEq(transfer.to, addr1, "140"); + assertEq(transfer.value, type(uint256).max, "150"); + assertEq(transfer.tokenAddress, AccountAccessParser.ETHER, "160"); + } + + // Test transfer with zero addresses + { + VmSafe.AccountAccess memory access = accountAccess(addr0, new VmSafe.StorageAccess[](0)); + access.value = 100; + access.accessor = addr0; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getETHTransfer(access); + assertEq(transfer.from, addr0, "170"); + assertEq(transfer.to, addr0, "180"); + assertEq(transfer.value, 100, "190"); + assertEq(transfer.tokenAddress, AccountAccessParser.ETHER, "200"); + } + } + + function test_getERC20Transfer_succeeds() public pure { + // Test ERC20 transfer + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, 100); + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr2, "10"); + assertEq(transfer.to, addr3, "20"); + assertEq(transfer.value, 100, "30"); + assertEq(transfer.tokenAddress, addr1, "40"); + } + + // Test reverted ERC20 transfer (should return zero values) + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, 100); + access.reverted = reverted; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr0, "50"); + assertEq(transfer.to, addr0, "60"); + assertEq(transfer.value, 0, "70"); + assertEq(transfer.tokenAddress, addr0, "80"); + } + + // Test ERC20 transferFrom + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = abi.encodeWithSelector(IERC20.transferFrom.selector, addr3, addr4, 100); + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr3, "90"); + assertEq(transfer.to, addr4, "100"); + assertEq(transfer.value, 100, "110"); + assertEq(transfer.tokenAddress, addr1, "120"); + } + + // Test reverted ERC20 transferFrom (should return zero values) + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = abi.encodeWithSelector(IERC20.transferFrom.selector, addr3, addr4, 100); + access.reverted = reverted; + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr0, "130"); + assertEq(transfer.to, addr0, "140"); + assertEq(transfer.value, 0, "150"); + assertEq(transfer.tokenAddress, addr0, "160"); + } + + // Test invalid selector (should return zero values) + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = abi.encodeWithSelector(bytes4(keccak256("invalidFunction()")), addr3, 100); + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr0, "170"); + assertEq(transfer.to, addr0, "180"); + assertEq(transfer.value, 0, "190"); + assertEq(transfer.tokenAddress, addr0, "200"); + } + + // Test empty data (should return zero values) + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = new bytes(0); + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr0, "210"); + assertEq(transfer.to, addr0, "220"); + assertEq(transfer.value, 0, "230"); + assertEq(transfer.tokenAddress, addr0, "240"); + } + + // Test max uint256 value transfer + { + VmSafe.AccountAccess memory access = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + access.accessor = addr2; + access.data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, type(uint256).max); + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr2, "250"); + assertEq(transfer.to, addr3, "260"); + assertEq(transfer.value, type(uint256).max, "270"); + assertEq(transfer.tokenAddress, addr1, "280"); + } + + // Test with zero addresses + { + VmSafe.AccountAccess memory access = accountAccess(addr0, new VmSafe.StorageAccess[](0)); + access.accessor = addr0; + access.data = abi.encodeWithSelector(IERC20.transfer.selector, addr0, 100); + + AccountAccessParser.DecodedTransfer memory transfer = AccountAccessParser.getERC20Transfer(access); + assertEq(transfer.from, addr0, "290"); + assertEq(transfer.to, addr0, "300"); + assertEq(transfer.value, 100, "310"); + assertEq(transfer.tokenAddress, addr0, "320"); + } + } + + function test_decode_succeeds() public view { + // Test empty array + { + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](0); + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "10"); + assertEq(diffs.length, 0, "20"); + } + + // Test ETH transfer only + { + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + accesses[0].accessor = addr2; + accesses[0].value = 100; + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 1, "30"); + assertEq(transfers[0].from, addr2, "40"); + assertEq(transfers[0].to, addr1, "50"); + assertEq(transfers[0].value, 100, "60"); + assertEq(transfers[0].tokenAddress, AccountAccessParser.ETHER, "70"); + assertEq(diffs.length, 0, "80"); + } + + // Test reverted ETH transfer (should be excluded) + { + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + accesses[0].accessor = addr2; + accesses[0].value = 100; + accesses[0].reverted = reverted; + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "90"); + assertEq(diffs.length, 0, "100"); + } + + // Test ERC20 transfer only + { + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + accesses[0].accessor = addr2; + accesses[0].data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, 100); + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 1, "110"); + assertEq(transfers[0].from, addr2, "120"); + assertEq(transfers[0].to, addr3, "130"); + assertEq(transfers[0].value, 100, "140"); + assertEq(transfers[0].tokenAddress, addr1, "150"); + assertEq(diffs.length, 0, "160"); + } + + // Test reverted ERC20 transfer (should be excluded) + { + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, new VmSafe.StorageAccess[](0)); + accesses[0].accessor = addr2; + accesses[0].data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, 100); + accesses[0].reverted = reverted; + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "170"); + assertEq(diffs.length, 0, "180"); + } + + // Test state diffs only + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr1, AccountAccessParser.GUARDIAN_SLOT, isWrite, val0, val2); + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, storageAccesses); + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "190"); + assertEq(diffs.length, 1, "200"); + assertEq(diffs[0].who, addr1, "210"); + assertEq(diffs[0].raw.slot, AccountAccessParser.GUARDIAN_SLOT, "220"); + assertEq(diffs[0].raw.oldValue, val0, "230"); + assertEq(diffs[0].raw.newValue, val2, "240"); + } + + // Test reverted state diffs (should be excluded) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr1, AccountAccessParser.GUARDIAN_SLOT, isWrite, val0, val2); + storageAccesses[0].reverted = reverted; + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, storageAccesses); + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "250"); + assertEq(diffs.length, 0, "260"); + } + + // Test combination of transfers and state diffs + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](1); + storageAccesses[0] = storageAccess(addr1, AccountAccessParser.PAUSED_SLOT, isWrite, val0, val1); + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](2); + accesses[0] = accountAccess(addr1, storageAccesses); + accesses[0].value = 100; // ETH transfer + accesses[1] = accountAccess(addr2, new VmSafe.StorageAccess[](0)); + accesses[1].data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, 200); // ERC20 transfer + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 2, "270"); + assertEq(diffs.length, 1, "280"); + + // Check ETH transfer + assertEq(transfers[0].value, 100, "290"); + assertEq(transfers[0].tokenAddress, AccountAccessParser.ETHER, "300"); + + // Check ERC20 transfer + assertEq(transfers[1].from, addr0, "310"); + assertEq(transfers[1].to, addr3, "320"); + assertEq(transfers[1].value, 200, "330"); + assertEq(transfers[1].tokenAddress, addr2, "340"); + + // Check state diff + assertEq(diffs[0].who, addr1, "350"); + assertEq(diffs[0].raw.slot, AccountAccessParser.PAUSED_SLOT, "360"); + assertEq(diffs[0].raw.oldValue, val0, "370"); + assertEq(diffs[0].raw.newValue, val1, "380"); + } + + // Test combination of reverted and non-reverted operations + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](2); + storageAccesses[0] = storageAccess(addr1, AccountAccessParser.PAUSED_SLOT, isWrite, val0, val1); + storageAccesses[1] = storageAccess(addr1, AccountAccessParser.GUARDIAN_SLOT, isWrite, val0, val2); + storageAccesses[1].reverted = reverted; + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](2); + accesses[0] = accountAccess(addr1, storageAccesses); + accesses[0].value = 100; // ETH transfer + accesses[1] = accountAccess(addr2, new VmSafe.StorageAccess[](0)); + accesses[1].data = abi.encodeWithSelector(IERC20.transfer.selector, addr3, 200); // ERC20 transfer + accesses[1].reverted = reverted; + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 1, "390"); + assertEq(transfers[0].value, 100, "400"); + assertEq(transfers[0].tokenAddress, AccountAccessParser.ETHER, "410"); + assertEq(diffs.length, 1, "420"); + assertEq(diffs[0].raw.oldValue, val0, "430"); + assertEq(diffs[0].raw.newValue, val1, "440"); + } + + // Test state changes that revert back to original (should not appear in diffs) + { + VmSafe.StorageAccess[] memory storageAccesses = new VmSafe.StorageAccess[](2); + storageAccesses[0] = storageAccess(addr1, AccountAccessParser.PAUSED_SLOT, isWrite, val0, val1); + storageAccesses[1] = storageAccess(addr1, AccountAccessParser.PAUSED_SLOT, isWrite, val1, val0); + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](1); + accesses[0] = accountAccess(addr1, storageAccesses); + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "450"); + assertEq(diffs.length, 0, "460"); + } + + // Test multiple accounts with state changes + { + VmSafe.StorageAccess[] memory storageAccesses1 = new VmSafe.StorageAccess[](1); + storageAccesses1[0] = storageAccess(addr1, AccountAccessParser.PAUSED_SLOT, isWrite, val0, val1); + + VmSafe.StorageAccess[] memory storageAccesses2 = new VmSafe.StorageAccess[](1); + storageAccesses2[0] = storageAccess(addr2, AccountAccessParser.GUARDIAN_SLOT, isWrite, val0, val3); + + VmSafe.AccountAccess[] memory accesses = new VmSafe.AccountAccess[](2); + accesses[0] = accountAccess(addr1, storageAccesses1); + accesses[1] = accountAccess(addr2, storageAccesses2); + + ( + AccountAccessParser.DecodedTransfer[] memory transfers, + AccountAccessParser.DecodedStateDiff[] memory diffs + ) = accesses.decode(); + + assertEq(transfers.length, 0, "470"); + assertEq(diffs.length, 2, "480"); + assertEq(diffs[0].who, addr1, "490"); + assertEq(diffs[0].raw.slot, AccountAccessParser.PAUSED_SLOT, "500"); + assertEq(diffs[0].raw.oldValue, val0, "510"); + assertEq(diffs[0].raw.newValue, val1, "520"); + assertEq(diffs[1].who, addr2, "530"); + assertEq(diffs[1].raw.slot, AccountAccessParser.GUARDIAN_SLOT, "540"); + assertEq(diffs[1].raw.oldValue, val0, "550"); + assertEq(diffs[1].raw.newValue, val3, "560"); + } + } + + function accountAccess(address _account, VmSafe.StorageAccess[] memory _storageAccesses) + internal + pure + returns (VmSafe.AccountAccess memory) + { + return VmSafe.AccountAccess({ + chainInfo: VmSafe.ChainInfo({chainId: 1, forkId: 1}), + kind: VmSafe.AccountAccessKind.Call, + account: _account, + accessor: addr0, + initialized: true, + oldBalance: 0, + newBalance: 0, + deployedCode: new bytes(0), + value: 0, + data: new bytes(0), + reverted: false, + storageAccesses: _storageAccesses, + depth: 0 + }); + } + + function storageAccess(address _account, bytes32 _slot, bool _isWrite, bytes32 _previousValue, bytes32 _newValue) + internal + pure + returns (VmSafe.StorageAccess memory) + { + return VmSafe.StorageAccess({ + account: _account, + slot: _slot, + isWrite: _isWrite, + previousValue: _previousValue, + newValue: _newValue, + reverted: false + }); + } +} diff --git a/test/tasks/MultisigTask.t.sol b/test/tasks/MultisigTask.t.sol index 7ecfb227d..572c281d2 100644 --- a/test/tasks/MultisigTask.t.sol +++ b/test/tasks/MultisigTask.t.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.15; import {IMulticall3} from "forge-std/interfaces/IMulticall3.sol"; +import {VmSafe} from "forge-std/Vm.sol"; import {Test} from "forge-std/Test.sol"; import {IGnosisSafe, Enum} from "@base-contracts/script/universal/IGnosisSafe.sol"; @@ -138,7 +139,7 @@ contract MultisigTaskUnitTest is Test { ); vm.expectRevert("MultisigTask: hash mismatch"); - task.simulate(); + task.simulate(""); } function testBuildFailsRevertPreviousSnapshotFails() public { @@ -173,11 +174,11 @@ contract MultisigTaskUnitTest is Test { task.simulateRun(MAINNET_CONFIG); } - function testRun() public { + function testRun() public returns (VmSafe.AccountAccess[] memory accountAccesses) { vm.expectRevert("No actions found"); task.getTaskActions(); - task.simulateRun(MAINNET_CONFIG); + accountAccesses = task.simulateRun(MAINNET_CONFIG); (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = task.getTaskActions(); @@ -205,13 +206,13 @@ contract MultisigTaskUnitTest is Test { } function testSimulateFailsTxAlreadyExecuted() public { - testRun(); + VmSafe.AccountAccess[] memory accountAccesses = testRun(); vm.expectRevert("GS025"); - task.simulate(); + task.simulate(""); - // validations should pass after a successful run - task.validate(); + /// validations should pass after a successful run + task.validate(accountAccesses); } function testGetCalldata() public { diff --git a/test/tasks/NestedMultisigTask.t.sol b/test/tasks/NestedMultisigTask.t.sol index 0ac96e9d7..3d5349823 100644 --- a/test/tasks/NestedMultisigTask.t.sol +++ b/test/tasks/NestedMultisigTask.t.sol @@ -1,18 +1,18 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; +import {IDisputeGameFactory} from "@eth-optimism-bedrock/interfaces/dispute/IDisputeGameFactory.sol"; import {IGnosisSafe, Enum} from "@base-contracts/script/universal/IGnosisSafe.sol"; import {IMulticall3} from "forge-std/interfaces/IMulticall3.sol"; +import {Signatures} from "@base-contracts/script/universal/Signatures.sol"; +import {GameTypes} from "@eth-optimism-bedrock/src/dispute/lib/Types.sol"; +import {LibSort} from "@solady/utils/LibSort.sol"; import {Test} from "forge-std/Test.sol"; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; +import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; import {TestOPCMUpgradeVxyz} from "src/improvements/template/TestOPCMUpgradeVxyz.sol"; import {DisputeGameUpgradeTemplate} from "src/improvements/template/DisputeGameUpgradeTemplate.sol"; -import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; -import {LibSort} from "@solady/utils/LibSort.sol"; -import {Signatures} from "@base-contracts/script/universal/Signatures.sol"; -import {IDisputeGameFactory} from "@eth-optimism-bedrock/interfaces/dispute/IDisputeGameFactory.sol"; -import {GameTypes} from "@eth-optimism-bedrock/src/dispute/lib/Types.sol"; /// @notice This test is used to test the nested multisig task. contract NestedMultisigTaskTest is Test { @@ -205,12 +205,9 @@ contract NestedMultisigTaskTest is Test { multisigTask.approveFromChildMultisig(taskConfigFilePath, childMultisig, packedSignaturesChild); } - // no offchain signatures for the parent multisig - bytes memory packedSignaturesParent; - - // execute the task + /// execute the task multisigTask = new DisputeGameUpgradeTemplate(); - multisigTask.executeRun(taskConfigFilePath, packedSignaturesParent); + multisigTask.simulateRun(taskConfigFilePath); // check that the implementation is upgraded correctly assertEq( @@ -302,12 +299,9 @@ contract NestedMultisigTaskTest is Test { multisigTask.approveFromChildMultisig(opcmTaskConfigFilePath, childMultisig, packedSignaturesChild); } - // no offchain signatures for the parent multisig - bytes memory packedSignaturesParent; - // execute the task multisigTask = new TestOPCMUpgradeVxyz(); - multisigTask.executeRun(opcmTaskConfigFilePath, packedSignaturesParent); + multisigTask.simulateRun(opcmTaskConfigFilePath); } function getNestedDataToSign(address owner) internal view returns (bytes memory) { diff --git a/test/tasks/Regression.t.sol b/test/tasks/Regression.t.sol index 7b87c5972..ee9b0436b 100644 --- a/test/tasks/Regression.t.sol +++ b/test/tasks/Regression.t.sol @@ -8,10 +8,15 @@ import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; import {GasConfigTemplate} from "src/improvements/template/GasConfigTemplate.sol"; import {SetGameTypeTemplate} from "src/improvements/template/SetGameTypeTemplate.sol"; import {DisputeGameUpgradeTemplate} from "src/improvements/template/DisputeGameUpgradeTemplate.sol"; +import {TestOPCMUpgradeVxyz} from "src/improvements/template/TestOPCMUpgradeVxyz.sol"; +/// @notice test that the call data and data to sign generated in simulateRun for the multisigs +/// are always the same. This means that if there were any bug introduced in the multisig task, or opcm base task, +/// same call data or data to sign will not be generated at the same block and these tests will fail. contract RegressionTest is Test { function testRegressionCallDataMatches_SingleMultisigGasConfigTemplate() public { string memory taskConfigFilePath = "test/tasks/mock/configs/SingleMultisigGasConfigTemplate.toml"; + // call data generated by manually running the gas config template at block 21724199 on mainnet string memory expectedCallData = "0x174dea7100000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000001200000000000000000000000005e6432f18bc5d497b1ab2288a025fbf9d69e22210000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000024b40a817c0000000000000000000000000000000000000000000000000000000005f5e100000000000000000000000000000000000000000000000000000000000000000000000000000000007bd909970b0eedcf078de6aeff23ce571663b8aa0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000024b40a817c0000000000000000000000000000000000000000000000000000000005f5e10000000000000000000000000000000000000000000000000000000000"; vm.createSelectFork("mainnet", 21724199); @@ -19,17 +24,21 @@ contract RegressionTest is Test { multisigTask.simulateRun(taskConfigFilePath); string memory callData = vm.toString(multisigTask.getCalldata()); + // assert that the call data generated in simulateRun is the same as the expected call data assertEq(keccak256(bytes(callData)), keccak256(bytes(expectedCallData))); + // data to sign generated by manually running the gas config template at block 21724199 on mainnet string memory expectedDataToSign = "0x19010f634ad56005ddbd68dc52233931a858f740b8ab706671c42b055efef561257e5ba28ec1e58ea69211eb8e875f10ae165fb3fb4052b15ca2516486f4b059135f"; string memory dataToSign = vm.toString(multisigTask.getDataToSign(multisigTask.parentMultisig(), multisigTask.getCalldata())); + // assert that the data to sign generated in simulateRun is the same as the expected data to sign assertEq(keccak256(bytes(dataToSign)), keccak256(bytes(expectedDataToSign))); } function testRegressionCallDataMatches_NestedMultisigDisputeGameUpgradeTemplate() public { string memory taskConfigFilePath = "test/tasks/mock/configs/NestedMultisigDisputeGameUpgradeTemplate.toml"; + // call data generated by manually running the dispute game upgrade template at block 21724199 on mainnet string memory expectedCallData = "0x174dea71000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000e5965ab5962edc7477c8520243a95517cd252fa9000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000004414f6b1a30000000000000000000000000000000000000000000000000000000000000000000000000000000000000000f691f8a6d908b58c534b624cf16495b491e633ba00000000000000000000000000000000000000000000000000000000"; vm.createSelectFork("mainnet", 21724199); @@ -37,8 +46,11 @@ contract RegressionTest is Test { multisigTask.simulateRun(taskConfigFilePath); string memory callData = vm.toString(multisigTask.getCalldata()); + // assert that the call data generated in simulateRun is the same as the expected call data assertEq(keccak256(bytes(callData)), keccak256(bytes(expectedCallData))); + // data to sign generated by manually running the dispute game upgrade template at block 21724199 on mainnet + // for each child multisig string[] memory expectedDataToSign = new string[](2); expectedDataToSign[0] = "0x1901a4a9c312badf3fcaa05eafe5dc9bee8bd9316c78ee8b0bebe3115bb21b732672032d168a6a75092d06448c977c02a33ee3890827ab9cc8a14a57e62494214746"; @@ -48,12 +60,15 @@ contract RegressionTest is Test { for (uint256 i = 0; i < owners.length; i++) { string memory dataToSign = vm.toString(multisigTask.getDataToSign(owners[i], multisigTask.generateApproveMulticallData())); + // assert that the data to sign generated for child multisig in simulateRun is the same + // as the expected data to sign assertEq(keccak256(bytes(dataToSign)), keccak256(bytes(expectedDataToSign[i]))); } } function testRegressionCallDataMatches_OPMainnetSetGameTypeTemplate() public { string memory taskConfigFilePath = "test/tasks/mock/configs/OPMainnetSetGameTypeTemplate.toml"; + // call data generated by manually running the set game type template at block 21724199 on mainnet string memory expectedCallData = "0x174dea71000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000c6901f65369fc59fc1b4d6d6be7a2318ff38db5b0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000044a1155ed9000000000000000000000000beb5fc579115071764c7423a4f12edde41f106ed000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000"; vm.createSelectFork("mainnet", 21724199); @@ -61,17 +76,21 @@ contract RegressionTest is Test { multisigTask.simulateRun(taskConfigFilePath); string memory callData = vm.toString(multisigTask.getCalldata()); + // assert that the call data generated in simulateRun is the same as the expected call data assertEq(keccak256(bytes(callData)), keccak256(bytes(expectedCallData))); + // data to sign generated by manually running the set game type template at block 21724199 string memory expectedDataToSign = "0x19014e6a6554de0308f5ece8ff736beed8a1b876d16f5c27cac8e466d7de0c70389084af4d0fecafda1f7bfcaf76684bbec959187b61160bdf1d1ab14045664fe412"; string memory dataToSign = vm.toString(multisigTask.getDataToSign(multisigTask.parentMultisig(), multisigTask.getCalldata())); + // assert that the data to sign generated in simulateRun is the same as the expected data to sign assertEq(keccak256(bytes(dataToSign)), keccak256(bytes(expectedDataToSign))); } function testRegressionCallDataMatches_OPMainnetGasConfigTemplate() public { string memory taskConfigFilePath = "test/tasks/mock/configs/OPMainnetGasConfigTemplate.toml"; + // call data generated by manually running the gas config template at block 21724199 on mainnet string memory expectedCallData = "0x174dea71000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000229047fed2591dbec1ef1118d64f7af3db9eb2900000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000024b40a817c000000000000000000000000000000000000000000000000000000000393870000000000000000000000000000000000000000000000000000000000"; vm.createSelectFork("mainnet", 21724199); @@ -79,12 +98,46 @@ contract RegressionTest is Test { multisigTask.simulateRun(taskConfigFilePath); string memory callData = vm.toString(multisigTask.getCalldata()); + // assert that the call data generated in simulateRun is the same as the expected call data assertEq(keccak256(bytes(callData)), keccak256(bytes(expectedCallData))); + // data to sign generated by manually running the gas config template at block 21724199 on mainnet string memory expectedDataToSign = "0x1901a4a9c312badf3fcaa05eafe5dc9bee8bd9316c78ee8b0bebe3115bb21b732672c98bc9c1761f2e403be0ad32b16d9c5fedf228f97eb0420c722b511129ebc803"; string memory dataToSign = vm.toString(multisigTask.getDataToSign(multisigTask.parentMultisig(), multisigTask.getCalldata())); + // assert that the data to sign generated in simulateRun is the same as the expected data to sign assertEq(keccak256(bytes(dataToSign)), keccak256(bytes(expectedDataToSign))); } + + function testRegressionCallDataMatches_OPCMUpgradeVxyz() public { + string memory taskConfigFilePath = "test/tasks/mock/configs/TestOPCMUpgradeVxyz.toml"; + // call data generated by manually running the TestOPCMUpgradeVxyz template at block 7733622 on sepolia + string memory expectedCallData = + "0x82ad56cb0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000200000000000000000000000005bc817c7c3f1a8dcaa01d229cbdeed9624c80e09000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000104ff2dd5a100000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000002000000000000000000000000034edd2a225f7f429a63e0f1d2084b9e0a93b538000000000000000000000000189abaaaa82dfc015a588a7dbad6f13b1d3485bc00000000000000000000000000000000000000000000000000000000000000400000000000000000000000005d63a8dc2737ce771aa4a6510d063b6ba2c4f6f2000000000000000000000000f7bc4b3a78c7dd8be9b69b3128eeb0d6776ce18a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"; + vm.createSelectFork("sepolia", 7733622); + MultisigTask multisigTask = new TestOPCMUpgradeVxyz(); + multisigTask.simulateRun(taskConfigFilePath); + + string memory callData = vm.toString(multisigTask.getCalldata()); + // assert that the call data generated in simulateRun is the same as the expected call data + assertEq(keccak256(bytes(callData)), keccak256(bytes(expectedCallData))); + + // data to sign generated by manually running the TestOPCMUpgradeVxyz template at block 7733622 on sepolia + // for each child multisig + string[] memory expectedDataToSign = new string[](2); + expectedDataToSign[0] = + "0x190137e1f5dd3b92a004a23589b741196c8a214629d4ea3a690ec8e41ae45c689cbbd6ce457eb94b23958530ed93f69e9c5178b74c3b709b6e5d7864623dc4998d1f"; + expectedDataToSign[1] = + "0x1901be081970e9fc104bd1ea27e375cd21ec7bb1eec56bfe43347c3e36c5d27b853345c1d45ffe973f7f85040ac9a1fa97f33862f03406b005218766234ac38e8f10"; + + address[] memory owners = IGnosisSafe(multisigTask.parentMultisig()).getOwners(); + for (uint256 i = 0; i < owners.length; i++) { + string memory dataToSign = + vm.toString(multisigTask.getDataToSign(owners[i], multisigTask.generateApproveMulticallData())); + // assert that the data to sign generated for child multisig in simulateRun is the same + // as the expected data to sign + assertEq(keccak256(bytes(dataToSign)), keccak256(bytes(expectedDataToSign[i]))); + } + } } diff --git a/test/tasks/SingleMultisigTask.t.sol b/test/tasks/SingleMultisigTask.t.sol index ad7255aca..a56dbcfe9 100644 --- a/test/tasks/SingleMultisigTask.t.sol +++ b/test/tasks/SingleMultisigTask.t.sol @@ -1,18 +1,20 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; +import {IGnosisSafe, Enum} from "@base-contracts/script/universal/IGnosisSafe.sol"; +import {SystemConfig} from "@eth-optimism-bedrock/src/L1/SystemConfig.sol"; +import {IMulticall3} from "forge-std/interfaces/IMulticall3.sol"; +import {Signatures} from "@base-contracts/script/universal/Signatures.sol"; +import {LibSort} from "@solady/utils/LibSort.sol"; import {Test} from "forge-std/Test.sol"; -import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; +import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; import {GasConfigTemplate} from "src/improvements/template/GasConfigTemplate.sol"; +import {MockDisputeGameTask} from "test/tasks/mock/MockDisputeGameTask.sol"; +import {DisputeGameUpgradeTemplate} from "src/improvements/template/DisputeGameUpgradeTemplate.sol"; import {IncorrectGasConfigTemplate1} from "test/tasks/mock/template/IncorrectGasConfigTemplate1.sol"; import {IncorrectGasConfigTemplate2} from "test/tasks/mock/template/IncorrectGasConfigTemplate2.sol"; -import {IMulticall3} from "forge-std/interfaces/IMulticall3.sol"; -import {IGnosisSafe, Enum} from "@base-contracts/script/universal/IGnosisSafe.sol"; -import {LibSort} from "@solady/utils/LibSort.sol"; -import {Signatures} from "@base-contracts/script/universal/Signatures.sol"; -import {SystemConfig} from "@eth-optimism-bedrock/src/L1/SystemConfig.sol"; contract SingleMultisigTaskTest is Test { struct MultiSigOwner { @@ -218,14 +220,48 @@ contract SingleMultisigTaskTest is Test { bytes memory expectedRevertMessage = bytes( string.concat( "MultisigTask: address ", - localMultisigTask.getAddressLabel(addressRegistry.getAddress("SystemConfigOwner", 34443)), - " not in task state change addresses" + localMultisigTask.getAddressLabel(addressRegistry.getAddress("SystemConfigProxy", 34443)), + " not in allowed storage accesses" ) ); vm.expectRevert(expectedRevertMessage); localMultisigTask.simulateRun(taskConfigFilePath); } + function testMockDisputeGameWithCodeExceptionsWorks() public { + vm.createSelectFork("mainnet"); + string memory opcmTaskConfigFilePath = "test/tasks/mock/configs/MockDisputeGameUpgradesToEOA.toml"; + multisigTask = new MockDisputeGameTask(); + + multisigTask.simulateRun(opcmTaskConfigFilePath); + } + + function testSimulateRunDisputeGameWithoutCodeExceptionsFails() public { + vm.createSelectFork("mainnet"); + string memory opcmTaskConfigFilePath = "test/tasks/mock/configs/MockDisputeGameUpgradesToEOA.toml"; + multisigTask = new DisputeGameUpgradeTemplate(); + + uint256 start = vm.snapshot(); + + multisigTask.simulateRun("src/improvements/tasks/example/eth/001-initial-example-dg-upgrade/config.toml"); + addrRegistry = multisigTask.addrRegistry(); + address account = addrRegistry.getAddress("DisputeGameFactoryProxy", getChain("optimism").chainId); + + vm.revertTo(start); + + string memory err = string.concat( + "Likely address in storage has no code\n", + " account: ", + vm.toString(account), + "\n slot: ", + vm.toString(bytes32(0xffdfc1249c027f9191656349feb0761381bb32c9f557e01f419fd08754bf5a1b)), + "\n value: ", + vm.toString(bytes32(0x0000000000000000000000000000000fffffffffffffffffffffffffffffffff)) + ); + vm.expectRevert(bytes(err)); + multisigTask.simulateRun(opcmTaskConfigFilePath); + } + function testExecuteWithSignatures() public { uint256 snapshotId = vm.snapshot(); runTask(); @@ -299,7 +335,7 @@ contract SingleMultisigTaskTest is Test { // execute the task with the signatures multisigTask = new GasConfigTemplate(); - multisigTask.executeRun(taskConfigFilePath, packedSignatures); + multisigTask.simulateRun(taskConfigFilePath, packedSignatures); // check that the gas limits are set correctly after the task is executed SystemConfig systemConfig = SystemConfig(systemConfigMode); diff --git a/test/tasks/mock/MockDisputeGameTask.sol b/test/tasks/mock/MockDisputeGameTask.sol new file mode 100644 index 000000000..3e443333c --- /dev/null +++ b/test/tasks/mock/MockDisputeGameTask.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity 0.8.15; + +import {IProxyAdmin} from "@eth-optimism-bedrock/interfaces/universal/IProxyAdmin.sol"; +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 {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; +import {DisputeGameUpgradeTemplate} from "src/improvements/template/DisputeGameUpgradeTemplate.sol"; + +/// Mock task that upgrades the Dispute Game implementation +/// to an example implementation address without code +contract MockDisputeGameTask is DisputeGameUpgradeTemplate { + /// @notice code exceptions for this template is address 0x0000000FFfFFfffFffFfFffFFFfffffFffFFffFf + function getCodeExceptions() internal view virtual override returns (address[] memory) { + address[] memory addresses = new address[](1); + addresses[0] = address(0x0000000FFfFFfffFffFfFffFFFfffffFffFFffFf); + + return addresses; + } +} diff --git a/test/tasks/mock/MockMultisigTask.sol b/test/tasks/mock/MockMultisigTask.sol index 41651fc1e..b2a0b091f 100644 --- a/test/tasks/mock/MockMultisigTask.sol +++ b/test/tasks/mock/MockMultisigTask.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.15; import {IProxyAdmin} from "@eth-optimism-bedrock/interfaces/universal/IProxyAdmin.sol"; 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 {MockTarget} from "test/tasks/mock/MockTarget.sol"; import {MultisigTask} from "src/improvements/tasks/MultisigTask.sol"; @@ -26,8 +27,9 @@ contract MockMultisigTask is MultisigTask { /// @notice Returns the storage write permissions required for this task /// @return Array of storage write permissions function _taskStorageWrites() internal pure override returns (string[] memory) { - string[] memory storageWrites = new string[](1); + string[] memory storageWrites = new string[](2); storageWrites[0] = "L1ERC721BridgeProxy"; + storageWrites[1] = "ProxyAdminOwner"; return storageWrites; } @@ -63,4 +65,7 @@ contract MockMultisigTask is MultisigTask { ) public { actions.push(Action(target, value, data, operation, description)); } + + /// @notice no code exceptions for this template + function getCodeExceptions() internal view virtual override returns (address[] memory) {} } diff --git a/test/tasks/mock/configs/MockDisputeGameUpgradesToEOA.toml b/test/tasks/mock/configs/MockDisputeGameUpgradesToEOA.toml new file mode 100644 index 000000000..d9727888a --- /dev/null +++ b/test/tasks/mock/configs/MockDisputeGameUpgradesToEOA.toml @@ -0,0 +1,7 @@ +# this is the file used to determine the network configuration + +l2chains = [{name = "OP Mainnet", chainId = 10}] + +templateName = "DisputeGameUpgradeTemplate" + +implementations = [{gameType = 0, implementation = "0x0000000FFfFFfffFffFfFffFFFfffffFffFFffFf", l2ChainId = 10}] diff --git a/test/tasks/mock/template/IncorrectGasConfigTemplate1.sol b/test/tasks/mock/template/IncorrectGasConfigTemplate1.sol index a8dfbd095..482251ab7 100644 --- a/test/tasks/mock/template/IncorrectGasConfigTemplate1.sol +++ b/test/tasks/mock/template/IncorrectGasConfigTemplate1.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {SystemConfig} from "@eth-optimism-bedrock/src/L1/SystemConfig.sol"; diff --git a/test/tasks/mock/template/IncorrectGasConfigTemplate2.sol b/test/tasks/mock/template/IncorrectGasConfigTemplate2.sol index 49f3f892a..1a92fa4a8 100644 --- a/test/tasks/mock/template/IncorrectGasConfigTemplate2.sol +++ b/test/tasks/mock/template/IncorrectGasConfigTemplate2.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import {GasConfigTemplate} from "src/improvements/template/GasConfigTemplate.sol"; @@ -7,10 +8,9 @@ import {AddressRegistry} from "src/improvements/AddressRegistry.sol"; /// @notice not all allowed storages writes are written to contract IncorrectGasConfigTemplate2 is GasConfigTemplate { function _taskStorageWrites() internal pure override returns (string[] memory) { - string[] memory storageWrites = new string[](2); + string[] memory storageWrites = new string[](1); /// expected to be written to storageWrites[0] = "SystemConfigOwner"; - storageWrites[1] = "SystemConfigProxy"; return storageWrites; } }