Skip to content

Commit

Permalink
Enhance 1.4.1 contract checks for support proxy contract
Browse files Browse the repository at this point in the history
  • Loading branch information
@mmv08
mmv08 committed Oct 23, 2023
1 parent 810fad9 commit bd26f51
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 16 deletions.
33 changes: 21 additions & 12 deletions contracts/libraries/Safe130To141Migration.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@ interface ISafe {
* @title Migration Contract for Safe Upgrade
* @notice This contract facilitates the migration of a Safe contract from version 1.3.0 to 1.4.1.
* The older versions should use built-in upgrade methods.
* @dev IMPORTANT: The migration will only work with proxies that store the implementation address in the storage slot 0.
* @dev IMPORTANT: The library is intended to be used with the Safe standard proxy that stores the singleton address
* at the storage slot 0. Use at your own risk with custom proxy implementations. The library will block calls
* if the address stored at the storage slot 0 is not a contract. The check is implemented in the `checkCurrentSingleton` function.
*/
contract Safe130To141Migration is SafeStorage {
// Address of this contract
address public immutable MIGRATION_SINGLETON;

// Address of Safe contract version 1.4.1 Singleton
address public constant SAFE_141_SINGLETON = address(0x41675C099F32341bf84BFc5382aF534df5C7461a);

Expand All @@ -32,13 +31,27 @@ contract Safe130To141Migration is SafeStorage {
* @dev Initializes the migrationSingleton with the contract's own address.
*/
constructor() {
MIGRATION_SINGLETON = address(this);

require(isContract(SAFE_141_SINGLETON), "Safe 1.4.1 Singleton is not deployed");
require(isContract(SAFE_141_SINGLETON_L2), "Safe 1.4.1 Singleton (L2) is not deployed");
require(isContract(SAFE_141_FALLBACK_HANDLER), "Safe 1.4.1 Fallback Handler is not deployed");
}

/**
* @dev Checks whether the current singleton address is a contract.
* The canonical Safe proxy stores the singleton address at the storage slot 0.
* This migration contract doesn't define it's own storage, so the fact that the storage slot 0 is defined
* and is a contract ensures that the contract was called with a DELEGATECALL.
*/
function checkCurrentSingleton() internal view {
require(isContract(singleton), "Unsupported proxy contract");
}

/// @dev Modifier that checks whether the current singleton address is a contract.
modifier validSingletonOnly() {
checkCurrentSingleton();
_;
}

/**
* @notice Event indicating a change of master copy address.
* @param singleton New master copy address
Expand All @@ -49,9 +62,7 @@ contract Safe130To141Migration is SafeStorage {
* @notice Migrate to Safe 1.4.1 Singleton (L1) at `SAFE_141_SINGLETON`
* @dev This function should only be called via a delegatecall to perform the upgrade.
*/
function migrate() public {
require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall");

function migrate() public validSingletonOnly {
singleton = SAFE_141_SINGLETON;

emit ChangedMasterCopy(singleton);
Expand All @@ -71,9 +82,7 @@ contract Safe130To141Migration is SafeStorage {
* @notice Migrate to Safe 1.4.1 Singleton (L2) at `SAFE_141_SINGLETON_L2`
* @dev This function should only be called via a delegatecall to perform the upgrade.
*/
function migrateL2() public {
require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall");

function migrateL2() public validSingletonOnly {
singleton = SAFE_141_SINGLETON_L2;

emit ChangedMasterCopy(singleton);
Expand Down
79 changes: 75 additions & 4 deletions test/libraries/Safe130To141Migration.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect } from "chai";
import hre, { ethers, deployments } from "hardhat";
import { AddressZero } from "@ethersproject/constants";
import { getSafeWithSingleton, migrationContractFrom130To141, getSafeSingletonAt } from "../utils/setup";
import deploymentData from "../json/safeDeployment.json";
import safeRuntimeBytecode from "../json/safeRuntimeBytecode.json";
Expand Down Expand Up @@ -38,11 +39,33 @@ describe("Safe130To141Migration library", () => {
const singleton130L2 = await getSafeSingletonAt(singleton130L2Address);

const migration = await (await migrationContractFrom130To141()).deploy();

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()),
);
return {
safe130: await getSafeWithSingleton(singleton130, [user1.address]),
safe130l2: await getSafeWithSingleton(singleton130L2, [user1.address]),
migration,
signers,
safeWith1967Proxy,
};
});

Expand All @@ -56,7 +79,19 @@ describe("Safe130To141Migration library", () => {
describe("migrate", () => {
it("can only be called from Safe itself", async () => {
const { migration } = await setupTests();
await expect(migration.migrate()).to.be.revertedWith("Migration should only be called via delegatecall");
await expect(migration.migrate()).to.be.revertedWith("Unsupported proxy contract");
});

it("reverts if the singleton in storage at slot 0 is not a contract", async () => {
const {
migration,
safeWith1967Proxy,
signers: [user1],
} = await setupTests();

await expect(executeContractCallWithSigners(safeWith1967Proxy, migration, "migrate", [], [user1], true)).to.be.revertedWith(
"GS013",
);
});

it("can migrate", async () => {
Expand Down Expand Up @@ -100,7 +135,19 @@ describe("Safe130To141Migration library", () => {
describe("migrateWithFallbackHandler", () => {
it("can only be called from Safe itself", async () => {
const { migration } = await setupTests();
await expect(migration.migrateWithFallbackHandler()).to.be.revertedWith("Migration should only be called via delegatecall");
await expect(migration.migrateWithFallbackHandler()).to.be.revertedWith("Unsupported proxy contract");
});

it("reverts if the singleton in storage at slot 0 is not a contract", async () => {
const {
migration,
safeWith1967Proxy,
signers: [user1],
} = await setupTests();

await expect(
executeContractCallWithSigners(safeWith1967Proxy, migration, "migrateWithFallbackHandler", [], [user1], true),
).to.be.revertedWith("GS013");
});

it("can migrate", async () => {
Expand Down Expand Up @@ -150,7 +197,31 @@ describe("Safe130To141Migration library", () => {
describe("migrateL2", () => {
it("can only be called from Safe itself", async () => {
const { migration } = await setupTests();
await expect(migration.migrateL2()).to.be.revertedWith("Migration should only be called via delegatecall");
await expect(migration.migrateL2()).to.be.revertedWith("Unsupported proxy contract");
});

it("reverts if the singleton in storage at slot 0 is not a contract", async () => {
const {
migration,
safeWith1967Proxy,
signers: [user1],
} = await setupTests();

await expect(
executeContractCallWithSigners(safeWith1967Proxy, migration, "migrateL2WithFallbackHandler", [], [user1], true),
).to.be.revertedWith("GS013");
});

it("reverts if the singleton in storage at slot 0 is not a contract", async () => {
const {
migration,
safeWith1967Proxy,
signers: [user1],
} = await setupTests();

await expect(executeContractCallWithSigners(safeWith1967Proxy, migration, "migrateL2", [], [user1], true)).to.be.revertedWith(
"GS013",
);
});

it("can migrate", async () => {
Expand Down Expand Up @@ -194,7 +265,7 @@ describe("Safe130To141Migration library", () => {
describe("migrateL2WithFallbackHandler", () => {
it("can only be called from Safe itself", async () => {
const { migration } = await setupTests();
await expect(migration.migrateL2WithFallbackHandler()).to.be.revertedWith("Migration should only be called via delegatecall");
await expect(migration.migrateL2WithFallbackHandler()).to.be.revertedWith("Unsupported proxy contract");
});

it("can migrate", async () => {
Expand Down

0 comments on commit bd26f51

Please sign in to comment.