From 828bf65182d0f976f92c73153fb9d2a15e270063 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Thu, 19 Oct 2023 19:36:04 +0200 Subject: [PATCH 01/11] Add contract to migrate a Safe from not L2 to L2 - Related to https://github.com/safe-global/safe-transaction-service/issues/1703 --- contracts/libraries/SafeToL2Migration.sol | 131 +++++++++++++++ test/libraries/SafeToL2Migration.spec.ts | 195 ++++++++++++++++++++++ 2 files changed, 326 insertions(+) create mode 100644 contracts/libraries/SafeToL2Migration.sol create mode 100644 test/libraries/SafeToL2Migration.spec.ts diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol new file mode 100644 index 0000000..469bf48 --- /dev/null +++ b/contracts/libraries/SafeToL2Migration.sol @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: LGPL-3.0-only +/* solhint-disable one-contract-per-file */ +pragma solidity >=0.7.0 <0.9.0; + +import {SafeStorage} from "../libraries/SafeStorage.sol"; +import {Enum} from "../common/Enum.sol"; + +interface ISafe { + function VERSION() external view returns (string memory); +} + + + +/** + * @title Migration Contract for updating a Safe from 1.3.0/1.4.1 version to a L2 version. Useful when replaying a Safe from a non L2 network in a L2 network. + * @notice This contract facilitates the migration of a Safe contract from version 1.3.0 to 1.3.0L2 or from 1.4.1 to 1.4.1L2 + * Older versions are not supported + * @dev IMPORTANT: The migration will only work with proxies that store the implementation address in the storage slot 0. + */ +contract SafeToL2Migration is SafeStorage { + // Address of this contract + address public immutable MIGRATION_SINGLETON; + + /** + * @notice Constructor + * @dev Initializes the migrationSingleton with the contract's own address. + */ + constructor() { + MIGRATION_SINGLETON = address(this); + } + + /** + * @notice Event indicating a change of master copy address. + * @param singleton New master copy address + */ + event ChangedMasterCopy(address singleton); + + event SafeMultiSigTransaction( + address to, + uint256 value, + bytes data, + Enum.Operation operation, + uint256 safeTxGas, + uint256 baseGas, + uint256 gasPrice, + address gasToken, + address payable refundReceiver, + bytes signatures, + // We combine nonce, sender and threshold into one to avoid stack too deep + // Dev note: additionalInfo should not contain `bytes`, as this complicates decoding + bytes additionalInfo + ); + + /** + * @notice Migrate from Safe 1.3.0/1.4.1 Singleton (L1) to the same version provided L2 singleton + * @dev This function should only be called via a delegatecall to perform the upgrade. + */ + function migrateToL2(address l2Singleton) public { + require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); + require(address(singleton) != l2Singleton, "Safe is already using the singleton"); + // Nonce is increased before executing a tx, so first executed tx will have nonce=1 + require(nonce == 1, "Safe must have not executed any tx"); + bytes32 oldSingletonVersion = keccak256(abi.encodePacked(ISafe(singleton).VERSION())); + bytes32 newSingletonVersion = keccak256(abi.encodePacked(ISafe(l2Singleton).VERSION())); + + require(oldSingletonVersion == newSingletonVersion, "L2 singleton must match current version singleton"); + // There's no way to make sure if address is a valid singleton, unless we cofigure the contract for every chain + require(newSingletonVersion == keccak256(abi.encodePacked("1.3.0")) || newSingletonVersion == keccak256(abi.encodePacked("1.4.1")), "Provided singleton version is not supported"); + + singleton = l2Singleton; + + // Simulate a L2 transaction so indexer picks up the Safe + // 0xef2624ae - keccack("migrateToL2(address)") + bytes memory data = abi.encodeWithSelector(0xef2624ae, l2Singleton); + bytes memory additionalInfo; + { + // nonce, sender, threshold + additionalInfo = abi.encode(nonce, msg.sender, threshold); + } + emit SafeMultiSigTransaction( + MIGRATION_SINGLETON, + 0, + data, + Enum.Operation.DelegateCall, + 0, + 0, + 0, + address(0), + address(0), + "", // We cannot detect signatures + additionalInfo + ); + emit ChangedMasterCopy(singleton); + } + + /** + * @notice Checks whether an Ethereum address corresponds to a contract or an externally owned account (EOA). + * + * @param account The Ethereum address to be checked. + * + * @return A boolean value indicating whether the address is associated with a contract (true) or an EOA (false). + * + * @dev This function relies on the `extcodesize` assembly opcode to determine whether an address is a contract. + * It may return incorrect results in some edge cases: + * + * - During the contract deployment process, including the constructor, this function may incorrectly identify the + * contract's own address as an EOA, as the code is not yet deployed. + * + * - If a contract performs a self-destruct operation (using `selfdestruct`) after deployment, this function may + * incorrectly identify the address as an EOA once the contract is destroyed, as its code will be removed. + * + * - When interacting with external contracts that use delegatecall or other mechanisms to execute code from + * different contracts, this function may not accurately distinguish between a contract and an EOA, as it only + * checks the code size at the specified address. + * + * - Contracts that are created using the CREATE2 opcode may not be accurately identified as contracts by this + * function, especially if the code is not deployed until after the creation. + * + * Developers should use caution when relying on the results of this function for critical decision-making. + */ + function isContract(address account) internal view returns (bool) { + uint256 size; + // solhint-disable-next-line no-inline-assembly + assembly { + size := extcodesize(account) + } + + // If the code size is greater than 0, it is a contract; otherwise, it is an EOA. + return size > 0; + } +} diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts new file mode 100644 index 0000000..d1340e6 --- /dev/null +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -0,0 +1,195 @@ +import { expect } from "chai"; +import hre, { ethers, deployments } from "hardhat"; +import { AddressZero } from "@ethersproject/constants"; +import { getSafeWithSingleton, migrationContractTo150, getSafeSingletonAt, getMock } from "../utils/setup"; +import deploymentData from "../json/safeDeployment.json"; +import safeRuntimeBytecode from "../json/safeRuntimeBytecode.json"; +import { buildSafeTransaction, executeContractCallWithSigners, executeTxWithSigners } from "../../src/utils/execution"; + +const SAFE_SINGLETON_141_ADDRESS = "0x3E5c63644E683549055b9Be8653de26E0B4CD36E"; + +const SAFE_SINGLETON_141_L2_ADDRESS = "0xfb1bffC9d739B8D520DaF37dF666da4C687191EA"; + +const SAFE_SINGLETON_150_ADDRESS = "0x88627c8904eCd9DF96A572Ef32A7ff13b199Ed8D"; + +const SAFE_SINGLETON_150_L2_ADDRESS = "0x0Ee37514644683f7EB9745a5726C722DeBa77e52"; + +const FALLBACK_HANDLER_STORAGE_SLOT = "0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5"; + +const GUARD_STORAGE_SLOT = "0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8"; + +describe("SafeToL2Migration library", () => { + const migratedInterface = new ethers.Interface(["function masterCopy() view returns(address)"]); + + const setupTests = deployments.createFixture(async ({ deployments }) => { + await deployments.fixture(); + + // Set the runtime code for hardcoded addresses, so the expected events are emitted + await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_141_ADDRESS, safeRuntimeBytecode.safe141]); + await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_141_L2_ADDRESS, safeRuntimeBytecode.safe141l2]); + await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_150_ADDRESS, safeRuntimeBytecode.safe150]); + await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_150_L2_ADDRESS, safeRuntimeBytecode.safe150l2]); + + const signers = await ethers.getSigners(); + const [user1] = signers; + const singleton130Address = (await (await user1.sendTransaction({ data: deploymentData.safe130 })).wait())?.contractAddress; + const singleton130L2Address = (await (await user1.sendTransaction({ data: deploymentData.safe130l2 })).wait())?.contractAddress; + + if (!singleton130Address || !singleton130L2Address) { + throw new Error("Could not deploy Safe130 or Safe130L2"); + } + const singleton130 = await getSafeSingletonAt(singleton130Address); + const singleton130L2 = await getSafeSingletonAt(singleton130L2Address); + + const guardContract = await hre.ethers.getContractAt("Guard", AddressZero); + const guardEip165Calldata = guardContract.interface.encodeFunctionData("supportsInterface", ["0x945b8148"]); + const validGuardMock = await getMock(); + await validGuardMock.givenCalldataReturnBool(guardEip165Calldata, true); + + const invalidGuardMock = await getMock(); + await invalidGuardMock.givenCalldataReturnBool(guardEip165Calldata, false); + + const safeWith1967Proxy = await getSafeSingletonAt( + await hre.ethers + .getContractFactory("UpgradeableProxy") + .then((factory) => + factory.deploy( + singleton130Address, + singleton130.interface.encodeFunctionData("setup", [ + [user1.address], + 1, + AddressZero, + "0x", + AddressZero, + AddressZero, + 0, + AddressZero, + ]), + ), + ) + .then((proxy) => proxy.getAddress()), + ); + const safeToL2MigrationContract = await hre.ethers.getContractFactory("SafeToL2Migration"); + const migration = await safeToL2MigrationContract.deploy(); + return { + safe130: await getSafeWithSingleton(singleton130, [user1.address]), + safe130l2: await getSafeWithSingleton(singleton130L2, [user1.address]), + safeWith1967Proxy, + migration, + signers, + validGuardMock, + invalidGuardMock, + singleton130Address, + singleton130L2Address, + }; + }); + + describe("migrateToL2", () => { + it("reverts if the singleton is not set", async () => { + const { + migration, + safeWith1967Proxy, + signers: [user1], + singleton130L2Address, + } = await setupTests(); + + await expect( + executeContractCallWithSigners(safeWith1967Proxy, migration, "migrateToL2", [singleton130L2Address], [user1], true), + ).to.be.revertedWith("GS013"); + }); + + it("reverts if new singleton is the same as the old one", async () => { + const { + safe130, + migration, + signers: [user1], + singleton130Address, + } = await setupTests(); + await expect( + executeContractCallWithSigners(safe130, migration, "migrateToL2", [singleton130Address], [user1], true), + ).to.be.revertedWith("GS013"); + }); + + it("reverts if new singleton is not supported", async () => { + const { + safe130, + migration, + signers: [user1], + } = await setupTests(); + await expect( + executeContractCallWithSigners(safe130, migration, "migrateToL2", [SAFE_SINGLETON_150_L2_ADDRESS], [user1], true), + ).to.be.revertedWith("GS013"); + }); + + it("reverts if nonce > 0", async () => { + const { + safe130, + migration, + signers: [user1], + singleton130Address, + singleton130L2Address, + } = await setupTests(); + const safeAddress = await safe130.getAddress(); + // The emit matcher checks the address, which is the Safe as delegatecall is used + const migrationSafe = migration.attach(safeAddress); + + // Increase nonce by sending eth + await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") }); + const nonce = 0; + const safeTx = buildSafeTransaction({ to: user1.address, value: ethers.parseEther("1"), nonce }); + await executeTxWithSigners(safe130, safeTx, [user1]); + + await expect( + executeContractCallWithSigners(safe130, migration, "migrateToL2", [singleton130L2Address], [user1], true), + ).to.be.revertedWith("GS013"); + + const singletonResp = await user1.call({ to: safeAddress, data: migratedInterface.encodeFunctionData("masterCopy") }); + expect(migratedInterface.decodeFunctionResult("masterCopy", singletonResp)[0]).to.eq(singleton130Address); + }); + + it("migrates from singleton 1.3.0 to 1.3.0L2", async () => { + const { + safe130, + migration, + signers: [user1], + singleton130L2Address, + } = await setupTests(); + const safeAddress = await safe130.getAddress(); + // The emit matcher checks the address, which is the Safe as delegatecall is used + const migrationSafe = migration.attach(safeAddress); + + await expect(executeContractCallWithSigners(safe130, migration, "migrateToL2", [singleton130L2Address], [user1], true)) + .to.emit(migrationSafe, "ChangedMasterCopy") + .withArgs(singleton130L2Address); + + const singletonResp = await user1.call({ to: safeAddress, data: migratedInterface.encodeFunctionData("masterCopy") }); + expect(migratedInterface.decodeFunctionResult("masterCopy", singletonResp)[0]).to.eq(singleton130L2Address); + }); + + it("doesn't touch important storage slots", async () => { + const { + safe130, + migration, + signers: [user1], + singleton130L2Address, + } = await setupTests(); + const safeAddress = await safe130.getAddress(); + + const ownerCountBeforeMigration = await hre.ethers.provider.getStorage(safeAddress, 3); + const thresholdBeforeMigration = await hre.ethers.provider.getStorage(safeAddress, 4); + const nonceBeforeMigration = await hre.ethers.provider.getStorage(safeAddress, 5); + const guardBeforeMigration = await hre.ethers.provider.getStorage(safeAddress, GUARD_STORAGE_SLOT); + const fallbackHandlerBeforeMigration = await hre.ethers.provider.getStorage(safeAddress, FALLBACK_HANDLER_STORAGE_SLOT); + + await expect(executeContractCallWithSigners(safe130, migration, "migrateToL2", [singleton130L2Address], [user1], true)); + + expect(await hre.ethers.provider.getStorage(safeAddress, 3)).to.be.eq(ownerCountBeforeMigration); + expect(await hre.ethers.provider.getStorage(safeAddress, 4)).to.be.eq(thresholdBeforeMigration); + expect(await hre.ethers.provider.getStorage(safeAddress, 5)).to.be.eq(nonceBeforeMigration); + expect(await hre.ethers.provider.getStorage(safeAddress, GUARD_STORAGE_SLOT)).to.be.eq(guardBeforeMigration); + expect(await hre.ethers.provider.getStorage(safeAddress, FALLBACK_HANDLER_STORAGE_SLOT)).to.be.eq( + fallbackHandlerBeforeMigration, + ); + }); + }); +}); From 8aea6e4b65a5f123193669b378297769bd62f076 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Fri, 20 Oct 2023 10:49:44 +0200 Subject: [PATCH 02/11] Fix linting --- test/libraries/SafeToL2Migration.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts index d1340e6..c774e00 100644 --- a/test/libraries/SafeToL2Migration.spec.ts +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -1,7 +1,7 @@ import { expect } from "chai"; import hre, { ethers, deployments } from "hardhat"; import { AddressZero } from "@ethersproject/constants"; -import { getSafeWithSingleton, migrationContractTo150, getSafeSingletonAt, getMock } from "../utils/setup"; +import { getSafeWithSingleton, getSafeSingletonAt, getMock } from "../utils/setup"; import deploymentData from "../json/safeDeployment.json"; import safeRuntimeBytecode from "../json/safeRuntimeBytecode.json"; import { buildSafeTransaction, executeContractCallWithSigners, executeTxWithSigners } from "../../src/utils/execution"; @@ -130,8 +130,6 @@ describe("SafeToL2Migration library", () => { singleton130L2Address, } = await setupTests(); const safeAddress = await safe130.getAddress(); - // The emit matcher checks the address, which is the Safe as delegatecall is used - const migrationSafe = migration.attach(safeAddress); // Increase nonce by sending eth await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") }); From 3c888a5d2d988bd9c9b338c578b8f99c21411a0a Mon Sep 17 00:00:00 2001 From: dodger213 Date: Fri, 20 Oct 2023 14:17:18 +0200 Subject: [PATCH 03/11] Add tests --- contracts/libraries/SafeToL2Migration.sol | 8 +++--- test/libraries/SafeToL2Migration.spec.ts | 31 ++++++++++++++++++++--- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index 469bf48..e7a8142 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -6,6 +6,7 @@ import {SafeStorage} from "../libraries/SafeStorage.sol"; import {Enum} from "../common/Enum.sol"; interface ISafe { + // solhint-disable-next-line function VERSION() external view returns (string memory); } @@ -72,11 +73,8 @@ contract SafeToL2Migration is SafeStorage { // Simulate a L2 transaction so indexer picks up the Safe // 0xef2624ae - keccack("migrateToL2(address)") bytes memory data = abi.encodeWithSelector(0xef2624ae, l2Singleton); - bytes memory additionalInfo; - { - // nonce, sender, threshold - additionalInfo = abi.encode(nonce, msg.sender, threshold); - } + // nonce, sender, threshold + bytes memory additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); emit SafeMultiSigTransaction( MIGRATION_SINGLETON, 0, diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts index c774e00..15ab734 100644 --- a/test/libraries/SafeToL2Migration.spec.ts +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -130,6 +130,7 @@ describe("SafeToL2Migration library", () => { singleton130L2Address, } = await setupTests(); const safeAddress = await safe130.getAddress(); + expect(await safe130.nonce()).to.be.eq(0); // Increase nonce by sending eth await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") }); @@ -137,6 +138,7 @@ describe("SafeToL2Migration library", () => { const safeTx = buildSafeTransaction({ to: user1.address, value: ethers.parseEther("1"), nonce }); await executeTxWithSigners(safe130, safeTx, [user1]); + expect(await safe130.nonce()).to.be.eq(1); await expect( executeContractCallWithSigners(safe130, migration, "migrateToL2", [singleton130L2Address], [user1], true), ).to.be.revertedWith("GS013"); @@ -155,13 +157,36 @@ describe("SafeToL2Migration library", () => { const safeAddress = await safe130.getAddress(); // The emit matcher checks the address, which is the Safe as delegatecall is used const migrationSafe = migration.attach(safeAddress); - - await expect(executeContractCallWithSigners(safe130, migration, "migrateToL2", [singleton130L2Address], [user1], true)) + const migrationAddress = await migration.getAddress(); + + const functionName = "migrateToL2"; + const expectedData = migration.interface.encodeFunctionData(functionName, [singleton130L2Address]); + const safeThreshold = await safe130.getThreshold(); + const additionalInfo = hre.ethers.AbiCoder.defaultAbiCoder().encode( + ["uint256", "address", "uint256"], + [0, user1.address, safeThreshold], + ); + await expect(executeContractCallWithSigners(safe130, migration, functionName, [singleton130L2Address], [user1], true)) .to.emit(migrationSafe, "ChangedMasterCopy") - .withArgs(singleton130L2Address); + .withArgs(singleton130L2Address) + .to.emit(migrationSafe, "SafeMultiSigTransaction") + .withArgs( + migrationAddress, + 0, + expectedData, + 1, + 0, + 0, + 0, + AddressZero, + AddressZero, + "0x", // We cannot detect signatures + additionalInfo, + ); const singletonResp = await user1.call({ to: safeAddress, data: migratedInterface.encodeFunctionData("masterCopy") }); expect(migratedInterface.decodeFunctionResult("masterCopy", singletonResp)[0]).to.eq(singleton130L2Address); + expect(await safe130.nonce()).to.be.eq(1); }); it("doesn't touch important storage slots", async () => { From 6853efdf1f8e2dc8c7fa4cf3f4c2bc882f5010f1 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 24 Oct 2023 12:29:48 +0200 Subject: [PATCH 04/11] Remove dead code --- contracts/libraries/SafeToL2Migration.sol | 47 +++-------------------- 1 file changed, 6 insertions(+), 41 deletions(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index e7a8142..4020ad6 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -10,8 +10,6 @@ interface ISafe { function VERSION() external view returns (string memory); } - - /** * @title Migration Contract for updating a Safe from 1.3.0/1.4.1 version to a L2 version. Useful when replaying a Safe from a non L2 network in a L2 network. * @notice This contract facilitates the migration of a Safe contract from version 1.3.0 to 1.3.0L2 or from 1.4.1 to 1.4.1L2 @@ -66,8 +64,11 @@ contract SafeToL2Migration is SafeStorage { require(oldSingletonVersion == newSingletonVersion, "L2 singleton must match current version singleton"); // There's no way to make sure if address is a valid singleton, unless we cofigure the contract for every chain - require(newSingletonVersion == keccak256(abi.encodePacked("1.3.0")) || newSingletonVersion == keccak256(abi.encodePacked("1.4.1")), "Provided singleton version is not supported"); - + require( + newSingletonVersion == keccak256(abi.encodePacked("1.3.0")) || newSingletonVersion == keccak256(abi.encodePacked("1.4.1")), + "Provided singleton version is not supported" + ); + singleton = l2Singleton; // Simulate a L2 transaction so indexer picks up the Safe @@ -85,45 +86,9 @@ contract SafeToL2Migration is SafeStorage { 0, address(0), address(0), - "", // We cannot detect signatures + "", // We cannot detect signatures additionalInfo ); emit ChangedMasterCopy(singleton); } - - /** - * @notice Checks whether an Ethereum address corresponds to a contract or an externally owned account (EOA). - * - * @param account The Ethereum address to be checked. - * - * @return A boolean value indicating whether the address is associated with a contract (true) or an EOA (false). - * - * @dev This function relies on the `extcodesize` assembly opcode to determine whether an address is a contract. - * It may return incorrect results in some edge cases: - * - * - During the contract deployment process, including the constructor, this function may incorrectly identify the - * contract's own address as an EOA, as the code is not yet deployed. - * - * - If a contract performs a self-destruct operation (using `selfdestruct`) after deployment, this function may - * incorrectly identify the address as an EOA once the contract is destroyed, as its code will be removed. - * - * - When interacting with external contracts that use delegatecall or other mechanisms to execute code from - * different contracts, this function may not accurately distinguish between a contract and an EOA, as it only - * checks the code size at the specified address. - * - * - Contracts that are created using the CREATE2 opcode may not be accurately identified as contracts by this - * function, especially if the code is not deployed until after the creation. - * - * Developers should use caution when relying on the results of this function for critical decision-making. - */ - function isContract(address account) internal view returns (bool) { - uint256 size; - // solhint-disable-next-line no-inline-assembly - assembly { - size := extcodesize(account) - } - - // If the code size is greater than 0, it is a contract; otherwise, it is an EOA. - return size > 0; - } } From ceefca91109eeafb88bde8bde3cb2a92e4b59330 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 24 Oct 2023 14:32:04 +0200 Subject: [PATCH 05/11] Update contracts/libraries/SafeToL2Migration.sol Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com> --- contracts/libraries/SafeToL2Migration.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index 4020ad6..e8a19ed 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -72,7 +72,7 @@ contract SafeToL2Migration is SafeStorage { singleton = l2Singleton; // Simulate a L2 transaction so indexer picks up the Safe - // 0xef2624ae - keccack("migrateToL2(address)") + // 0xef2624ae - keccak("migrateToL2(address)") bytes memory data = abi.encodeWithSelector(0xef2624ae, l2Singleton); // nonce, sender, threshold bytes memory additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); From 4b3dc883d1ef5258d35e394c01cd5ca7e55f897d Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 24 Oct 2023 14:35:16 +0200 Subject: [PATCH 06/11] Adding docs --- contracts/libraries/SafeToL2Migration.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index e8a19ed..d937fbb 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -52,7 +52,9 @@ contract SafeToL2Migration is SafeStorage { /** * @notice Migrate from Safe 1.3.0/1.4.1 Singleton (L1) to the same version provided L2 singleton + * Safe is required to have nonce 0 so backend can support it after the migration * @dev This function should only be called via a delegatecall to perform the upgrade. + * Singletons versions will be compared, so it implies that contract exists */ function migrateToL2(address l2Singleton) public { require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); From 16469caff82978da05a18d16d40a6122107a028f Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 24 Oct 2023 14:36:05 +0200 Subject: [PATCH 07/11] Fix typo --- contracts/libraries/SafeToL2Migration.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index d937fbb..608d282 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -54,7 +54,7 @@ contract SafeToL2Migration is SafeStorage { * @notice Migrate from Safe 1.3.0/1.4.1 Singleton (L1) to the same version provided L2 singleton * Safe is required to have nonce 0 so backend can support it after the migration * @dev This function should only be called via a delegatecall to perform the upgrade. - * Singletons versions will be compared, so it implies that contract exists + * Singletons versions will be compared, so it implies that contracts exist */ function migrateToL2(address l2Singleton) public { require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); From b0e498fd89d5350b6d6bb76d7951b316816ed7f6 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 24 Oct 2023 14:44:17 +0200 Subject: [PATCH 08/11] Add test for migrating from v1.4.1 to v1.4.1 L2 --- test/libraries/SafeToL2Migration.spec.ts | 48 +++++++++++++++++++++--- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts index 15ab734..cee5196 100644 --- a/test/libraries/SafeToL2Migration.spec.ts +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -10,8 +10,6 @@ const SAFE_SINGLETON_141_ADDRESS = "0x3E5c63644E683549055b9Be8653de26E0B4CD36E"; const SAFE_SINGLETON_141_L2_ADDRESS = "0xfb1bffC9d739B8D520DaF37dF666da4C687191EA"; -const SAFE_SINGLETON_150_ADDRESS = "0x88627c8904eCd9DF96A572Ef32A7ff13b199Ed8D"; - const SAFE_SINGLETON_150_L2_ADDRESS = "0x0Ee37514644683f7EB9745a5726C722DeBa77e52"; const FALLBACK_HANDLER_STORAGE_SLOT = "0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5"; @@ -27,7 +25,6 @@ describe("SafeToL2Migration library", () => { // Set the runtime code for hardcoded addresses, so the expected events are emitted await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_141_ADDRESS, safeRuntimeBytecode.safe141]); await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_141_L2_ADDRESS, safeRuntimeBytecode.safe141l2]); - await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_150_ADDRESS, safeRuntimeBytecode.safe150]); await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_150_L2_ADDRESS, safeRuntimeBytecode.safe150l2]); const signers = await ethers.getSigners(); @@ -39,7 +36,7 @@ describe("SafeToL2Migration library", () => { throw new Error("Could not deploy Safe130 or Safe130L2"); } const singleton130 = await getSafeSingletonAt(singleton130Address); - const singleton130L2 = await getSafeSingletonAt(singleton130L2Address); + const singleton141 = await getSafeSingletonAt(SAFE_SINGLETON_141_ADDRESS); const guardContract = await hre.ethers.getContractAt("Guard", AddressZero); const guardEip165Calldata = guardContract.interface.encodeFunctionData("supportsInterface", ["0x945b8148"]); @@ -73,7 +70,7 @@ describe("SafeToL2Migration library", () => { const migration = await safeToL2MigrationContract.deploy(); return { safe130: await getSafeWithSingleton(singleton130, [user1.address]), - safe130l2: await getSafeWithSingleton(singleton130L2, [user1.address]), + safe141: await getSafeWithSingleton(singleton141, [user1.address]), safeWith1967Proxy, migration, signers, @@ -189,6 +186,47 @@ describe("SafeToL2Migration library", () => { expect(await safe130.nonce()).to.be.eq(1); }); + it("migrates from singleton 1.4.1 to 1.4.1L2", async () => { + const { + safe141, + migration, + signers: [user1], + } = await setupTests(); + const safeAddress = await safe141.getAddress(); + // The emit matcher checks the address, which is the Safe as delegatecall is used + const migrationSafe = migration.attach(safeAddress); + const migrationAddress = await migration.getAddress(); + + const functionName = "migrateToL2"; + const expectedData = migration.interface.encodeFunctionData(functionName, [SAFE_SINGLETON_141_L2_ADDRESS]); + const safeThreshold = await safe141.getThreshold(); + const additionalInfo = hre.ethers.AbiCoder.defaultAbiCoder().encode( + ["uint256", "address", "uint256"], + [0, user1.address, safeThreshold], + ); + await expect(executeContractCallWithSigners(safe141, migration, functionName, [SAFE_SINGLETON_141_L2_ADDRESS], [user1], true)) + .to.emit(migrationSafe, "ChangedMasterCopy") + .withArgs(SAFE_SINGLETON_141_L2_ADDRESS) + .to.emit(migrationSafe, "SafeMultiSigTransaction") + .withArgs( + migrationAddress, + 0, + expectedData, + 1, + 0, + 0, + 0, + AddressZero, + AddressZero, + "0x", // We cannot detect signatures + additionalInfo, + ); + + const singletonResp = await user1.call({ to: safeAddress, data: migratedInterface.encodeFunctionData("masterCopy") }); + expect(migratedInterface.decodeFunctionResult("masterCopy", singletonResp)[0]).to.eq(SAFE_SINGLETON_141_L2_ADDRESS); + expect(await safe141.nonce()).to.be.eq(1); + }); + it("doesn't touch important storage slots", async () => { const { safe130, From 8f5976bf97a6592f6a1325fe8ae0b886525437e5 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 24 Oct 2023 17:23:55 +0200 Subject: [PATCH 09/11] WIP Update from 1.1.1 --- contracts/libraries/SafeToL2Migration.sol | 89 ++++++++++++++++++----- test/libraries/SafeToL2Migration.spec.ts | 53 +++++++++++++- 2 files changed, 120 insertions(+), 22 deletions(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index 608d282..566b2e0 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -8,6 +8,12 @@ import {Enum} from "../common/Enum.sol"; interface ISafe { // solhint-disable-next-line function VERSION() external view returns (string memory); + + function setFallbackHandler(address handler) external; + + function getOwners() external view returns (address[] memory); + + function getThreshold() external view returns (uint256); } /** @@ -34,6 +40,8 @@ contract SafeToL2Migration is SafeStorage { */ event ChangedMasterCopy(address singleton); + event SafeSetup(address indexed initiator, address[] owners, uint256 threshold, address initializer, address fallbackHandler); + event SafeMultiSigTransaction( address to, uint256 value, @@ -50,6 +58,30 @@ contract SafeToL2Migration is SafeStorage { bytes additionalInfo ); + function migrate(address l2Singleton) private { + singleton = l2Singleton; + + // Simulate a L2 transaction so indexer picks up the Safe + // 0xef2624ae - keccak("migrateToL2(address)") + bytes memory data = abi.encodeWithSelector(0xef2624ae, l2Singleton); + // nonce, sender, threshold + bytes memory additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); + emit SafeMultiSigTransaction( + MIGRATION_SINGLETON, + 0, + data, + Enum.Operation.DelegateCall, + 0, + 0, + 0, + address(0), + address(0), + "", // We cannot detect signatures + additionalInfo + ); + emit ChangedMasterCopy(singleton); + } + /** * @notice Migrate from Safe 1.3.0/1.4.1 Singleton (L1) to the same version provided L2 singleton * Safe is required to have nonce 0 so backend can support it after the migration @@ -58,6 +90,7 @@ contract SafeToL2Migration is SafeStorage { */ function migrateToL2(address l2Singleton) public { require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); + require(address(singleton) != l2Singleton, "Safe is already using the singleton"); // Nonce is increased before executing a tx, so first executed tx will have nonce=1 require(nonce == 1, "Safe must have not executed any tx"); @@ -71,26 +104,44 @@ contract SafeToL2Migration is SafeStorage { "Provided singleton version is not supported" ); - singleton = l2Singleton; + migrate(l2Singleton); + } - // Simulate a L2 transaction so indexer picks up the Safe - // 0xef2624ae - keccak("migrateToL2(address)") - bytes memory data = abi.encodeWithSelector(0xef2624ae, l2Singleton); - // nonce, sender, threshold - bytes memory additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); - emit SafeMultiSigTransaction( - MIGRATION_SINGLETON, - 0, - data, - Enum.Operation.DelegateCall, - 0, - 0, - 0, - address(0), - address(0), - "", // We cannot detect signatures - additionalInfo + function migrateFromV111(address l2Singleton, address fallbackHandler) public { + require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); + + bytes32 oldSingletonVersion = keccak256(abi.encodePacked(ISafe(singleton).VERSION())); + require(oldSingletonVersion == keccak256(abi.encodePacked("1.1.1")), "Provided singleton version is not supported"); + + bytes32 newSingletonVersion = keccak256(abi.encodePacked(ISafe(l2Singleton).VERSION())); + require( + newSingletonVersion == keccak256(abi.encodePacked("1.3.0")) || newSingletonVersion == keccak256(abi.encodePacked("1.4.1")), + "Provided singleton version is not supported" ); - emit ChangedMasterCopy(singleton); + + require(isContract(fallbackHandler), "fallbackHandler is not a contract"); + ISafe safe = ISafe(address(this)); + safe.setFallbackHandler(fallbackHandler); + emit SafeSetup(MIGRATION_SINGLETON, safe.getOwners(), safe.getThreshold(), address(0), fallbackHandler); + migrate(l2Singleton); + } + + /** + * @notice Checks whether an Ethereum address corresponds to a contract or an externally owned account (EOA). + * @param account The Ethereum address to be checked. + * @return A boolean value indicating whether the address is associated with a contract (true) or an EOA (false). + * @dev This function relies on the `extcodesize` assembly opcode to determine whether an address is a contract. + * It may return incorrect results in some edge cases (see documentation for details). + * Developers should use caution when relying on the results of this function for critical decision-making. + */ + function isContract(address account) internal view returns (bool) { + uint256 size; + // solhint-disable-next-line no-inline-assembly + assembly { + size := extcodesize(account) + } + + // If the code size is greater than 0, it is a contract; otherwise, it is an EOA. + return size > 0; } } diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts index cee5196..ccf6e24 100644 --- a/test/libraries/SafeToL2Migration.spec.ts +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -4,7 +4,13 @@ import { AddressZero } from "@ethersproject/constants"; import { getSafeWithSingleton, getSafeSingletonAt, getMock } from "../utils/setup"; import deploymentData from "../json/safeDeployment.json"; import safeRuntimeBytecode from "../json/safeRuntimeBytecode.json"; -import { buildSafeTransaction, executeContractCallWithSigners, executeTxWithSigners } from "../../src/utils/execution"; +import { + buildSafeTransaction, + executeContractCallWithSigners, + executeTx, + executeTxWithSigners, + safeApproveHash, +} from "../../src/utils/execution"; const SAFE_SINGLETON_141_ADDRESS = "0x3E5c63644E683549055b9Be8653de26E0B4CD36E"; @@ -12,6 +18,8 @@ const SAFE_SINGLETON_141_L2_ADDRESS = "0xfb1bffC9d739B8D520DaF37dF666da4C687191E const SAFE_SINGLETON_150_L2_ADDRESS = "0x0Ee37514644683f7EB9745a5726C722DeBa77e52"; +const COMPATIBILITY_FALLBACK_HANDLER_150 = "0x8aa755cB169991fEDC3E306751dCb71964A041c7"; + const FALLBACK_HANDLER_STORAGE_SLOT = "0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5"; const GUARD_STORAGE_SLOT = "0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8"; @@ -26,15 +34,21 @@ describe("SafeToL2Migration library", () => { await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_141_ADDRESS, safeRuntimeBytecode.safe141]); await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_141_L2_ADDRESS, safeRuntimeBytecode.safe141l2]); await hre.network.provider.send("hardhat_setCode", [SAFE_SINGLETON_150_L2_ADDRESS, safeRuntimeBytecode.safe150l2]); + await hre.network.provider.send("hardhat_setCode", [ + COMPATIBILITY_FALLBACK_HANDLER_150, + safeRuntimeBytecode.safe150CompatibilityFallbackHandler, + ]); const signers = await ethers.getSigners(); const [user1] = signers; + const singleton111Address = (await (await user1.sendTransaction({ data: deploymentData.safe111 })).wait())?.contractAddress; const singleton130Address = (await (await user1.sendTransaction({ data: deploymentData.safe130 })).wait())?.contractAddress; const singleton130L2Address = (await (await user1.sendTransaction({ data: deploymentData.safe130l2 })).wait())?.contractAddress; - if (!singleton130Address || !singleton130L2Address) { - throw new Error("Could not deploy Safe130 or Safe130L2"); + if (!singleton111Address || !singleton130Address || !singleton130L2Address) { + throw new Error("Could not deploy Safe111, Safe130 or Safe130L2"); } + const singleton111 = await getSafeSingletonAt(singleton111Address); const singleton130 = await getSafeSingletonAt(singleton130Address); const singleton141 = await getSafeSingletonAt(SAFE_SINGLETON_141_ADDRESS); @@ -69,6 +83,7 @@ describe("SafeToL2Migration library", () => { const safeToL2MigrationContract = await hre.ethers.getContractFactory("SafeToL2Migration"); const migration = await safeToL2MigrationContract.deploy(); return { + safe111: await getSafeWithSingleton(singleton111, [user1.address]), safe130: await getSafeWithSingleton(singleton130, [user1.address]), safe141: await getSafeWithSingleton(singleton141, [user1.address]), safeWith1967Proxy, @@ -227,6 +242,38 @@ describe("SafeToL2Migration library", () => { expect(await safe141.nonce()).to.be.eq(1); }); + it("migrates from singleton 1.1.1 to 1.4.1L2", async () => { + const { + safe111, + migration, + signers: [user1], + } = await setupTests(); + expect(await safe111.VERSION()).eq("1.1.1"); + const safeAddress = await safe111.getAddress(); + + // The emit matcher checks the address, which is the Safe as delegatecall is used + const migrationSafe = migration.attach(safeAddress); + const migrationAddress = await migration.getAddress(); + + const functionName = "migrateFromV111"; + const data = migration.interface.encodeFunctionData(functionName, [ + SAFE_SINGLETON_141_L2_ADDRESS, + COMPATIBILITY_FALLBACK_HANDLER_150, + ]); + const nonce = await safe111.nonce(); + expect(nonce).to.be.eq(0); + const tx = buildSafeTransaction({ to: migrationAddress, data, operation: 1, nonce }); + + expect(await executeTx(safe111, tx, [await safeApproveHash(user1, safe111, tx, true)])) + .to.emit(migrationSafe, "ChangedMasterCopy") + .withArgs(SAFE_SINGLETON_141_L2_ADDRESS); + + expect(await safe111.nonce()).to.be.eq(1); + expect(await safe111.VERSION()).to.be.eq("1.4.1"); + const singletonResp = await user1.call({ to: safeAddress, data: migratedInterface.encodeFunctionData("masterCopy") }); + expect(migratedInterface.decodeFunctionResult("masterCopy", singletonResp)[0]).to.eq(SAFE_SINGLETON_141_L2_ADDRESS); + }); + it("doesn't touch important storage slots", async () => { const { safe130, From b247d60c1c1ff241cbea96c1b83a4d645dcc0b9b Mon Sep 17 00:00:00 2001 From: dodger213 Date: Wed, 25 Oct 2023 12:14:18 +0200 Subject: [PATCH 10/11] Fix tests --- contracts/libraries/SafeToL2Migration.sol | 20 +++++++++++-------- test/libraries/SafeToL2Migration.spec.ts | 24 ++++++++++++++++++++++- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index 566b2e0..51b1b8b 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -58,18 +58,17 @@ contract SafeToL2Migration is SafeStorage { bytes additionalInfo ); - function migrate(address l2Singleton) private { + function migrate(address l2Singleton, bytes memory functionData) private { singleton = l2Singleton; - // Simulate a L2 transaction so indexer picks up the Safe - // 0xef2624ae - keccak("migrateToL2(address)") - bytes memory data = abi.encodeWithSelector(0xef2624ae, l2Singleton); - // nonce, sender, threshold + // Encode nonce, sender, threshold bytes memory additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); + + // Simulate a L2 transaction so indexer picks up the Safe emit SafeMultiSigTransaction( MIGRATION_SINGLETON, 0, - data, + functionData, Enum.Operation.DelegateCall, 0, 0, @@ -104,7 +103,9 @@ contract SafeToL2Migration is SafeStorage { "Provided singleton version is not supported" ); - migrate(l2Singleton); + // 0xef2624ae - keccak("migrateToL2(address)") + bytes memory functionData = abi.encodeWithSelector(0xef2624ae, l2Singleton); + migrate(l2Singleton, functionData); } function migrateFromV111(address l2Singleton, address fallbackHandler) public { @@ -123,7 +124,10 @@ contract SafeToL2Migration is SafeStorage { ISafe safe = ISafe(address(this)); safe.setFallbackHandler(fallbackHandler); emit SafeSetup(MIGRATION_SINGLETON, safe.getOwners(), safe.getThreshold(), address(0), fallbackHandler); - migrate(l2Singleton); + + // 0xd9a20812 - keccak("migrateFromV111(address,address)") + bytes memory functionData = abi.encodeWithSelector(0xd9a20812, l2Singleton, fallbackHandler); + migrate(l2Singleton, functionData); } /** diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts index ccf6e24..1e1fb6b 100644 --- a/test/libraries/SafeToL2Migration.spec.ts +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -262,11 +262,33 @@ describe("SafeToL2Migration library", () => { ]); const nonce = await safe111.nonce(); expect(nonce).to.be.eq(0); + const safeThreshold = await safe111.getThreshold(); + const additionalInfo = hre.ethers.AbiCoder.defaultAbiCoder().encode( + ["uint256", "address", "uint256"], + [0, user1.address, safeThreshold], + ); + const tx = buildSafeTransaction({ to: migrationAddress, data, operation: 1, nonce }); expect(await executeTx(safe111, tx, [await safeApproveHash(user1, safe111, tx, true)])) .to.emit(migrationSafe, "ChangedMasterCopy") - .withArgs(SAFE_SINGLETON_141_L2_ADDRESS); + .withArgs(SAFE_SINGLETON_141_L2_ADDRESS) + .to.emit(migrationSafe, "SafeMultiSigTransaction") + .withArgs( + migrationAddress, + 0, + data, + 1, + 0, + 0, + 0, + AddressZero, + AddressZero, + "0x", // We cannot detect signatures + additionalInfo, + ) + .to.emit(migrationSafe, "SafeSetup") + .withArgs(migrationAddress, await safe111.getOwners(), safeThreshold, AddressZero, COMPATIBILITY_FALLBACK_HANDLER_150); expect(await safe111.nonce()).to.be.eq(1); expect(await safe111.VERSION()).to.be.eq("1.4.1"); From 3a3dd52abc2db43ac985b3fea1c5b869d4c87a5c Mon Sep 17 00:00:00 2001 From: dodger213 Date: Wed, 25 Oct 2023 12:38:48 +0200 Subject: [PATCH 11/11] Organize contract --- contracts/libraries/SafeToL2Migration.sol | 38 +++++++++++++++++------ test/libraries/SafeToL2Migration.spec.ts | 8 ++++- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index 51b1b8b..ddc6d6c 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -17,9 +17,9 @@ interface ISafe { } /** - * @title Migration Contract for updating a Safe from 1.3.0/1.4.1 version to a L2 version. Useful when replaying a Safe from a non L2 network in a L2 network. - * @notice This contract facilitates the migration of a Safe contract from version 1.3.0 to 1.3.0L2 or from 1.4.1 to 1.4.1L2 - * Older versions are not supported + * @title Migration Contract for updating a Safe from 1.1.1/1.3.0/1.4.1 versions to a L2 version. Useful when replaying a Safe from a non L2 network in a L2 network. + * @notice This contract facilitates the migration of a Safe contract from version 1.1.1 to 1.3.0/1.4.1 L2, 1.3.0 to 1.3.0L2 or from 1.4.1 to 1.4.1L2 + * Other versions are not supported * @dev IMPORTANT: The migration will only work with proxies that store the implementation address in the storage slot 0. */ contract SafeToL2Migration is SafeStorage { @@ -58,13 +58,25 @@ contract SafeToL2Migration is SafeStorage { bytes additionalInfo ); + /** + * @notice Modifier to make a function callable via delegatecall only. + * If the function is called via a regular call, it will revert. + */ + modifier onlyDelegateCall() { + require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); + _; + } + + /** + * @dev Internal function with common migration steps, changes the singleton and emits SafeMultiSigTransaction event + */ function migrate(address l2Singleton, bytes memory functionData) private { singleton = l2Singleton; // Encode nonce, sender, threshold bytes memory additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); - // Simulate a L2 transaction so indexer picks up the Safe + // Simulate a L2 transaction so Safe Tx Service indexer picks up the Safe emit SafeMultiSigTransaction( MIGRATION_SINGLETON, 0, @@ -87,9 +99,7 @@ contract SafeToL2Migration is SafeStorage { * @dev This function should only be called via a delegatecall to perform the upgrade. * Singletons versions will be compared, so it implies that contracts exist */ - function migrateToL2(address l2Singleton) public { - require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); - + function migrateToL2(address l2Singleton) public onlyDelegateCall { require(address(singleton) != l2Singleton, "Safe is already using the singleton"); // Nonce is increased before executing a tx, so first executed tx will have nonce=1 require(nonce == 1, "Safe must have not executed any tx"); @@ -108,8 +118,15 @@ contract SafeToL2Migration is SafeStorage { migrate(l2Singleton, functionData); } - function migrateFromV111(address l2Singleton, address fallbackHandler) public { - require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); + /** + * @notice Migrate from Safe 1.1.1 Singleton to 1.3.1 or 1.4.1 L2 + * Safe is required to have nonce 0 so backend can support it after the migration + * @dev This function should only be called via a delegatecall to perform the upgrade. + * Singletons version will be checked, so it implies that contracts exist. + * A valid and compatible fallbackHandler needs to be provided, only existance will be checked. + */ + function migrateFromV111(address l2Singleton, address fallbackHandler) public onlyDelegateCall { + require(isContract(fallbackHandler), "fallbackHandler is not a contract"); bytes32 oldSingletonVersion = keccak256(abi.encodePacked(ISafe(singleton).VERSION())); require(oldSingletonVersion == keccak256(abi.encodePacked("1.1.1")), "Provided singleton version is not supported"); @@ -120,9 +137,10 @@ contract SafeToL2Migration is SafeStorage { "Provided singleton version is not supported" ); - require(isContract(fallbackHandler), "fallbackHandler is not a contract"); ISafe safe = ISafe(address(this)); safe.setFallbackHandler(fallbackHandler); + + // Safes < 1.3.0 did not emit SafeSetup, so Safe Tx Service backend needs the event to index the Safe emit SafeSetup(MIGRATION_SINGLETON, safe.getOwners(), safe.getThreshold(), address(0), fallbackHandler); // 0xd9a20812 - keccak("migrateFromV111(address,address)") diff --git a/test/libraries/SafeToL2Migration.spec.ts b/test/libraries/SafeToL2Migration.spec.ts index 1e1fb6b..66aa7aa 100644 --- a/test/libraries/SafeToL2Migration.spec.ts +++ b/test/libraries/SafeToL2Migration.spec.ts @@ -248,8 +248,11 @@ describe("SafeToL2Migration library", () => { migration, signers: [user1], } = await setupTests(); - expect(await safe111.VERSION()).eq("1.1.1"); const safeAddress = await safe111.getAddress(); + expect(await safe111.VERSION()).eq("1.1.1"); + expect("0x" + (await hre.ethers.provider.getStorage(safeAddress, FALLBACK_HANDLER_STORAGE_SLOT)).slice(26)).to.be.eq( + AddressZero, + ); // The emit matcher checks the address, which is the Safe as delegatecall is used const migrationSafe = migration.attach(safeAddress); @@ -294,6 +297,9 @@ describe("SafeToL2Migration library", () => { expect(await safe111.VERSION()).to.be.eq("1.4.1"); const singletonResp = await user1.call({ to: safeAddress, data: migratedInterface.encodeFunctionData("masterCopy") }); expect(migratedInterface.decodeFunctionResult("masterCopy", singletonResp)[0]).to.eq(SAFE_SINGLETON_141_L2_ADDRESS); + expect("0x" + (await hre.ethers.provider.getStorage(safeAddress, FALLBACK_HANDLER_STORAGE_SLOT)).slice(26)).to.be.eq( + COMPATIBILITY_FALLBACK_HANDLER_150.toLowerCase(), + ); }); it("doesn't touch important storage slots", async () => {