Merge branch 'main' of https://github.com/safe-global/safe-contracts …

…into feat/improve-proxy-factory

benchmark/utils/setup.ts

@@ -23,7 +23,7 @@ const generateTarget = async (owners: Wallet[], threshold: number, guardAddress:
 export const configs = [
     { name: "single owner", signers: [user1], threshold: 1 },
     { name: "single owner and guard", signers: [user1], threshold: 1, useGuard: true },
-    { name: "2 out of 23", signers: [user1, user2], threshold: 2 },
+    { name: "2 out of 2", signers: [user1, user2], threshold: 2 },
     { name: "3 out of 3", signers: [user1, user2, user3], threshold: 3 },
     { name: "3 out of 5", signers: [user1, user2, user3, user4, user5], threshold: 3 },
 ]
@@ -33,7 +33,7 @@ export const setupBenchmarkContracts = (benchmarkFixture?: () => Promise<any>, l
         await deployments.fixture();
         const guardFactory = await hre.ethers.getContractFactory("DelegateCallTransactionGuard");
         const guard = await guardFactory.deploy(AddressZero)
-        const targets = []
+        const targets: Contract[] = []
         for (const config of configs) {
             targets.push(await generateTarget(config.signers, config.threshold, config.useGuard ? guard.address : AddressZero, logGasUsage))
         }

contracts/proxies/GnosisSafeProxyFactory.sol

@@ -22,25 +22,13 @@ contract GnosisSafeProxyFactory {
     /// @dev Allows to create new proxy contact using CREATE2 but it doesn't run the initializer.
     ///      This method is only meant as an utility to be called from other methods
     /// @param _singleton Address of singleton contract.
-    /// @param initializer Payload for message call sent to new proxy contract.
-    /// @param saltNonce Nonce that will be used to generate the salt to calculate the address of the new proxy contract.
-    /// @param includeChainIdInSalt If true, the chain id will be included in the salt to calculate the address of the new proxy contract.
-    function deployProxyWithNonce(
+    /// @param salt Create2 salt to use for calculating the address of the new proxy contract.
+    function deployProxy(
         address _singleton,
-        bytes memory initializer,
-        uint256 saltNonce,
-        bool includeChainIdInSalt
+        bytes32 salt
     ) internal returns (GnosisSafeProxy proxy) {
         require(isContract(_singleton), "Singleton contract not deployed");
 
-        // If the initializer changes the proxy address should change too. Hashing the initializer data is cheaper than just concatinating it
-        bytes32 salt;
-        if (includeChainIdInSalt) {
-            salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce, getChainId()));
-        } else {
-            salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce));
-        }
-
         bytes memory deploymentData = abi.encodePacked(type(GnosisSafeProxy).creationCode, uint256(uint160(_singleton)));
         // solhint-disable-next-line no-inline-assembly
         assembly {
@@ -49,7 +37,7 @@ contract GnosisSafeProxyFactory {
         require(address(proxy) != address(0), "Create2 call failed");
     }
 
-    /// @dev Allows to create new proxy contact and execute a message call to the new proxy within one transaction.
+    /// @dev Allows to create a new proxy contact and execute a message call to the new proxy within one transaction.
     /// @param _singleton Address of singleton contract.
     /// @param initializer Payload for message call sent to new proxy contract.
     /// @param saltNonce Nonce that will be used to generate the salt to calculate the address of the new proxy contract.
@@ -58,7 +46,9 @@ contract GnosisSafeProxyFactory {
         bytes memory initializer,
         uint256 saltNonce
     ) public returns (GnosisSafeProxy proxy) {
-        proxy = deployProxyWithNonce(_singleton, initializer, saltNonce, false);
+        // If the initializer changes the proxy address should change too. Hashing the initializer data is cheaper than just concatinating it
+        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce));
+        proxy = deployProxy(_singleton, salt);
         if (initializer.length > 0)
             // solhint-disable-next-line no-inline-assembly
             assembly {
@@ -69,7 +59,8 @@ contract GnosisSafeProxyFactory {
         emit ProxyCreation(proxy, _singleton);
     }
 
-    /// @dev Same as `createProxyWithNonce` but includes the chain id in the salt to calculate the address of the new proxy contract.
+    /// @dev Allows to create new proxy contact that should exist only on 1 network (e.g. specific governance or admin accounts)
+    ///      by including the chain id in the create2 salt. Such proxies cannot be created on other networks by replaying the transaction.
     /// @param _singleton Address of singleton contract.
     /// @param initializer Payload for message call sent to new proxy contract.
     /// @param saltNonce Nonce that will be used to generate the salt to calculate the address of the new proxy contract.
@@ -78,7 +69,9 @@ contract GnosisSafeProxyFactory {
         bytes memory initializer,
         uint256 saltNonce
     ) public returns (GnosisSafeProxy proxy) {
-        proxy = deployProxyWithNonce(_singleton, initializer, saltNonce, true);
+        // If the initializer changes the proxy address should change too. Hashing the initializer data is cheaper than just concatinating it
+        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce, getChainId()));
+        proxy = deployProxy(_singleton, salt);
         if (initializer.length > 0)
             // solhint-disable-next-line no-inline-assembly
             assembly {
@@ -116,7 +109,8 @@ contract GnosisSafeProxyFactory {
         bytes calldata initializer,
         uint256 saltNonce
     ) external returns (GnosisSafeProxy proxy) {
-        proxy = deployProxyWithNonce(_singleton, initializer, saltNonce, false);
+        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce));
+        proxy = deployProxy(_singleton, salt);
         revert(string(abi.encodePacked(proxy)));
     }
 
@@ -131,7 +125,8 @@ contract GnosisSafeProxyFactory {
         bytes calldata initializer,
         uint256 saltNonce
     ) external returns (GnosisSafeProxy proxy) {
-        proxy = deployProxyWithNonce(_singleton, initializer, saltNonce, true);
+        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce, getChainId()));
+        proxy = deployProxy(_singleton, salt);
         revert(string(abi.encodePacked(proxy)));
     }
 

test/factory/ProxyFactory.spec.ts

@@ -53,8 +53,9 @@ describe("ProxyFactory", async () => {
 
         it('should revert if singleton address is not a contract', async () => {
             const { factory } = await setupTests()
+            const randomAddress = ethers.utils.getAddress(ethers.utils.hexlify(ethers.utils.randomBytes(20)))
             await expect(
-                factory.createProxyWithNonce(AddressZero, "0x", saltNonce)
+                factory.createProxyWithNonce(randomAddress, "0x", saltNonce)
             ).to.be.revertedWith("Singleton contract not deployed")
         })