Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add template to set a dispute game implementation #354

Closed
wants to merge 4 commits into from
Closed
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: 6 additions & 0 deletions tasks/sep/fp-recovery/005-set-game-implementation/.env
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
ETH_RPC_URL="https://ethereum-sepolia.publicnode.com"
COUNCIL_SAFE=0xf64bc17485f0B4Ea5F06A96514182FC4cB561977
FOUNDATION_SAFE=0xDEe57160aAfCF04c34C887B5962D0a69676d3C8B
SAFE_NONCE=""
L1_CHAIN_NAME="sepolia"
L2_CHAIN_NAME="op"
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;

import {NestedSignFromJson as OriginalNestedSignFromJson} from "script/NestedSignFromJson.s.sol";
import {console2 as console} from "forge-std/console2.sol";
import {stdJson} from "forge-std/StdJson.sol";
import {stdToml} from "forge-std/StdToml.sol";
import {Vm, VmSafe} from "forge-std/Vm.sol";
import {GnosisSafe} from "safe-contracts/GnosisSafe.sol";
import {LibString} from "solady/utils/LibString.sol";
import {Types} from "@eth-optimism-bedrock/scripts/Types.sol";
import "@eth-optimism-bedrock/src/dispute/lib/Types.sol";
import {DisputeGameFactory} from "@eth-optimism-bedrock/src/dispute/DisputeGameFactory.sol";
import {FaultDisputeGame} from "@eth-optimism-bedrock/src/dispute/FaultDisputeGame.sol";
import {PermissionedDisputeGame} from "@eth-optimism-bedrock/src/dispute/PermissionedDisputeGame.sol";

contract NestedSignFromJson is OriginalNestedSignFromJson {
using LibString for string;

// Chains for this task.
string l1ChainName = vm.envString("L1_CHAIN_NAME");
string l2ChainName = vm.envString("L2_CHAIN_NAME");

// Safe contract for this task.
GnosisSafe ownerSafe = GnosisSafe(payable(vm.envAddress("OWNER_SAFE")));
address livenessGuard = 0xc26977310bC89DAee5823C2e2a73195E85382cC7;
Copy link
Contributor

Choose a reason for hiding this comment

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

Safe's store the guard address in slot 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8 (source), so we can read the guard address from the Safe instead of hardcoding it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea how to do that from within a solidity script. The getGuard method would be ideal but it's internal only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I wish the guard method was public. Form solidity this can be done with vm.load(address target, bytes32 slot): https://github.com/foundry-rs/forge-std/blob/1eea5bae12ae557d589f9f0f0edae2faa47cb262/src/Vm.sol#L588-L589


// Known EOAs to exclude from safety checks.
address systemConfigOwner; // In registry addresses.
address batchSenderAddress; // In registry genesis-system-configs
address p2pSequencerAddress; // cast call $SystemConfig "unsafeBlockSigner()(address)"
address batchInboxAddress; // In registry yaml.
Comment on lines +28 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect the slots holding these are never touched and we can delete all these

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed!


Types.ContractSet proxies;

// Current dispute game implementation
FaultDisputeGame currentImpl;

// New dispute game implementation
FaultDisputeGame faultDisputeGame;

// Game type to set
GameType targetGameType;

// DisputeGameFactoryProxy address. Loaded from superchain-registry
DisputeGameFactory dgfProxy;

function setUp() public {
proxies = _getContractSet();
// Read the DisputeGameFactoryProxy and new dispute game implementation from the input JSON.
string memory inputJson;
string memory path = "/tasks/sep/fp-recovery/005-set-game-implementation/input.json";
try vm.readFile(string.concat(vm.projectRoot(), path)) returns (string memory data) {
inputJson = data;
} catch {
revert(string.concat("Failed to read ", path));
}

dgfProxy = DisputeGameFactory(stdJson.readAddress(inputJson, "$.transactions[0].to"));
targetGameType = GameType.wrap(uint32(stdJson.readUint(inputJson, "$.transactions[0].contractInputsValues._gameType")));
faultDisputeGame = FaultDisputeGame(stdJson.readAddress(inputJson, "$.transactions[0].contractInputsValues._impl"));
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Should we rename the faultDisputeGame to TargetFaultDisputeGame to make it clear that it is a target contract?

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Should we rename the faultDisputeGame to TargetFaultDisputeGame to make it clear that it is a target new impl?

currentImpl = FaultDisputeGame(address(dgfProxy.gameImpls(GameType(targetGameType))));

_precheckDisputeGameImplementation();
}

function getCodeExceptions() internal view override returns (address[] memory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering of the relevance of this method getCodeException.
@mds1 do you think this is usually necessary? Because for me, the getAllowedStorageAccess is enough to check if the storage access is modified. Even if the function getCodeException is not specifically for storage access, I think this is adding overhead to the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The historical context behind this is https://www.notion.so/oplabs/PM-46-Testnet-Contract-Address-in-Mainnet-MCP-L1-Upgrade-Playbook-4ba491167af24959be654d66bbe78d8c?pvs=4

The summary is that we had the Sepolia SuperchainConfig address being written to storage in a mainnet playbook. getCodeExceptions was our solution—most addresses we write to storage are addresses of other contracts in the system, so if an EOA is in storage that is a signal that something might be wrong. However there are a few addresses that we do see in storage with no code, so getCodeExceptions lets us allowlist those

// Owners of the nested safes will appear in storage in the LivenessGuard when added, and they are allowed
// to have code AND to have no code.
address[] memory nestedSafes = ownerSafe.getOwners();
// First count the total owners from the nested safes
uint256 totalNumberOfSigners;
for (uint256 a = 0; a < nestedSafes.length; a++) {
GnosisSafe safe = GnosisSafe(payable(nestedSafes[a]));
totalNumberOfSigners += safe.getOwners().length;
}
address[] memory safeOwners = new address[](totalNumberOfSigners);
uint256 addedSigners;
for (uint256 a = 0; a < nestedSafes.length; a++) {
GnosisSafe safe = GnosisSafe(payable(nestedSafes[a]));
address[] memory nestedSafeOwners = safe.getOwners();
for (uint256 i = 0; i < nestedSafeOwners.length; i++) {
safeOwners[addedSigners] = nestedSafeOwners[i];
addedSigners++;
}
}

// To make sure we probably handle all signers whether or not they have code, first we count
// the number of signers that have no code.
uint256 numberOfSafeSignersWithNoCode;
for (uint256 i = 0; i < safeOwners.length; i++) {
if (safeOwners[i].code.length == 0) {
numberOfSafeSignersWithNoCode++;
}
}

// Then we extract those EOA addresses into a dedicated array.
uint256 trackedSignersWithNoCode;
address[] memory safeSignersWithNoCode = new address[](numberOfSafeSignersWithNoCode);
for (uint256 i = 0; i < safeOwners.length; i++) {
if (safeOwners[i].code.length == 0) {
safeSignersWithNoCode[trackedSignersWithNoCode] = safeOwners[i];
trackedSignersWithNoCode++;
}
}

// Here we add the standard (non Safe signer) exceptions.
address[] memory shouldHaveCodeExceptions = new address[](4 + numberOfSafeSignersWithNoCode);

shouldHaveCodeExceptions[0] = systemConfigOwner;
shouldHaveCodeExceptions[1] = batchSenderAddress;
shouldHaveCodeExceptions[2] = p2pSequencerAddress;
shouldHaveCodeExceptions[3] = batchInboxAddress;

// And finally, we append the Safe signer exceptions.
for (uint256 i = 0; i < safeSignersWithNoCode.length; i++) {
shouldHaveCodeExceptions[4 + i] = safeSignersWithNoCode[i];
}

return shouldHaveCodeExceptions;
}

function _precheckDisputeGameImplementation() internal view {
console.log("pre-check new game implementations");

if (address(currentImpl) == address(0)) {
return;
}
Comment on lines +126 to +128
Copy link
Contributor

@Ethnical Ethnical Nov 1, 2024

Choose a reason for hiding this comment

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

NIT: Adding a comment above this line. To explain why we return early when the current impl == 0.
Even if this indicating in the doc:
No checks are performed if there is no prior implementation, in which case it is recommended to implement custom checks.

require(address(currentImpl.vm()) == address(faultDisputeGame.vm()), "10");
require(address(currentImpl.weth()) == address(faultDisputeGame.weth()), "20");
require(address(currentImpl.anchorStateRegistry()) == address(faultDisputeGame.anchorStateRegistry()), "30");
require(currentImpl.l2ChainId() == faultDisputeGame.l2ChainId(), "40");
require(currentImpl.splitDepth() == faultDisputeGame.splitDepth(), "50");
require(currentImpl.maxGameDepth() == faultDisputeGame.maxGameDepth(), "60");
require(uint64(Duration.unwrap(currentImpl.maxClockDuration())) == uint64(Duration.unwrap(faultDisputeGame.maxClockDuration())), "70");
require(uint64(Duration.unwrap(currentImpl.clockExtension())) == uint64(Duration.unwrap(faultDisputeGame.clockExtension())), "80");

if (targetGameType.raw() == GameTypes.PERMISSIONED_CANNON.raw()) {
PermissionedDisputeGame currentPDG = PermissionedDisputeGame(address(currentImpl));
PermissionedDisputeGame permissionedDisputeGame = PermissionedDisputeGame(address(faultDisputeGame));
require(address(currentPDG.proposer()) == address(permissionedDisputeGame.proposer()), "90");
require(address(currentPDG.challenger()) == address(permissionedDisputeGame.challenger()), "100");
}
}

function getAllowedStorageAccess() internal view override returns (address[] memory allowed) {
address[] memory nestedSafes = ownerSafe.getOwners();
allowed = new address[](3 + nestedSafes.length);
allowed[0] = address(dgfProxy);
allowed[1] = address(ownerSafe);
allowed[2] = livenessGuard;
for (uint256 i = 0; i < nestedSafes.length; i++) {
allowed[3 + i] = nestedSafes[i];
}
}

/// @notice Checks the correctness of the deployment
function _nestedPostCheck(Vm.AccountAccess[] memory accesses, SimulationPayload memory /* simPayload */ )
internal
view
override
{
console.log("Running post-deploy assertions");

checkStateDiff(accesses);
_checkDisputeGameImplementations();

console.log("All assertions passed!");
}

function _checkDisputeGameImplementations() internal view {
console.log("check dispute game implementations");

require(address(faultDisputeGame) == address(dgfProxy.gameImpls(targetGameType)), "check-100");
}

/// @notice Reads the contract addresses from lib/superchain-registry/superchain/configs/${l1ChainName}/${l2ChainName}.toml
function _getContractSet() internal returns (Types.ContractSet memory _proxies) {
string memory chainConfig;

// Read chain-specific config toml file
string memory path = string.concat(
"/lib/superchain-registry/superchain/configs/", l1ChainName, "/", l2ChainName, ".toml"
);
try vm.readFile(string.concat(vm.projectRoot(), path)) returns (string memory data) {
chainConfig = data;
} catch {
revert(string.concat("Failed to read ", path));
}

// Read the known EOAs out of the config toml file
systemConfigOwner = stdToml.readAddress(chainConfig, "$.addresses.SystemConfigOwner");
batchSenderAddress = stdToml.readAddress(chainConfig, "$.addresses.BatchSubmitter");
p2pSequencerAddress = stdToml.readAddress(chainConfig, "$.addresses.UnsafeBlockSigner");
batchInboxAddress = stdToml.readAddress(chainConfig, "$.batch_inbox_addr");

// Read the chain-specific OptimismPortalProxy address
_proxies.DisputeGameFactory = stdToml.readAddress(chainConfig, "$.addresses.DisputeGameFactoryProxy");
}
}
48 changes: 48 additions & 0 deletions tasks/sep/fp-recovery/005-set-game-implementation/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# ProxyAdminOwner - Set Dispute Game Implementation

Status: CONTINGENCY TASK, SIGN AS NEEDED

## Objective

This task sets the implementation for game type _TODO:game type_ to _TODO:new implementation address_.

## Tx #1: Set game implementation in the `DisputeGameFactoryProxy`

Sets the game type implementation contract.

**Function Signature:** `setImplementation(uint32,address)`

**To:** `0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1`

**Value:** `0 WEI`

**Raw Input Data:** `0x14f6b1a3<game-type><implementation-addr>`

### Inputs

**\_gameType:** `<user-input>`

**\_impl:** `<user-input>`

## Preparing the Operation

1. Copy this directory to the appropriate final task location.

2. Review the assertions in `NestedSignFromJson.s.sol` `_precheckDisputeGameImplementation` function.
The template assertions check that properties of the new implementation match the old one if it exists.
No checks are performed if there is no prior implementation, in which case it is recommended to implement custom checks.

3. Update the relative path to lib/superchain-registry in `justfile` if needed.
ajsutton marked this conversation as resolved.
Show resolved Hide resolved

4. Set the `L1_CHAIN_NAME` and `L2_CHAIN_NAME` configuration to the appropriate chain in the `.env` file.

5. Generate the batch with `just generate-input <gameType> <newImplAddr>`.

6. Collect signatures and execute the action according to the instructions in [NESTED.md](../../../NESTED.md).

### State Validations

The two state modifications that are made by this action are:

1. An update to the nonce of the Gnosis safe owner of the `ProxyAdminOwner`.
2. An update to the `gameImpls` mapping in `DisputeGameFactoryProxy`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"metadata": {
"name": "ProxyAdminOwner - Set Dispute Game Implementation",
"description": "This task sets the implementation for a specific dispute game type in the `DisputeGameFactory`."
},
"transactions": [
{
"metadata": {
"name": "Set implementation for game type",
"description": "Sets the implementation for a specific dispute game type in the `DisputeGameFactory`."
},
"to": "",
"value": "0x0",
"data": "",
"contractMethod": {
"type": "function",
"name": "setImplementation",
"inputs": [
{
"name": "_gameType",
"type": "uint32"
},
{
"name": "_impl",
"type": "address"
}
],
"outputs": [],
"stateMutability": "nonpayable"
},
"contractInputsValues": {
"_gameType": "",
"_impl": ""
}
}
]
}
28 changes: 28 additions & 0 deletions tasks/sep/fp-recovery/005-set-game-implementation/justfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
set positional-arguments

# default recipe to display help information
default:
@just --list

# Generate the `input.json` with the game type and implementation to set.
generate-input gameType='' implAddr='':
#!/usr/bin/env bash
set -euo pipefail
. .env
SET_GAME_TYPE_SIG="setImplementation(uint32,address)"
ENCODED_CALL=$(cast calldata "${SET_GAME_TYPE_SIG}" "{{gameType}}" "{{implAddr}}")

CONFIG_URL="https://raw.githubusercontent.com/ethereum-optimism/superchain-registry/refs/heads/main/superchain/configs/${L1_CHAIN_NAME}/${L2_CHAIN_NAME}.toml"
CONFIG_TOML=$(curl -s --show-error --fail "${CONFIG_URL}")
SC_CONFIG_ADDR=$(echo "${CONFIG_TOML}" | yq -p toml .addresses.SystemConfigProxy)
PROXY_ADMIN_OWNER_ADDR=$(echo "${CONFIG_TOML}" | yq -p toml .addresses.ProxyAdminOwner)

DGF_ADDR=$(cast call "${SC_CONFIG_ADDR}" "disputeGameFactory()(address)")
echo "OWNER_SAFE=${PROXY_ADMIN_OWNER_ADDR}" >> .env
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: When we launch multiples time the script this will append into the .env the OWNER_SAFE.
This shouldn't have any impact, I was thinking of a case when we run the script for the first time and wants to know how it works.
By adding multiples times this parameters this shouldn't not have impact, should we still fix this by checking if the OWNER_SAFE is already written into the .env or this is overkill in your opinion?
here ->

echo "OWNER_SAFE=${PROXY_ADMIN_OWNER_ADDR}" >> .env

CleanShot 2024-11-01 at 14 43 33@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it wasn't worth it given the last value is the one that gets used anyway. The smarter approach to this in #363 makes it much less likely this will happen as there's a separate prep task and then multiple set-implementation calls can be added to the batch.


cp ./input-template.json ./input.json
jq "(.transactions[0].data = \"$ENCODED_CALL\") |
(.transactions[0].to = \"${DGF_ADDR}\") |
(.transactions[0].contractInputsValues._gameType = \"{{gameType}}\") |
(.transactions[0].contractInputsValues._impl = \"{{implAddr}}\")" ./input.json > ./input.tmp.json
mv ./input.tmp.json ./input.json