Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/improvements/tasks/MultisigTask.sol
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,9 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage
returns (TaskPayload memory payload_, Action[] memory actions_)
{
require(_preExecutionSnapshot == 0, "MultisigTask: already initialized");
templateConfig.safeAddressString = loadSafeAddressString(MultisigTask(address(this)), _taskConfigFilePath);
// Commented out - address(this) not allowed in scripts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not following why these changes are needed in MultisigTask. Can you explain them to me some more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping address(this) in MultisigTask is causing this error for me:

Error: script failed: Usage of address(this) detected in script contract. Script contracts are ephemeral and their addresses should not be relied upon.
error: Recipe simulate failed with exit code 1

it seems like keeping address(this) doesnt work in scripts and we use it in two places. one is for getting the safeAddressString, which i changed to just calling safeAddressString() and the other uses address(this) to do debugging w vm.label.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to make sure you're using mise to install the correct version of foundry. Can you follow the installation steps in the readme and try again? This change is not necessary.

// templateConfig.safeAddressString = loadSafeAddressString(MultisigTask(address(this)), _taskConfigFilePath);
templateConfig.safeAddressString = safeAddressString();
IGnosisSafe _root;
(addrRegistry, _root, multicallTarget) = _configureTask(_taskConfigFilePath);

Expand All @@ -627,7 +629,7 @@ abstract contract MultisigTask is Test, Script, StateOverrideManager, TaskManage
_setAllowedBalanceChanges();

vm.label(AddressRegistry.unwrap(addrRegistry), "AddrRegistry");
vm.label(address(this), "MultisigTask");
// vm.label(address(this), "MultisigTask"); // Commented out - address(this) not allowed in scripts

actions_ = build(address(_root));
bytes[] memory allCalldatas = transactionDatas(actions_, allSafes, allOriginalNonces);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
TENDERLY_GAS=10000000
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# 030-worldchain-l2pao-key-handback-over

Status: [DRAFT, NOT READY TO SIGN]()

## Objective

Transfer the L2 ProxyAdmin Owner for Worldchain Sepolia to Alchemy-controlled EOA.

## Simulation & Signing

Simulation commands for each safe:
```bash
cd src/improvements/tasks/sep/030-worldchain-l2pao-key-handback-over
SIMULATE_WITHOUT_LEDGER=1 just --dotenv-path $(pwd)/.env simulate
```

Signing commands for each safe:
```bash
cd src/improvements/tasks/sep/030-worldchain-l2pao-key-handback-over
just --dotenv-path $(pwd)/.env sign
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# Validation

This document can be used to validate the inputs and result of the execution of the upgrade transaction which you are signing.

The steps are:
1. [Validate the Domain and Message Hashes](#expected-domain-and-message-hashes)
2. [Verifying the state changes via the normalized state diff hash](#normalized-state-diff-hash-attestation)
3. [Verifying the transaction input](#understanding-task-calldata)
4. [Verifying the state changes](#task-state-changes)

## Expected Domain and Message Hashes

First, we need to validate the domain and message hashes. These values should match both the values on your ledger and the values printed to the terminal when you run the task.

> [!CAUTION]
>
> Before signing, ensure the below hashes match what is on your ledger.
>
> ### Worldchain Sepolia Proxy Admin Owner (`0x945185C01fb641bA3E63a9bdF66575e35a407837`)
>
> - Domain Hash: `0x6faec9c52949ba8274340008df12c69faedd5c44e77f77c956d2ca8e4bcd877e`
> - Message Hash: `0x3e35ccc62b450d604653d0a0054e13afd3f6f76c4c1e6cc70bc40e8838ed7bc7`

## Normalized State Diff Hash Attestation

The normalized state diff hash **MUST** match the hash produced by the state changes attested to in the state diff audit report. As a signer, you are responsible for verifying that this hash is correct. Please compare the hash below with the one in the audit report. If no audit report is available for this task, you must still ensure that the normalized state diff hash matches the output in your terminal.

**Normalized hash:** `0x569e75fc77c1a856f6daaf9e69d8a9566ca34aa47f9133711ce065a571af0cfd`

## Understanding Task Calldata

The transaction initiates a deposit transaction via the OptimismPortal on L1 Sepolia, which will be executed on L2 (Worldchain Sepolia) to transfer the L2 ProxyAdmin ownership to an EOA.

### Decoding the depositTransaction call:
```bash
# The outer multicall to OptimismPortal
cast calldata-decode "depositTransaction(address,uint256,uint64,bool,bytes)" \
0xe9e05c42000000000000000000000000420000000000000000000000000000000000001800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030d40000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000024f2fde38b000000000000000000000000e78a0a96c5d6ae6c606418ed4a9ced378cb030a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
```

Returns:
- `_to`: `0x4200000000000000000000000000000000000018` (L2 ProxyAdmin predeploy)
- `_value`: `0` (no ETH sent)
- `_gasLimit`: `200000` (gas for L2 execution)
- `_isCreation`: `false` (not a contract creation)
- `_data`: `0xf2fde38b000000000000000000000000e78a0a96c5d6ae6c606418ed4a9ced378cb030a0`

### Decoding the inner transferOwnership call:
```bash
cast calldata-decode "transferOwnership(address)" \
0xf2fde38b000000000000000000000000e78a0a96c5d6ae6c606418ed4a9ced378cb030a0
```

Returns:
- `newOwnerEOA`: `0xe78a0A96C5D6aE6C606418ED4A9Ced378cb030A0` (the target EOA)

# State Validations

For each contract listed in the state diff, please verify that no contracts or state changes shown in the Tenderly diff are missing from this document. Additionally, please verify that for each contract:

- The following state changes (and none others) are made to that contract. This validates that no unexpected state changes occur.
- All addresses (in section headers and storage values) match the provided name, using the Etherscan and Superchain Registry links provided. This validates the bytecode deployed at the addresses contains the correct logic.
- All key values match the semantic meaning provided, which can be validated using the storage layout links provided.

### State Overrides

Note: The changes listed below do not include threshold, nonce and owner mapping overrides. These changes are listed and explained in the [SINGLE-VALIDATION.md](../../../../../SINGLE-VALIDATION.md) file.

### Task State Changes

---

### `0x945185c01fb641ba3e63a9bdf66575e35a407837` ([Worldchain Sepolia ProxyAdminOwner](https://github.com/ethereum-optimism/superchain-registry/blob/1ff0df40c7602761c55ab2cb693614ca0382bd64/superchain/configs/sepolia/worldchain.toml#L44)) - Chain ID: 11155111

- **Key:** `0x0000000000000000000000000000000000000000000000000000000000000005`
- **Decoded Kind:** `uint256`
- **Before:** `0x0000000000000000000000000000000000000000000000000000000000000030`
- **After:** `0x0000000000000000000000000000000000000000000000000000000000000031`
- **Summary:** Safe nonce increment
- **Detail:** The nonce is incremented by 1 as the safe executes the transaction to initiate the L2 ProxyAdmin ownership transfer.

---

### `0xff6eba109271fe6d4237eeed4bab1dd9a77dd1a4` ([OptimismPortal](https://sepolia.etherscan.io/address/0xff6eba109271fe6d4237eeed4bab1dd9a77dd1a4)) - Chain ID: 11155111

- **Key:** `0x0000000000000000000000000000000000000000000000000000000000000001`
- **Decoded Kind:** `struct ResourceMetering.ResourceParams`
- **Before:** `0x00000000008cc5de0000000000090a010000000000000000000000003b9aca00`
- **After:** `0x00000000008cc78a0000000000030d400000000000000000000000003b9aca00`
- **Summary:** Resource metering params update
- **Detail:** The OptimismPortal's resource metering parameters are updated as part of processing the deposit transaction. This is an expected side effect of calling `depositTransaction`.

## Manual L2 Verification Steps

After the L1 transaction is executed, you must verify that the L2 deposit transaction successfully transfers ownership:

1. **Find the L2 deposit transaction**: Look for a transaction on Worldchain Sepolia from the L1 caller to the L2 ProxyAdmin at `0x4200000000000000000000000000000000000018`.

2. **Verify the OwnershipTransferred event**: Confirm that the event shows:
- `previousOwner`: `0x2FC3ffc903729a0f03966b917003800B145F67F3` (aliased 2/2 safe)
- `newOwnerEOA`: `0xe78a0A96C5D6aE6C606418ED4A9Ced378cb030A0` (target EOA)

3. **Verify final state**: Call `owner()` on the L2 ProxyAdmin to confirm it returns `0xe78a0A96C5D6aE6C606418ED4A9Ced378cb030A0`.

```bash
# After L2 execution, verify the new owner
cast call 0x4200000000000000000000000000000000000018 "owner()(address)" --rpc-url worldchain-sepolia
# Should return: 0xe78a0A96C5D6aE6C606418ED4A9Ced378cb030A0
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
l2chains = [{name = "Worldchain Sepolia", chainId = 4801}]
templateName = "TransferL2PAOFromL1ToEOA"

# The new owner address (EOA). See here https://www.notion.so/oplabs/Worldchain-key-handback-over-address-validation-272f153ee1628002bfa2e00a718c57d5?source=copy_link
newOwnerEOA = "0xe78a0A96C5D6aE6C606418ED4A9Ced378cb030A0"
128 changes: 128 additions & 0 deletions src/improvements/template/TransferL2PAOFromL1ToEOA.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import {VmSafe} from "forge-std/Vm.sol";
import {stdToml} from "forge-std/StdToml.sol";
import {Predeploys} from "@eth-optimism-bedrock/src/libraries/Predeploys.sol";

import {L2TaskBase} from "src/improvements/tasks/types/L2TaskBase.sol";
import {SuperchainAddressRegistry} from "src/improvements/SuperchainAddressRegistry.sol";
import {Action} from "src/libraries/MultisigTypes.sol";

/// @notice Template contract to transfer ownership of the L2 ProxyAdmin to an EOA address.
/// This template is used when the L2 ProxyAdmin is currently held by a multisig (e.g., 2/2 safe)
/// and needs to be transferred to an EOA. The transfer is executed via L1 using the OptimismPortal
/// deposit transaction mechanism.
/// See: https://docs.optimism.io/stack/transactions/deposit-flow
///
/// ATTENTION: Use caution when using this template — transferring ownership is high risk.
/// To gain additional assurance that the corresponding L2 deposit transaction works as expected,
/// you must follow the steps outlined in the documentation: ../doc/simulate-l2-ownership-transfer.md
/// Add the results of the simulation to the VALIDATION.md file for the task.
///
/// Manual Post-Execution checks to follow when executing this task:
/// 1. Find the L2 deposit transaction sent from the L1 caller.
/// 2. The transaction should be interacting with the L2 ProxyAdmin at 0x4200000000000000000000000000000000000018.
/// 3. Verify that the OwnershipTransferred event was emitted with the correct new EOA owner.
contract TransferL2PAOFromL1ToEOA is L2TaskBase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract TransferL2PAOFromL1ToEOA violates the Solidity style guide by not using triple-slash natspec comment style. The contract has a multi-line comment block using /// but should use the proper natspec format with @notice tags. According to the Solidity Guide, comments should use triple-slash solidity natspec comment style and always use @notice instead of @dev. The contract documentation should be reformatted to use proper natspec tags like /// @notice for the contract description.

Suggested change
contract TransferL2PAOFromL1ToEOA is L2TaskBase {
/// @notice A contract that handles transferring L2 PAO tokens from L1 to an EOA (Externally Owned Account)
contract TransferL2PAOFromL1ToEOA is L2TaskBase {

Spotted by Diamond (based on custom rule: Custom rules)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

using stdToml for string;

/// @notice The new owner EOA address.
address public newOwnerEOA;

/// @notice Returns the safe address string identifier
function safeAddressString() public pure override returns (string memory) {
return "ProxyAdminOwner";
}

/// @notice Returns the storage write permissions required for this task.
function _taskStorageWrites() internal pure virtual override returns (string[] memory) {
string[] memory storageWrites = new string[](1);
storageWrites[0] = "OptimismPortalProxy";
return storageWrites;
}

/// @notice Sets up the template with the new EOA owner from a TOML file.
function _templateSetup(string memory taskConfigFilePath, address rootSafe) internal override {
super._templateSetup(taskConfigFilePath, rootSafe);
string memory toml = vm.readFile(taskConfigFilePath);

// New owner EOA address.
newOwnerEOA = abi.decode(vm.parseToml(toml, ".newOwnerEOA"), (address));

// Only allow one chain to be modified at a time with this template.
SuperchainAddressRegistry.ChainInfo[] memory _chains = superchainAddrRegistry.getChains();
require(_chains.length == 1, "Must specify exactly one chain id to transfer ownership for");
}

/// @notice Builds the actions for transferring ownership of the proxy admin on the L2 to an EOA.
/// It does this by calling the L1 OptimismPortal's depositTransaction function.
function _build(address) internal override {
SuperchainAddressRegistry.ChainInfo[] memory chains = superchainAddrRegistry.getChains();

// See this Tenderly simulation for an example of this gas limit working: https://www.tdly.co/shared/simulation/d5028138-469c-4bb2-97fd-50f5f4bb8515
uint64 gasLimit = 200000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded gas limit of 200000 reduces robustness and safety. Consider making this value configurable through the config file or adding validation to ensure the gas limit is sufficient for the specific L2 operation. While a Tenderly simulation is referenced in the comments, the template would be more robust with either runtime validation of gas sufficiency or configuration-based flexibility to handle different scenarios.

Suggested change
uint64 gasLimit = 200000;
uint64 gasLimit = config.getL2TransferGasLimit(); // Read from config instead of hardcoding

Spotted by Diamond (based on custom rule: Superchain Ops task/template review)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

OptimismPortal optimismPortal =
OptimismPortal(superchainAddrRegistry.getAddress("OptimismPortalProxy", chains[0].chainId));
optimismPortal.depositTransaction(
address(Predeploys.PROXY_ADMIN),
0,
gasLimit,
false,
abi.encodeCall(ProxyAdmin.transferOwnership, (newOwnerEOA))
);
}

/// @notice Validates that the owner was transferred correctly.
function _validate(VmSafe.AccountAccess[] memory, Action[] memory actions, address) internal view override {
// Validate that the depositTransaction action was created correctly
SuperchainAddressRegistry.ChainInfo[] memory chains = superchainAddrRegistry.getChains();
address expectedPortal = superchainAddrRegistry.getAddress("OptimismPortalProxy", chains[0].chainId);

// Expected calldata for depositTransaction
uint64 gasLimit = 200000;
bytes memory expectedCalldata = abi.encodeCall(
OptimismPortal.depositTransaction,
(
address(Predeploys.PROXY_ADMIN), // _to
0, // _value
gasLimit, // _gasLimit
false, // _isCreation
abi.encodeCall(ProxyAdmin.transferOwnership, (newOwnerEOA)) // _data
)
);

// Check that we have exactly one action to the OptimismPortal with the expected calldata
bool found = false;
uint256 matches = 0;
for (uint256 i = 0; i < actions.length; i++) {
if (actions[i].target == expectedPortal) {
if (keccak256(actions[i].arguments) == keccak256(expectedCalldata)) {
found = true;
matches++;
}
assertEq(actions[i].value, 0, "Should not send ETH with depositTransaction");
}
}

assertTrue(found, "depositTransaction action not found");
assertEq(matches, 1, "Should have exactly one depositTransaction action");

// Note: We can't validate the L2 state change since it only happens after L1 execution
// Manual verification steps are documented in the contract comments above
}

/// @notice No code exceptions for this template.
function _getCodeExceptions() internal view virtual override returns (address[] memory) {}
}

interface OptimismPortal {
function depositTransaction(address _to, uint256 _value, uint64 _gasLimit, bool _isCreation, bytes memory _data)
external
payable;
}

interface ProxyAdmin {
function owner() external view returns (address);
function transferOwnership(address newOwner) external;
}
Comment on lines +119 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OptimismPortal and ProxyAdmin interfaces are missing natspec documentation. According to the Solidity style guide, all interfaces should use triple-slash natspec comment style with @notice tags to document their purpose and functions. Add proper natspec documentation above each interface and their functions to explain what they do.

Suggested change
interface OptimismPortal {
function depositTransaction(address _to, uint256 _value, uint64 _gasLimit, bool _isCreation, bytes memory _data)
external
payable;
}
interface ProxyAdmin {
function owner() external view returns (address);
function transferOwnership(address newOwner) external;
}
/// @notice Interface for interacting with the Optimism Portal, which handles cross-chain transactions from L1 to L2
interface OptimismPortal {
/// @notice Deposits a transaction from L1 to L2
/// @param _to The address to send the transaction to on L2
/// @param _value The amount of ETH to send with the transaction
/// @param _gasLimit The gas limit for the L2 transaction
/// @param _isCreation Whether the transaction is a contract creation
/// @param _data The calldata for the transaction
function depositTransaction(address _to, uint256 _value, uint64 _gasLimit, bool _isCreation, bytes memory _data)
external
payable;
}
/// @notice Interface for interacting with a ProxyAdmin contract that manages proxy contracts
interface ProxyAdmin {
/// @notice Returns the current owner of the ProxyAdmin
/// @return The address of the current owner
function owner() external view returns (address);
/// @notice Transfers ownership of the ProxyAdmin to a new address
/// @param newOwner The address of the new owner
function transferOwnership(address newOwner) external;
}

Spotted by Diamond (based on custom rule: Custom rules)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

28 changes: 28 additions & 0 deletions test/tasks/Regression.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {MultisigTaskTestHelper} from "test/tasks/MultisigTask.t.sol";
import {DelayedWETHOwnershipTemplate} from "src/improvements/template/DelayedWETHOwnershipTemplate.sol";
import {TransferOwners} from "src/improvements/template/TransferOwners.sol";
import {TransferL2PAOFromL1} from "src/improvements/template/TransferL2PAOFromL1.sol";
import {TransferL2PAOFromL1ToEOA} from "src/improvements/template/TransferL2PAOFromL1ToEOA.sol";
import {DisableModule} from "src/improvements/template/DisableModule.sol";
import {Action} from "src/libraries/MultisigTypes.sol";
import {GnosisSafeApproveHash} from "src/improvements/template/GnosisSafeApproveHash.sol";
Expand Down Expand Up @@ -560,6 +561,33 @@ contract RegressionTest is Test {
_assertDataToSignNestedMultisig(multisigTask, actions, expectedDataToSign, MULTICALL3_ADDRESS, currentRootSafe);
}

/// @notice Expected call data and data to sign generated by manually running the TransferL2PAOFromL1ToEOA template
/// Simulate from task directory (test/tasks/example/sep/026-transfer-l2pao-to-eoa) with:
/// SIMULATE_WITHOUT_LEDGER=1 just --dotenv-path "$(pwd)/.env" --justfile ../../../../../src/improvements/justfile simulate
function testRegressionCallDataMatches_TransferL2PAOFromL1ToEOA() public {
string memory taskConfigFilePath = "test/tasks/example/sep/026-transfer-l2pao-to-eoa/config.toml";
// Call data generated by manually running the TransferL2PAOFromL1ToEOA template on sepolia
string memory expectedCallData =
"0x174dea71000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000ff6eba109271fe6d4237eeed4bab1dd9a77dd1a40000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000104e9e05c42000000000000000000000000420000000000000000000000000000000000001800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030d40000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000024f2fde38b000000000000000000000000e78a0a96c5d6ae6c606418ed4a9ced378cb030a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000";
MultisigTask multisigTask = new TransferL2PAOFromL1ToEOA();
// Using the Worldchain sepolia challenger safe from the actual simulation
address rootSafe = address(0x945185C01fb641bA3E63a9bdF66575e35a407837);
address[] memory allSafes = new address[](1);
allSafes[0] = rootSafe;
(Action[] memory actions, uint256[] memory allOriginalNonces) =
_setupAndSimulate(taskConfigFilePath, 9000000, "sepolia", multisigTask, allSafes);
bytes memory rootSafeCalldata =
_assertCallDataMatches(multisigTask, actions, allSafes, allOriginalNonces, expectedCallData);

// Data to sign generated by manually running the TransferL2PAOFromL1ToEOA template on sepolia
uint256 rootSafeNonce = allOriginalNonces[allOriginalNonces.length - 1];
string memory expectedDataToSign =
"0x19016faec9c52949ba8274340008df12c69faedd5c44e77f77c956d2ca8e4bcd877ece8dfc106d22fd21fc65bfa3fe41c4943eb0f02cfb831d03cd4a15934b5fe163";
_assertDataToSignSingleMultisig(
rootSafe, rootSafeCalldata, expectedDataToSign, rootSafeNonce, MULTICALL3_ADDRESS
);
}

/// @notice Expected call data and data to sign generated by manually running the GnosisSafeApproveHash template at block 8384642 on sepolia
/// Simulate from task directory (test/tasks/example/sep/013-gnosis-safe-approve-hash) with:
/// SIMULATE_WITHOUT_LEDGER=1 just --dotenv-path $(pwd)/.env --justfile ../../../../../src/improvements/nested.just simulate base
Expand Down
2 changes: 2 additions & 0 deletions test/tasks/example/sep/026-transfer-l2pao-to-eoa/.env
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
TENDERLY_GAS=25000000
FORK_BLOCK_NUMBER=9000000
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Test configuration for TransferL2PAOFromL1ToEOA template
l2chains = [{name = "Worldchain Sepolia", chainId = 4801}]

templateName = "TransferL2PAOFromL1ToEOA"

# The new EOA owner address (from the actual simulation)
newOwnerEOA = "0xe78a0a96c5d6ae6c606418ed4a9ced378cb030a0"