Add template to set a dispute game implementation #354

ajsutton · 2024-10-28T05:13:45Z

Description

Adds a template to the fp-recovery directory to set the implementation for a particular game type. Most inputs are loaded from the superchain-registry to make it as easy as possible to adapt for other chains.

smartcontracts

Seems reasonable to me, would also like to get review from someone more familiar with writing superchain-ops tasks

tasks/sep/fp-recovery/005-set-game-implementation/.env

mds1 · 2024-10-28T22:31:20Z

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

+    GnosisSafe securityCouncilSafe = GnosisSafe(payable(vm.envAddress("COUNCIL_SAFE")));
+    GnosisSafe fndSafe = GnosisSafe(payable(vm.envAddress("FOUNDATION_SAFE")));
+    GnosisSafe ownerSafe = GnosisSafe(payable(vm.envAddress("OWNER_SAFE")));
+    address livenessGuard = 0xc26977310bC89DAee5823C2e2a73195E85382cC7;


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

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

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

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

tasks/sep/fp-recovery/005-set-game-implementation/README.md

tasks/sep/fp-recovery/005-set-game-implementation/justfile

mds1 · 2024-10-28T22:39:14Z

tasks/sep/fp-recovery/005-set-game-implementation/justfile

+  SET_GAME_TYPE_SIG="setImplementation(uint32, address)"
+  ENCODED_CALL=$(cast calldata "${SET_GAME_TYPE_SIG}" "{{gameType}}" "{{implAddr}}")
+
+  REGISTRY_DIR="../../../../lib/superchain-registry"


Should we just read the latest registry values directly with curl instead of requiring a local checkout? This way there's no risk the user has an outdated local version

We'd need to know where to find the SystemConfig or similar starting point to read the values from on-chain though and that would still come from the superchain-registry. It may still be better though since the superchain-registry version here may not have been updated since they deployed fault proofs so wouldn't have the DisputeGameFactory address but would have SystemConfig that we could get the DGF address from.

Yea perhaps we just make an HTTP request with curl or similar to fetch the system config address, then we can fetch the DGF and other addresses we need from that one

I was able to load the SystemConfig address via a curl to the superchain-registry and the DisputeGameFactory from there.

I could also load the OWNER_SAFE from the superchain-registry ProxyAdminOwner field. From there I can call getOwners() get the address of the foundation and council safes, but I don't know which is which. The NestedSignFromJson.sol file can be updated to not need them, but the parent nested.just needs the env vars set for foundation and council safes so it knows which to use.

I've left it with NestedSignFromJson.sol just generically handling all the nested safe owners so it doesn't need to know the specific foundation and council addresses, and the OWNER_SAFE env var is populated from the superchain-registry. We still need the COUNCIL_SAFE and FOUNDATION_SAFE env vars to make nested.just happy though. Not sure if we should just keep NestedSignFromJson.sol simpler until nested.just doesn't need the council and foundation safe addresses - the new code might be overkill (and it allows foundation safe owners to have no code which wasn't previously allowed).

but I don't know which is which

The only way I can think of to infer this is with a check based the threshold and/or number of owners: The foundation is a 5 of 7, and the SC is a 10 of 13.

nested.just may always need some way to know which safe is foundation and which is the council, because signers need to specify which safe they are signing for. Though now that I think of it, in theory we could take the signer address as input, determine which safe has that signer (assuming no duplicate signers across safes), and figure out which safe they need to sign with

I think what you have is ok, and perhaps we just add an assertion that ownerSafe has 2 (different) signers, and that each signer has code

Interesting, I think this is bit overkill to try to remove the:
COUNCIL_SAFE
FOUNDATION_SAFE
Even if this unlikely that the 1 signer is owner on both safe (for privacy) as the security council member are public.
This will leak the identity of the safe owner.
However, I do think keeping them here for the signers is with clear name like this are explicit and good to keep imo.

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

…omJson.s.sol Co-authored-by: Matt Solomon <[email protected]>

…uperchain-registry. Dynamically load the safe owners. Add revert reasons to pre checks.

mds1 · 2024-10-31T20:22:29Z

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

+    // 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.


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

Ethnical · 2024-11-01T11:46:01Z

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

+        _precheckDisputeGameImplementation();
+    }
+
+    function getCodeExceptions() internal view override returns (address[] memory) {


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.

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

Ethnical · 2024-11-01T11:51:38Z

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

+        if (address(currentImpl) == address(0)) {
+            return;
+        }


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.

Ethnical · 2024-11-01T11:57:59Z

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

+
+        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"));


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

Ethnical · 2024-11-01T11:58:05Z

tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFromJson.s.sol

+
+        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"));


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

Ethnical · 2024-11-01T12:18:11Z

tasks/sep/fp-recovery/005-set-game-implementation/justfile

+  SET_GAME_TYPE_SIG="setImplementation(uint32, address)"
+  ENCODED_CALL=$(cast calldata "${SET_GAME_TYPE_SIG}" "{{gameType}}" "{{implAddr}}")
+
+  REGISTRY_DIR="../../../../lib/superchain-registry"


Interesting, I think this is bit overkill to try to remove the:
COUNCIL_SAFE
FOUNDATION_SAFE
Even if this unlikely that the 1 signer is owner on both safe (for privacy) as the security council member are public.
This will leak the identity of the safe owner.
However, I do think keeping them here for the signers is with clear name like this are explicit and good to keep imo.

Ethnical

Seems good to me!
Great works, thanks!
There is few nits that can be discussed but overall looks great!

Ethnical · 2024-11-01T13:49:11Z

tasks/sep/fp-recovery/005-set-game-implementation/justfile

+  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


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 ->

superchain-ops/tasks/sep/fp-recovery/005-set-game-implementation/justfile

Line 21 in b73271f

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

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.

mds1 · 2024-11-04T22:30:24Z

Closing this since #363 says it will replace this PR, but please reopen if we still need this for any reason

Add template to set a game implementation.

b83e3a4

ajsutton requested review from a team as code owners October 28, 2024 05:13

ajsutton requested a review from maurelian October 28, 2024 05:13

smartcontracts reviewed Oct 28, 2024

View reviewed changes

mds1 reviewed Oct 28, 2024

View reviewed changes

ajsutton and others added 3 commits October 30, 2024 09:36

Update tasks/sep/fp-recovery/005-set-game-implementation/NestedSignFr…

fa74643

…omJson.s.sol Co-authored-by: Matt Solomon <[email protected]>

Fix a bunch of review feedback.

3d3c16e

Load the DisputeGameFactory address and OwnerSafe via a curl to the s…

b73271f

…uperchain-registry. Dynamically load the safe owners. Add revert reasons to pre checks.

ajsutton requested a review from mds1 October 30, 2024 01:08

ajsutton mentioned this pull request Oct 31, 2024

Chain-independent script to generate setImplementation operations #363

Merged

mds1 reviewed Oct 31, 2024

View reviewed changes

Ethnical reviewed Nov 1, 2024

View reviewed changes

Ethnical approved these changes Nov 1, 2024

View reviewed changes

mds1 closed this Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add template to set a dispute game implementation #354

Add template to set a dispute game implementation #354

ajsutton commented Oct 28, 2024

smartcontracts left a comment

mds1 Oct 28, 2024

ajsutton Oct 29, 2024

mds1 Oct 31, 2024

mds1 Oct 28, 2024

ajsutton Oct 29, 2024

mds1 Oct 29, 2024

ajsutton Oct 30, 2024

mds1 Oct 31, 2024

Ethnical Nov 1, 2024

mds1 Oct 31, 2024

Ethnical Oct 31, 2024

Ethnical Nov 1, 2024

mds1 Nov 1, 2024

Ethnical Nov 1, 2024 •

edited

Loading

Ethnical Nov 1, 2024

Ethnical Nov 1, 2024

Ethnical Nov 1, 2024

Ethnical left a comment

Ethnical Nov 1, 2024

ajsutton Nov 3, 2024

mds1 commented Nov 4, 2024

Add template to set a dispute game implementation #354

Add template to set a dispute game implementation #354

Conversation

ajsutton commented Oct 28, 2024

smartcontracts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethnical Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethnical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mds1 commented Nov 4, 2024

Ethnical Nov 1, 2024 •

edited

Loading