From 2c963e3930e4253cc760609976f02b28faa79f28 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 3 Jul 2024 16:59:15 +0200 Subject: [PATCH] Add Safe to L2 Setup Contract (#759) This PR introduces a setup contract that can be called from the `Safe` setup function in order to automatically promote a Safe at setup time if the code is executing on an L2. Namely, this allows the Safe Proxy factory to use a single singleton and initializer for all chains, but end up with different `singleton`s depending on the chain ID. The expected use of this contract is to use the standard proxy factory: ```solidity Safe l1Singleton; SafeL2 l2Singleton; SafeToL2Setup l2Setup; proxyFactory.createProxyWithNonce( address(l1Singleton), abi.encodeCall( l1Singleton.setup, ( owners, threshold, address(l2Setup), abi.encodeCall(l2Setup.setupToL2, address(l2Singleton)), fallbackHandler, paymentToken, payment, paymentReceiver ) ), saltNonce ) ``` On L1 (i.e. Ethereum Mainnet where `chainId == 1`), you would end up with a Safe where `safe.singleton == l1Singleton` and on any other chains, you would end up with `safe.singleton == l2Singleton`. This would happen _before_ the first transaction. --------- Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com> --- .env.sample | 2 +- contracts/libraries/SafeToL2Setup.sol | 94 ++++++++++ hardhat.config.ts | 3 +- test/libraries/SafeToL2Setup.spec.ts | 252 ++++++++++++++++++++++++++ test/utils/setup.ts | 10 +- test/utils/storage.ts | 37 ++++ test/utils/strings.ts | 24 +++ 7 files changed, 416 insertions(+), 6 deletions(-) create mode 100644 contracts/libraries/SafeToL2Setup.sol create mode 100644 test/libraries/SafeToL2Setup.spec.ts create mode 100644 test/utils/strings.ts diff --git a/.env.sample b/.env.sample index 0f2a3ff2e..548217727 100644 --- a/.env.sample +++ b/.env.sample @@ -8,4 +8,4 @@ ETHERSCAN_API_KEY="" ERC4337_TEST_BUNDLER_URL= ERC4337_TEST_NODE_URL= ERC4337_TEST_SINGLETON_ADDRESS= -ERC4337_TEST_SAFE_FACTORY_ADDRESS= \ No newline at end of file +ERC4337_TEST_SAFE_FACTORY_ADDRESS= diff --git a/contracts/libraries/SafeToL2Setup.sol b/contracts/libraries/SafeToL2Setup.sol new file mode 100644 index 000000000..a88a4b7e3 --- /dev/null +++ b/contracts/libraries/SafeToL2Setup.sol @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.7.0 <0.9.0; + +import {SafeStorage} from "../libraries/SafeStorage.sol"; + +/** + * @title Safe to L2 Setup Contract + * @dev This contract expects the singleton to be the {Safe} by default. Even if there are more + * {SafeL2} proxies deployed, the average gas cost on L2s is significantly lower, making the + * current design more economically efficient overall. + * @notice This contract facilitates the deployment of a Safe to the same address on all networks by + * automatically changing the singleton to the L2 version when not on chain ID 1. + */ +contract SafeToL2Setup is SafeStorage { + /** + * @notice Address of the contract. + * @dev This is used to ensure that the contract is only ever `DELEGATECALL`-ed. + */ + address public immutable _SELF; + + /** + * @notice Event indicating a change of master copy address. + * @param singleton New master copy address + */ + event ChangedMasterCopy(address singleton); + + /** + * @notice Initializes a new {SafeToL2Setup} instance. + */ + constructor() { + _SELF = address(this); + } + + /** + * @notice Modifier ensure a function is only called via `DELEGATECALL`. Will revert otherwise. + */ + modifier onlyDelegateCall() { + require(address(this) != _SELF, "SafeToL2Setup should only be called via delegatecall"); + _; + } + + /** + * @notice Modifier to prevent using initialized Safes. + */ + modifier onlyNonceZero() { + require(nonce == 0, "Safe must have not executed any tx"); + _; + } + + /** + * @notice Modifier to ensure that the specified account is a contract. + * + */ + modifier onlyContract(address account) { + require(_codeSize(account) != 0, "Account doesn't contain code"); + _; + } + + /** + * @notice Setup the Safe with the provided L2 singleton if needed. + * @dev This function checks that the chain ID is not 1, and if it isn't updates the singleton + * to the provided L2 singleton. + */ + function setupToL2(address l2Singleton) public onlyDelegateCall onlyNonceZero onlyContract(l2Singleton) { + if (_chainId() != 1) { + singleton = l2Singleton; + emit ChangedMasterCopy(l2Singleton); + } + } + + /** + * @notice Returns the current chain ID. + */ + function _chainId() private view returns (uint256 result) { + /* solhint-disable no-inline-assembly */ + /// @solidity memory-safe-assembly + assembly { + result := chainid() + } + /* solhint-enable no-inline-assembly */ + } + + /** + * @notice Returns the code size of the specified account. + */ + function _codeSize(address account) internal view returns (uint256 result) { + /* solhint-disable no-inline-assembly */ + /// @solidity memory-safe-assembly + assembly { + result := extcodesize(account) + } + /* solhint-enable no-inline-assembly */ + } +} diff --git a/hardhat.config.ts b/hardhat.config.ts index a791f3c1a..dec9b4666 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -18,7 +18,7 @@ const argv = yargs // Load environment variables. dotenv.config(); -const { NODE_URL, INFURA_KEY, MNEMONIC, ETHERSCAN_API_KEY, PK, SOLIDITY_VERSION, SOLIDITY_SETTINGS } = process.env; +const { NODE_URL, INFURA_KEY, MNEMONIC, ETHERSCAN_API_KEY, PK, SOLIDITY_VERSION, SOLIDITY_SETTINGS, HARDHAT_CHAIN_ID } = process.env; const DEFAULT_MNEMONIC = "candy maple cake sugar pudding cream honey rich smooth crumble sweet treat"; @@ -75,6 +75,7 @@ const userConfig: HardhatUserConfig = { allowUnlimitedContractSize: true, blockGasLimit: 100000000, gas: 100000000, + chainId: typeof HARDHAT_CHAIN_ID === "string" && !Number.isNaN(parseInt(HARDHAT_CHAIN_ID)) ? parseInt(HARDHAT_CHAIN_ID) : 31337, }, mainnet: { ...sharedNetworkConfig, diff --git a/test/libraries/SafeToL2Setup.spec.ts b/test/libraries/SafeToL2Setup.spec.ts new file mode 100644 index 000000000..3a092a346 --- /dev/null +++ b/test/libraries/SafeToL2Setup.spec.ts @@ -0,0 +1,252 @@ +import { expect } from "chai"; +import hre, { deployments, ethers } from "hardhat"; +import { getFactory, getSafeL2SingletonContract, getSafeSingletonContract, getSafeWithOwners } from "../utils/setup"; +import { sameHexString } from "../utils/strings"; +import { executeContractCallWithSigners } from "../../src"; +import { EXPECTED_SAFE_STORAGE_LAYOUT, getContractStorageLayout } from "../utils/storage"; + +type HardhatTraceLog = { + depth: number; + gas: number; + gasCost: number; + op: string; + pc: number; + stack: string[]; + storage: { [key: string]: string }; + memory: string; +}; + +type HardhatTrace = { + failed: boolean; + gas: number; + returnValue: string; + structLogs: HardhatTraceLog[]; +}; + +describe("SafeToL2Setup", () => { + const setupTests = deployments.createFixture(async ({ deployments }) => { + await deployments.fixture(); + const safeToL2SetupLib = await (await hre.ethers.getContractFactory("SafeToL2Setup")).deploy(); + const signers = await ethers.getSigners(); + const safeSingleton = await getSafeSingletonContract(); + const safeL2 = await getSafeL2SingletonContract(); + const proxyFactory = await getFactory(); + return { + safeToL2SetupLib, + signers, + safeSingleton, + safeL2, + proxyFactory, + }; + }); + + describe("L2", () => { + before(function () { + if (hre.network.config.chainId === 1) { + this.skip(); + } + }); + + describe("setupToL2", () => { + it("follows the expected storage layout", async () => { + const safeStorageLayout = await getContractStorageLayout(hre, "SafeToL2Setup"); + + expect(safeStorageLayout).to.deep.eq(EXPECTED_SAFE_STORAGE_LAYOUT); + }); + + it("should emit an event", async () => { + const { + safeSingleton, + safeL2, + proxyFactory, + signers: [user1], + safeToL2SetupLib, + } = await setupTests(); + const safeL2SingletonAddress = await safeL2.getAddress(); + const safeToL2SetupCall = safeToL2SetupLib.interface.encodeFunctionData("setupToL2", [safeL2SingletonAddress]); + + const setupData = safeL2.interface.encodeFunctionData("setup", [ + [user1.address], + 1, + safeToL2SetupLib.target, + safeToL2SetupCall, + ethers.ZeroAddress, + ethers.ZeroAddress, + 0, + ethers.ZeroAddress, + ]); + const safeAddress = await proxyFactory.createProxyWithNonce.staticCall(safeSingleton.target, setupData, 0); + + await expect(proxyFactory.createProxyWithNonce(safeSingleton.target, setupData, 0)) + .to.emit(safeToL2SetupLib.attach(safeAddress), "ChangedMasterCopy") + .withArgs(safeL2SingletonAddress); + }); + + it("only allows singleton address that contains code", async () => { + const { + safeSingleton, + safeL2, + proxyFactory, + signers: [user1, user2], + safeToL2SetupLib, + } = await setupTests(); + const safeToL2SetupCall = safeToL2SetupLib.interface.encodeFunctionData("setupToL2", [user2.address]); + + const setupData = safeL2.interface.encodeFunctionData("setup", [ + [user1.address], + 1, + safeToL2SetupLib.target, + safeToL2SetupCall, + ethers.ZeroAddress, + ethers.ZeroAddress, + 0, + ethers.ZeroAddress, + ]); + + // For some reason, hardhat can't infer the revert reason + await expect(proxyFactory.createProxyWithNonce(safeSingleton.target, setupData, 0)).to.be.reverted; + }); + + it("can be used only via DELEGATECALL opcode", async () => { + const { safeToL2SetupLib } = await setupTests(); + const randomAddress = ethers.hexlify(ethers.randomBytes(20)); + + await expect(safeToL2SetupLib.setupToL2(randomAddress)).to.be.rejectedWith( + "SafeToL2Setup should only be called via delegatecall", + ); + }); + + it("can only be used through Safe initialization process", async () => { + const { + safeToL2SetupLib, + signers: [user1], + } = await setupTests(); + const safe = await getSafeWithOwners([user1.address]); + const safeToL2SetupLibAddress = await safeToL2SetupLib.getAddress(); + + await expect( + executeContractCallWithSigners(safe, safeToL2SetupLib, "setupToL2", [safeToL2SetupLibAddress], [user1], true), + ).to.be.rejectedWith("GS013"); + }); + + it("changes the expected storage slot without touching the most important ones", async () => { + const { + safeSingleton, + safeL2, + proxyFactory, + signers: [user1], + safeToL2SetupLib, + } = await setupTests(); + + const safeL2SingletonAddress = await safeL2.getAddress(); + const safeToL2SetupLibAddress = await safeToL2SetupLib.getAddress(); + const safeToL2SetupCall = safeToL2SetupLib.interface.encodeFunctionData("setupToL2", [safeL2SingletonAddress]); + + const setupData = safeL2.interface.encodeFunctionData("setup", [ + [user1.address], + 1, + safeToL2SetupLib.target, + safeToL2SetupCall, + ethers.ZeroAddress, + ethers.ZeroAddress, + 0, + ethers.ZeroAddress, + ]); + const safeAddress = await proxyFactory.createProxyWithNonce.staticCall(safeSingleton.target, setupData, 0); + + const transaction = await (await proxyFactory.createProxyWithNonce(safeSingleton.target, setupData, 0)).wait(); + if (!transaction?.hash) { + throw new Error("No transaction hash"); + } + // I decided to use tracing for this test because it gives an overview of all the storage slots involved in the transaction + // Alternatively, one could use `eth_getStorageAt` to check storage slots directly + // But that would not guarantee that other storage slots were not touched during the transaction + const trace = (await hre.network.provider.send("debug_traceTransaction", [transaction.hash])) as HardhatTrace; + // Hardhat uses the most basic struct/opcode logger tracer: https://geth.ethereum.org/docs/developers/evm-tracing/built-in-tracers#struct-opcode-logger + // To find the "snapshot" of the storage before the DELEGATECALL into the library, we need to find the first DELEGATECALL opcode calling into the library + // To do that, we search for the DELEGATECALL opcode with the stack input pointing to the library address + const delegateCallIntoTheLib = trace.structLogs.findIndex( + (log) => + log.op === "DELEGATECALL" && + sameHexString(log.stack[log.stack.length - 2], ethers.zeroPadValue(safeToL2SetupLibAddress, 32).slice(2)), + ); + const preDelegateCallStorage = trace.structLogs[delegateCallIntoTheLib].storage; + + // The SafeSetup event is emitted after the Safe is set up + // To get the storage snapshot after the Safe is set up, we need to find the LOG2 opcode with the topic input on the stack equal the SafeSetup event signature + const SAFE_SETUP_EVENT_SIGNATURE = safeSingleton.interface.getEvent("SafeSetup").topicHash; + const postSafeSetup = trace.structLogs.find( + (log, index) => + log.op === "LOG2" && + log.stack[log.stack.length - 3] === SAFE_SETUP_EVENT_SIGNATURE.slice(2) && + index > delegateCallIntoTheLib, + ); + if (!postSafeSetup) { + throw new Error("No SafeSetup event"); + } + const postSafeSetupStorage = postSafeSetup.storage; + + for (const [key, value] of Object.entries(postSafeSetupStorage)) { + // The slot key 0 is the singleton storage slot, it must equal the L2 singleton address + if (sameHexString(key, ethers.zeroPadValue("0x00", 32))) { + expect(sameHexString(ethers.zeroPadValue(safeL2SingletonAddress, 32), value)).to.be.true; + } else { + // All other storage slots must be the same as before the DELEGATECALL + if (key in preDelegateCallStorage) { + expect(sameHexString(preDelegateCallStorage[key], value)).to.be.true; + } else { + // This special case is needed because the SafeToL2Setup library inherits the SafeStorage library + // And that makes the tracer report all the storage slots in the SafeStorage library as well + // Even though if they were not touched during the transaction + expect(sameHexString(value, "0".repeat(64))).to.be.true; + } + } + } + + // Double-check that the storage slot was changed at the end of the transaction + const singletonInStorage = await hre.ethers.provider.getStorage(safeAddress, ethers.zeroPadValue("0x00", 32)); + expect(sameHexString(singletonInStorage, ethers.zeroPadValue(safeL2SingletonAddress, 32))).to.be.true; + }); + }); + }); + + describe("L1", () => { + before(function () { + if (hre.network.config.chainId !== 1) { + this.skip(); + } + }); + + it("should be a noop when the chain id is 1", async () => { + const { + safeSingleton, + safeL2, + proxyFactory, + signers: [user1], + safeToL2SetupLib, + } = await setupTests(); + const safeSingeltonAddress = await safeSingleton.getAddress(); + const safeL2SingletonAddress = await safeL2.getAddress(); + const safeToL2SetupCall = safeToL2SetupLib.interface.encodeFunctionData("setupToL2", [safeL2SingletonAddress]); + + const setupData = safeL2.interface.encodeFunctionData("setup", [ + [user1.address], + 1, + safeToL2SetupLib.target, + safeToL2SetupCall, + ethers.ZeroAddress, + ethers.ZeroAddress, + 0, + ethers.ZeroAddress, + ]); + const safeAddress = await proxyFactory.createProxyWithNonce.staticCall(safeSingleton.target, setupData, 0); + + await expect(proxyFactory.createProxyWithNonce(safeSingeltonAddress, setupData, 0)).to.not.emit( + safeToL2SetupLib.attach(safeAddress), + "ChangedMasterCopy", + ); + const singletonInStorage = await hre.ethers.provider.getStorage(safeAddress, ethers.zeroPadValue("0x00", 32)); + expect(sameHexString(singletonInStorage, ethers.zeroPadValue(safeSingeltonAddress, 32))).to.be.true; + }); + }); +}); diff --git a/test/utils/setup.ts b/test/utils/setup.ts index 07db2823f..4c1db8986 100644 --- a/test/utils/setup.ts +++ b/test/utils/setup.ts @@ -107,15 +107,17 @@ export const getSafeTemplate = async (saltNumber: string = getRandomIntAsString( export const getSafeWithOwners = async ( owners: string[], - threshold?: number, - fallbackHandler?: string, - logGasUsage?: boolean, + threshold: number = owners.length, + to: string = AddressZero, + data: string = "0x", + fallbackHandler: string = AddressZero, + logGasUsage: boolean = false, saltNumber: string = getRandomIntAsString(), ) => { const template = await getSafeTemplate(saltNumber); await logGas( `Setup Safe with ${owners.length} owner(s)${fallbackHandler && fallbackHandler !== AddressZero ? " and fallback handler" : ""}`, - template.setup(owners, threshold || owners.length, AddressZero, "0x", fallbackHandler || AddressZero, AddressZero, 0, AddressZero), + template.setup(owners, threshold, to, data, fallbackHandler, AddressZero, 0, AddressZero), !logGasUsage, ); return template; diff --git a/test/utils/storage.ts b/test/utils/storage.ts index 40b9cfe5c..a20ec1104 100644 --- a/test/utils/storage.ts +++ b/test/utils/storage.ts @@ -8,6 +8,43 @@ type StateVariable = { type: string; }; +export const EXPECTED_SAFE_STORAGE_LAYOUT: StateVariable[] = [ + { name: "singleton", slot: "0", offset: 0, type: "t_address" }, + { + name: "modules", + slot: "1", + offset: 0, + type: "t_mapping(t_address,t_address)", + }, + { + name: "owners", + slot: "2", + offset: 0, + type: "t_mapping(t_address,t_address)", + }, + { name: "ownerCount", slot: "3", offset: 0, type: "t_uint256" }, + { name: "threshold", slot: "4", offset: 0, type: "t_uint256" }, + { name: "nonce", slot: "5", offset: 0, type: "t_uint256" }, + { + name: "_deprecatedDomainSeparator", + slot: "6", + offset: 0, + type: "t_bytes32", + }, + { + name: "signedMessages", + slot: "7", + offset: 0, + type: "t_mapping(t_bytes32,t_uint256)", + }, + { + name: "approvedHashes", + slot: "8", + offset: 0, + type: "t_mapping(t_address,t_mapping(t_bytes32,t_uint256))", + }, +]; + export const getContractStorageLayout = async (hre: HardhatRuntimeEnvironment, smartContractName: string) => { const { sourceName, contractName } = await hre.artifacts.readArtifact(smartContractName); diff --git a/test/utils/strings.ts b/test/utils/strings.ts new file mode 100644 index 000000000..da3191a98 --- /dev/null +++ b/test/utils/strings.ts @@ -0,0 +1,24 @@ +/** + * Compares two strings and returns whether they are the same. + * @param a - The first string to compare. + * @param b - The second string to compare. + * @param ignoreCase - Optional. Specifies whether the comparison should be case-insensitive. Default is true. + * @returns True if the strings are the same, false otherwise. + */ +const sameString = (a: string, b: string, ignoreCase = true): boolean => { + return ignoreCase ? a.toLowerCase() === b.toLowerCase() : a === b; +}; + +/** + * Checks if two hexadecimal strings are the same, ignoring case by default. + * @param a - The first hexadecimal string. + * @param b - The second hexadecimal string. + * @param ignoreCase - Optional. If true, the comparison is case-insensitive. Default is true. + * @returns True if the hexadecimal strings are the same, false otherwise. + */ +const sameHexString = (a: string, b: string, ignoreCase = true): boolean => { + const normalized = (s: string) => s.toLowerCase().replace(/^0x/, ""); + return sameString(normalized(a), normalized(b), ignoreCase); +}; + +export { sameString, sameHexString };