Updating CHANGELOG for v1.5.0 (#854)

This pull request includes updates to the repository to reflect the new version 1.5.0, with a few improvements and updates to the documentation, contracts, and tests. ### Version Updates: * Updated the version number in `Safe.sol` from `1.4.1` to `1.5.0`. ### Documentation Updates: * Updated CHANGELOG to new addresses with `v1.5.0` and changes included with that version. * Updated `safe_tx_gas.md` to reflect changes in the `Safe.sol` contract, including detailed inline assembly code for error handling. ### Test Updates: * Updated migration tests to reflect the new version `1.5.0` in `UpgradeFromSafe111.spec.ts` and `UpgradeFromSafe120.spec.ts`. [[1]](diffhunk://#diff-fabd1eff3a7e83fccbd17c2ddd31b90179573d48337eb2d4af14cf7cdc45e68cL28-R28) [[2]](diffhunk://#diff-fabd1eff3a7e83fccbd17c2ddd31b90179573d48337eb2d4af14cf7cdc45e68cL39-R42) [[3]](diffhunk://#diff-77a3075c46c527d198eb5d1ccd5c10f9e0fae972ae7e45edc7ced44bcf6883fdL29-R29) [[4]](diffhunk://#diff-77a3075c46c527d198eb5d1ccd5c10f9e0fae972ae7e45edc7ced44bcf6883fdL40-R43) * Added new migration test (this is just added as a complimentary check, detailed checks are already added otherwise with Safe Migration Tests) to check `v1.3.0` & `v1.4.1` to `v1.5.1`
safe-global · Oct 31, 2024 · 3bd2d4b · 3bd2d4b
1 parent 76ea23d
commit 3bd2d4b
Show file tree

Hide file tree

Showing 12 changed files with 432 additions and 71 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
diff --git a/contracts/Safe.sol b/contracts/Safe.sol
@@ -49,7 +49,7 @@ contract Safe is
 {
     using SafeMath for uint256;
 
-    string public constant override VERSION = "1.4.1";
+    string public constant override VERSION = "1.5.0";
 
     // keccak256(
     //     "EIP712Domain(uint256 chainId,address verifyingContract)"

diff --git a/docs/error_codes.md b/docs/error_codes.md
@@ -9,7 +9,7 @@
 - `GS010`: `Not enough gas to execute Safe transaction`
 - `GS011`: `Could not pay gas costs with ether`
 - `GS012`: `Could not pay gas costs with token`
-- `GS013`: `Safe transaction failed when gasPrice and safeTxGas were 0`
+- `GS013`: `Safe transaction failed when gasPrice and safeTxGas were 0` (Deprecated in `v1.5.0`)
 
 ### General signature validation related
 - `GS020`: `Signatures data too short`

diff --git a/docs/safe_tx_gas.md b/docs/safe_tx_gas.md
@@ -49,10 +49,19 @@ To make it easier to set the `safeTxGas` value a change has been made with the 1
 
 **When `safeTxGas` is set to `0`, the Safe contract will revert if the internal Safe transaction fails** (see [#274](https://github.com/safe-global/safe-smart-account/issues/274))
 
-That means if `safeTxGas` is set to `0` the Safe contract sends along all the available gas when performing the internal Safe transaction. If that transaction fails the Safe will revert and therefore also undo all State changes. This can be seen in [`Safe.sol`](https://github.com/safe-global/safe-smart-account/blob/main/contracts/Safe.sol#L178-L180):
+That means if `safeTxGas` is set to `0` the Safe contract sends along all the available gas when performing the internal Safe transaction. If that transaction fails the Safe will revert and therefore also undo all State changes. This can be seen in [`Safe.sol`](https://github.com/safe-global/safe-smart-account/blob/main/contracts/Safe.sol#L178-L187):
 
 ```js
-require(success || safeTxGas != 0 || gasPrice != 0, "GS013");
+if (!success && safeTxGas == 0 && gasPrice == 0) {
+    /* solhint-disable no-inline-assembly */
+    /// @solidity memory-safe-assembly
+    assembly {
+        let p := mload(0x40)
+        returndatacopy(p, 0, returndatasize())
+        revert(p, returndatasize())
+    }
+    /* solhint-enable no-inline-assembly */
+}
 ```
 
 As this also means that the `nonce` for this transaction is **not** used, **it is possible to retry the transaction in the future**.

diff --git a/hardhat.config.ts b/hardhat.config.ts
@@ -166,6 +166,7 @@ if (NODE_URL) {
     userConfig.networks!.custom = {
         ...sharedNetworkConfig,
         url: NODE_URL,
+        zksync: HARDHAT_ENABLE_ZKSYNC === "1",
     };
 }
 export default userConfig;
diff --git a/test/guards/ReentrancyTransactionGuard.spec.ts b/test/guards/ReentrancyTransactionGuard.spec.ts
@@ -73,7 +73,6 @@ describe("ReentrancyTransactionGuard", () => {
             const signatures = [await safeSignTypedData(user1, safeAddress, safeTx)];
             const signatureBytes = buildSignatureBytes(signatures);
 
-            // We should revert with GS013 as the internal tx is reverted because of the reentrancy guard
             await expect(
                 executeContractCallWithSigners(
                     safe,

diff --git a/test/migration/UpgradeFromSafe111.spec.ts b/test/migration/UpgradeFromSafe111.spec.ts
@@ -25,7 +25,7 @@ describe("Upgrade from Safe 1.1.1", () => {
         const mockAddress = await mock.getAddress();
         const singleton111 = (await (await user1.sendTransaction({ data: deploymentData.safe111 })).wait())?.contractAddress;
         if (!singleton111) throw new Error("Could not deploy Safe 1.1.1");
-        const singleton140 = await (await getSafeSingleton()).getAddress();
+        const singleton150 = await (await getSafeSingleton()).getAddress();
         const factory = await getFactory();
         const saltNonce = 42;
         const proxyAddress = await calculateProxyAddress(factory, singleton111, "0x", saltNonce);
@@ -36,10 +36,10 @@ describe("Upgrade from Safe 1.1.1", () => {
 
         expect(await safe.VERSION()).to.be.eq("1.1.1");
         const nonce = await safe.nonce();
-        const data = ChangeMasterCopyInterface.encodeFunctionData("changeMasterCopy", [singleton140]);
+        const data = ChangeMasterCopyInterface.encodeFunctionData("changeMasterCopy", [singleton150]);
         const tx = buildSafeTransaction({ to: await safe.getAddress(), data, nonce });
         await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
-        expect(await safe.VERSION()).to.be.eq("1.4.1");
+        expect(await safe.VERSION()).to.be.eq("1.5.0");
 
         return {
             migratedSafe: safe,

diff --git a/test/migration/UpgradeFromSafe120.spec.ts b/test/migration/UpgradeFromSafe120.spec.ts
@@ -26,7 +26,7 @@ describe("Upgrade from Safe 1.2.0", () => {
         const singleton120 = (await (await user1.sendTransaction({ data: deploymentData.safe120 })).wait())?.contractAddress;
         if (!singleton120) throw new Error("Could not deploy Safe 1.2.0");
 
-        const singleton140 = await (await getSafeSingleton()).getAddress();
+        const singleton150 = await (await getSafeSingleton()).getAddress();
         const factory = await getFactory();
         const saltNonce = 42;
         const proxyAddress = await calculateProxyAddress(factory, singleton120, "0x", saltNonce);
@@ -37,10 +37,10 @@ describe("Upgrade from Safe 1.2.0", () => {
 
         expect(await safe.VERSION()).to.be.eq("1.2.0");
         const nonce = await safe.nonce();
-        const data = ChangeMasterCopyInterface.encodeFunctionData("changeMasterCopy", [singleton140]);
+        const data = ChangeMasterCopyInterface.encodeFunctionData("changeMasterCopy", [singleton150]);
         const tx = buildSafeTransaction({ to: await safe.getAddress(), data, nonce });
         await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
-        expect(await safe.VERSION()).to.be.eq("1.4.1");
+        expect(await safe.VERSION()).to.be.eq("1.5.0");
 
         return {
             migratedSafe: safe,

diff --git a/test/migration/UpgradeFromSafe130.spec.ts b/test/migration/UpgradeFromSafe130.spec.ts
@@ -0,0 +1,54 @@
+import { expect } from "chai";
+import hre, { deployments } from "hardhat";
+import { AddressZero } from "@ethersproject/constants";
+import { getFactory, getMock, getMultiSend } from "../utils/setup";
+import { buildSafeTransaction, executeTx, safeApproveHash } from "../../src/utils/execution";
+import { verificationTests } from "./subTests.spec";
+import deploymentData from "../json/safeDeployment.json";
+import { calculateProxyAddress } from "../../src/utils/proxies";
+
+describe("Upgrade from Safe 1.3.0", () => {
+    before(function () {
+        /**
+         * ## There's no safe 1.3.0 on zkSync, so we skip this test
+         */
+        if (hre.network.zksync) this.skip();
+    });
+
+    // We migrate the Safe and run the verification tests
+    const setupTests = deployments.createFixture(async ({ deployments }) => {
+        await deployments.fixture();
+        const mock = await getMock();
+        const mockAddress = await mock.getAddress();
+        const [user1] = await hre.ethers.getSigners();
+        const singleton130 = (await (await user1.sendTransaction({ data: deploymentData.safe130.evm })).wait())?.contractAddress;
+        if (!singleton130) throw new Error("Could not deploy Safe 1.3.0");
+
+        const factory = await getFactory();
+        const saltNonce = 42;
+        const proxyAddress = await calculateProxyAddress(factory, singleton130, "0x", saltNonce);
+        await factory.createProxyWithNonce(singleton130, "0x", saltNonce).then((tx) => tx.wait());
+
+        const safe = await hre.ethers.getContractAt("Safe", proxyAddress);
+        await safe.setup([user1.address], 1, AddressZero, "0x", mockAddress, AddressZero, 0, AddressZero);
+
+        expect(await safe.VERSION()).to.be.eq("1.3.0");
+        const safeMigrationDeployment = await deployments.get("SafeMigration");
+        const safeMigration = await hre.ethers.getContractAt("SafeMigration", safeMigrationDeployment.address);
+        const nonce = await safe.nonce();
+        const data = safeMigration.interface.encodeFunctionData("migrateSingleton");
+        const tx = buildSafeTransaction({ to: await safeMigration.getAddress(), data, nonce, operation: 1 });
+        await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
+        expect(await safe.VERSION()).to.be.eq("1.5.0");
+
+        return {
+            migratedSafe: safe,
+            mock,
+            multiSend: await getMultiSend(),
+        };
+    });
+
+    it("passes the Safe 1.3.0 tests", async () => {
+        await verificationTests(setupTests);
+    });
+});
diff --git a/test/migration/UpgradeFromSafe130L2.spec.ts b/test/migration/UpgradeFromSafe130L2.spec.ts
@@ -0,0 +1,54 @@
+import { expect } from "chai";
+import hre, { deployments } from "hardhat";
+import { AddressZero } from "@ethersproject/constants";
+import { getFactory, getMock, getMultiSend } from "../utils/setup";
+import { buildSafeTransaction, executeTx, safeApproveHash } from "../../src/utils/execution";
+import { verificationTests } from "./subTests.spec";
+import deploymentData from "../json/safeDeployment.json";
+import { calculateProxyAddress } from "../../src/utils/proxies";
+
+describe("Upgrade from Safe 1.3.0 L2", () => {
+    before(function () {
+        /**
+         * ## There's no safe 1.3.0 on zkSync, so we skip this test
+         */
+        if (hre.network.zksync) this.skip();
+    });
+
+    // We migrate the Safe and run the verification tests
+    const setupTests = deployments.createFixture(async ({ deployments }) => {
+        await deployments.fixture();
+        const mock = await getMock();
+        const mockAddress = await mock.getAddress();
+        const [user1] = await hre.ethers.getSigners();
+        const singleton130L2 = (await (await user1.sendTransaction({ data: deploymentData.safe130l2.evm })).wait())?.contractAddress;
+        if (!singleton130L2) throw new Error("Could not deploy Safe 1.3.0 L2");
+
+        const factory = await getFactory();
+        const saltNonce = 42;
+        const proxyAddress = await calculateProxyAddress(factory, singleton130L2, "0x", saltNonce);
+        await factory.createProxyWithNonce(singleton130L2, "0x", saltNonce).then((tx) => tx.wait());
+
+        const safe = await hre.ethers.getContractAt("Safe", proxyAddress);
+        await safe.setup([user1.address], 1, AddressZero, "0x", mockAddress, AddressZero, 0, AddressZero);
+
+        expect(await safe.VERSION()).to.be.eq("1.3.0");
+        const safeMigrationDeployment = await deployments.get("SafeMigration");
+        const safeMigration = await hre.ethers.getContractAt("SafeMigration", safeMigrationDeployment.address);
+        const nonce = await safe.nonce();
+        const data = safeMigration.interface.encodeFunctionData("migrateSingleton");
+        const tx = buildSafeTransaction({ to: await safeMigration.getAddress(), data, nonce, operation: 1 });
+        await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
+        expect(await safe.VERSION()).to.be.eq("1.5.0");
+
+        return {
+            migratedSafe: safe,
+            mock,
+            multiSend: await getMultiSend(),
+        };
+    });
+
+    it("passes the Safe 1.3.0 tests", async () => {
+        await verificationTests(setupTests);
+    });
+});
diff --git a/test/migration/UpgradeFromSafe141.spec.ts b/test/migration/UpgradeFromSafe141.spec.ts
@@ -0,0 +1,49 @@
+import { expect } from "chai";
+import hre, { deployments } from "hardhat";
+import { AddressZero } from "@ethersproject/constants";
+import { getAbi, getFactory, getMock, getMultiSend } from "../utils/setup";
+import { buildSafeTransaction, executeTx, safeApproveHash } from "../../src/utils/execution";
+import { verificationTests } from "./subTests.spec";
+import deploymentData from "../json/safeDeployment.json";
+import { calculateProxyAddress } from "../../src/utils/proxies";
+
+describe("Upgrade from Safe 1.4.1", () => {
+    // We migrate the Safe and run the verification tests
+    const setupTests = deployments.createFixture(async ({ deployments }) => {
+        await deployments.fixture();
+        const mock = await getMock();
+        const mockAddress = await mock.getAddress();
+        const [user1] = await hre.ethers.getSigners();
+        const safeDeploymentData = hre.network.zksync ? deploymentData.safe141.zksync : deploymentData.safe141.evm;
+        const safeContractFactory = new hre.ethers.ContractFactory(await getAbi("Safe"), safeDeploymentData, user1);
+        const singleton141 = await (await safeContractFactory.deploy()).getAddress();
+        if (!singleton141) throw new Error("Could not deploy Safe 1.4.1");
+
+        const factory = await getFactory();
+        const saltNonce = 42;
+        const proxyAddress = await calculateProxyAddress(factory, singleton141, "0x", saltNonce, hre.network.zksync);
+        await factory.createProxyWithNonce(singleton141, "0x", saltNonce).then((tx) => tx.wait());
+
+        const safe = await hre.ethers.getContractAt("Safe", proxyAddress);
+        await safe.setup([user1.address], 1, AddressZero, "0x", mockAddress, AddressZero, 0, AddressZero);
+
+        expect(await safe.VERSION()).to.be.eq("1.4.1");
+        const safeMigrationDeployment = await deployments.get("SafeMigration");
+        const safeMigration = await hre.ethers.getContractAt("SafeMigration", safeMigrationDeployment.address);
+        const nonce = await safe.nonce();
+        const data = safeMigration.interface.encodeFunctionData("migrateSingleton");
+        const tx = buildSafeTransaction({ to: await safeMigration.getAddress(), data, nonce, operation: 1 });
+        await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
+        expect(await safe.VERSION()).to.be.eq("1.5.0");
+
+        return {
+            migratedSafe: safe,
+            mock,
+            multiSend: await getMultiSend(),
+        };
+    });
+
+    it("passes the Safe 1.4.1 tests", async () => {
+        await verificationTests(setupTests);
+    });
+});
diff --git a/test/migration/UpgradeFromSafe141L2.spec.ts b/test/migration/UpgradeFromSafe141L2.spec.ts
@@ -0,0 +1,49 @@
+import { expect } from "chai";
+import hre, { deployments } from "hardhat";
+import { AddressZero } from "@ethersproject/constants";
+import { getAbi, getFactory, getMock, getMultiSend } from "../utils/setup";
+import { buildSafeTransaction, executeTx, safeApproveHash } from "../../src/utils/execution";
+import { verificationTests } from "./subTests.spec";
+import deploymentData from "../json/safeDeployment.json";
+import { calculateProxyAddress } from "../../src/utils/proxies";
+
+describe("Upgrade from Safe 1.4.1 L2", () => {
+    // We migrate the Safe and run the verification tests
+    const setupTests = deployments.createFixture(async ({ deployments }) => {
+        await deployments.fixture();
+        const mock = await getMock();
+        const mockAddress = await mock.getAddress();
+        const [user1] = await hre.ethers.getSigners();
+        const safeDeploymentData = hre.network.zksync ? deploymentData.safe141l2.zksync : deploymentData.safe141l2.evm;
+        const safeContractFactory = new hre.ethers.ContractFactory(await getAbi("Safe"), safeDeploymentData, user1);
+        const singleton141L2 = await (await safeContractFactory.deploy()).getAddress();
+        if (!singleton141L2) throw new Error("Could not deploy Safe 1.4.1 L2");
+
+        const factory = await getFactory();
+        const saltNonce = 42;
+        const proxyAddress = await calculateProxyAddress(factory, singleton141L2, "0x", saltNonce, hre.network.zksync);
+        await factory.createProxyWithNonce(singleton141L2, "0x", saltNonce).then((tx) => tx.wait());
+
+        const safe = await hre.ethers.getContractAt("Safe", proxyAddress);
+        await safe.setup([user1.address], 1, AddressZero, "0x", mockAddress, AddressZero, 0, AddressZero);
+
+        expect(await safe.VERSION()).to.be.eq("1.4.1");
+        const safeMigrationDeployment = await deployments.get("SafeMigration");
+        const safeMigration = await hre.ethers.getContractAt("SafeMigration", safeMigrationDeployment.address);
+        const nonce = await safe.nonce();
+        const data = safeMigration.interface.encodeFunctionData("migrateSingleton");
+        const tx = buildSafeTransaction({ to: await safeMigration.getAddress(), data, nonce, operation: 1 });
+        await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
+        expect(await safe.VERSION()).to.be.eq("1.5.0");
+
+        return {
+            migratedSafe: safe,
+            mock,
+            multiSend: await getMultiSend(),
+        };
+    });
+
+    it("passes the Safe 1.4.1 tests", async () => {
+        await verificationTests(setupTests);
+    });
+});