fix(pg): calculate correct Sphinx Module address for non-zero salt nonce

.changeset/clean-rings-play.md

@@ -0,0 +1,5 @@
+---
+'@sphinx-labs/plugins': patch
+---
+
+Calculate correct Sphinx Module address for non-zero salt nonce

packages/contracts/contracts/foundry/SphinxConstants.sol

@@ -1,11 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity >=0.6.2 <0.9.0;
 
-struct SphinxContractInfo {
-  bytes creationCode;
-  address expectedAddress;
-}
-
 contract SphinxConstants {
   address public constant compatibilityFallbackHandlerAddress = 0xf48f2B2d2a534e402487b3ee7C18c33Aec0Fe5e4;
   address public constant multiSendAddress = 0xA238CBeb142c10Ef7Ad8442C6D1f9E89e07e7761;

packages/contracts/contracts/foundry/SphinxUtils.sol

@@ -26,7 +26,7 @@ import {
     Label,
     ExecutionMode
 } from "./SphinxPluginTypes.sol";
-import { SphinxContractInfo, SphinxConstants } from "./SphinxConstants.sol";
+import { SphinxConstants } from "./SphinxConstants.sol";
 import { IGnosisSafeProxyFactory } from "./interfaces/IGnosisSafeProxyFactory.sol";
 import { IGnosisSafe } from "./interfaces/IGnosisSafe.sol";
 import { IMultiSend } from "./interfaces/IMultiSend.sol";
@@ -64,30 +64,6 @@ contract SphinxUtils is SphinxConstants, StdUtils {
         return address(uint160(uint256(ownerBytes32)));
     }
 
-    function create2Deploy(bytes memory _creationCode) public returns (address) {
-        address addr = computeCreate2Address(
-            bytes32(0),
-            keccak256(_creationCode),
-            DETERMINISTIC_DEPLOYMENT_PROXY
-        );
-
-        if (addr.code.length == 0) {
-            bytes memory code = abi.encodePacked(bytes32(0), _creationCode);
-            (bool success, ) = DETERMINISTIC_DEPLOYMENT_PROXY.call(code);
-            require(
-                success,
-                string(
-                    abi.encodePacked(
-                        "failed to deploy contract. expected address: ",
-                        vm.toString(addr)
-                    )
-                )
-            );
-        }
-
-        return addr;
-    }
-
     function inefficientSlice(
         SphinxLeafWithProof[] memory selected,
         uint256 start,
@@ -858,7 +834,22 @@ contract SphinxUtils is SphinxConstants, StdUtils {
 
     function getSphinxModuleAddress(SphinxConfig memory _config) public pure returns (address) {
         address safeProxyAddress = getGnosisSafeProxyAddress(_config);
-        bytes32 salt = keccak256(abi.encode(safeProxyAddress, safeProxyAddress, _config.saltNonce));
+        bytes32 salt = keccak256(
+            abi.encode(
+                safeProxyAddress,
+                safeProxyAddress,
+                // We always set the `saltNonce` of the Sphinx Module to `0` because the
+                // `sphinxConfig.saltNonce` field is only used when deploying the Gnosis Safe. It's
+                // not necessary to include the `saltNonce` here because a new Sphinx Module will be
+                // deployed if the user sets the `sphinxConfig.saltNonce` to a new value and then
+                // deploys a new Gnosis Safe using Sphinx's standard deployment method. A new Sphinx
+                // Module is deployed in this scenario because its address is determined by the
+                // address of the Gnosis Safe. It'd only be necessary to include a `saltNonce` for
+                // the Sphinx Module if a single Gnosis Safe wants to enable multiple Sphinx
+                // Modules, which isn't a feature that we currently support.
+                0
+            )
+        );
         address addr = predictDeterministicAddress(
             sphinxModuleImplAddress,
             salt,

packages/contracts/contracts/foundry/interfaces/IGnosisSafe.sol

@@ -40,6 +40,20 @@ interface IGnosisSafe {
      */
     function getOwners() external view returns (address[] memory);
 
+    /**
+     * @notice Returns an array of modules.
+     *         If all entries fit into a single page, the next pointer will be 0x1.
+     *         If another page is present, next will be the last element of the returned array.
+     * @param start Start of the page. Has to be a module or start pointer (0x1 address)
+     * @param pageSize Maximum number of modules that should be returned. Has to be > 0
+     * @return array Array of modules.
+     * @return next Start of the next page.
+     */
+    function getModulesPaginated(
+        address start,
+        uint256 pageSize
+    ) external view returns (address[] memory array, address next);
+
     /**
      * @notice Returns the number of required confirmations for a Safe transaction aka the threshold.
      * @return Threshold number.

packages/contracts/scripts/write-constants.ts

@@ -54,10 +54,6 @@ const writeConstants = async () => {
   const solidityFile =
     `// SPDX-License-Identifier: MIT\n` +
     `pragma solidity >=0.6.2 <0.9.0;\n\n` +
-    `struct SphinxContractInfo {\n` +
-    `  bytes creationCode;\n` +
-    `  address expectedAddress;\n` +
-    `}\n\n` +
     `contract SphinxConstants {\n` +
     `${Object.entries(constants)
       .map(([name, { type, value }]) => {

packages/plugins/contracts/test/SphinxTestUtils.sol

@@ -4,22 +4,28 @@ pragma solidity ^0.8.0;
 import { Vm } from "sphinx-forge-std/Vm.sol";
 import {
     Network,
-    NetworkInfo
+    NetworkInfo,
+    SphinxConfig
 } from "@sphinx-labs/contracts/contracts/foundry/SphinxPluginTypes.sol";
 import { StdCheatsSafe } from "sphinx-forge-std/StdCheats.sol";
 
-import {
-    SphinxConstants,
-    SphinxContractInfo
-} from "@sphinx-labs/contracts/contracts/foundry/SphinxConstants.sol";
+import { SphinxConstants } from "@sphinx-labs/contracts/contracts/foundry/SphinxConstants.sol";
 import { SphinxUtils } from "@sphinx-labs/contracts/contracts/foundry/SphinxUtils.sol";
+import {
+    IGnosisSafeProxyFactory
+} from "@sphinx-labs/contracts/contracts/foundry/interfaces/IGnosisSafeProxyFactory.sol";
+import {
+    IGnosisSafeProxy
+} from "@sphinx-labs/contracts/contracts/foundry/interfaces/IGnosisSafeProxy.sol";
+import { IGnosisSafe } from "@sphinx-labs/contracts/contracts/foundry/interfaces/IGnosisSafe.sol";
+import { SphinxInitCode, SphinxContractInfo } from "../../test/foundry/SphinxInitCode.sol";
 
 /**
  * @notice Helper functions for testing the Sphinx plugin. This is separate from `SphinxUtils`
  *         because this file only contains helper functions for tests, whereas `SphinxUtils`
  *         contains helper functions for the plugin itself.
  */
-contract SphinxTestUtils is SphinxConstants, StdCheatsSafe, SphinxUtils {
+contract SphinxTestUtils is SphinxConstants, StdCheatsSafe, SphinxUtils, SphinxInitCode {
     // Same as the `RawTx1559` struct defined in StdCheats.sol, except this struct has two
     // addditional fields: `additionalContracts` and `isFixedGasLimit`.
     struct AnvilBroadcastedTxn {
@@ -181,4 +187,75 @@ contract SphinxTestUtils is SphinxConstants, StdCheatsSafe, SphinxUtils {
     ) public pure returns (bytes memory) {
         return b[start:end];
     }
+
+    /**
+     * @notice Executes a single transaction that deploys a Gnosis Safe, deploys a Sphinx Module,
+     *         and enables the Sphinx Module in the Gnosis Safe
+     */
+    function deploySphinxModuleAndGnosisSafe(
+        SphinxConfig memory _config
+    ) public returns (IGnosisSafe) {
+        IGnosisSafeProxyFactory safeProxyFactory = IGnosisSafeProxyFactory(safeFactoryAddress);
+
+        bytes memory safeInitializerData = getGnosisSafeInitializerData(
+            _config.owners,
+            _config.threshold
+        );
+
+        // This is the transaction that deploys the Gnosis Safe, deploys the Sphinx Module,
+        // and enables the Sphinx Module in the Gnosis Safe.
+        IGnosisSafeProxy safeProxy = safeProxyFactory.createProxyWithNonce(
+            safeSingletonAddress,
+            safeInitializerData,
+            _config.saltNonce
+        );
+
+        return IGnosisSafe(address(safeProxy));
+    }
+
+    function deploySphinxSystem() public {
+        vm.etch(
+            DETERMINISTIC_DEPLOYMENT_PROXY,
+            hex"7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf3"
+        );
+
+        SphinxContractInfo[] memory contracts = getSphinxContractInfo();
+        for (uint256 i = 0; i < contracts.length; i++) {
+            SphinxContractInfo memory ct = contracts[i];
+            address addr = create2Deploy(ct.creationCode);
+            require(
+                addr == ct.expectedAddress,
+                string(
+                    abi.encodePacked(
+                        "address mismatch. expected address: ",
+                        vm.toString(ct.expectedAddress)
+                    )
+                )
+            );
+        }
+    }
+
+    function create2Deploy(bytes memory _initCodeWithArgs) public returns (address) {
+        address addr = computeCreate2Address(
+            bytes32(0),
+            keccak256(_initCodeWithArgs),
+            DETERMINISTIC_DEPLOYMENT_PROXY
+        );
+
+        if (addr.code.length == 0) {
+            bytes memory code = abi.encodePacked(bytes32(0), _initCodeWithArgs);
+            (bool success, ) = DETERMINISTIC_DEPLOYMENT_PROXY.call(code);
+            require(
+                success,
+                string(
+                    abi.encodePacked(
+                        "failed to deploy contract. expected address: ",
+                        vm.toString(addr)
+                    )
+                )
+            );
+        }
+
+        return addr;
+    }
 }

packages/plugins/package.json

@@ -14,9 +14,10 @@
   ],
   "scripts": {
     "start": "ts-node ./src/index.ts",
-    "build": "yarn build:ts && yarn build:contracts",
+    "build": "yarn build:ts && yarn write-initcode && yarn build:contracts",
     "build:ts": "tsc -p ./tsconfig.json && yarn copy-hardhat-config && chmod +x dist/cli/index.js",
     "build:contracts": "forge build",
+    "write-initcode": "npx ts-node script/write-initcode.ts > test/foundry/SphinxInitCode.sol",
     "copy-hardhat-config": "cp src/hardhat.config.js dist/hardhat.config.js",
     "clean": "rimraf out/ dist/ ./tsconfig.tsbuildinfo ./forge-cache",
     "test": "yarn kill-nodes && yarn test:solc && yarn test:ts",

packages/plugins/script/write-initcode.ts

@@ -0,0 +1,57 @@
+import { ethers } from 'ethers'
+import { getSphinxConstants, remove0x } from '@sphinx-labs/contracts'
+
+/**
+ * Writes the initcode and expected addresses of all Sphinx system contracts. This allows us to use
+ * the contracts in Forge tests.
+ *
+ * The output can be written to a file by appending this CLI command with: `> fileName.json`.
+ *
+ * NOTE: The generated `SphinxInitCode` contract is for testing purposes only. Including it in the
+ * production Foundry plugin would *significantly* slow down the user's compilation process if
+ * they're using Yul (i.e. `viaIR`) with the optimizer enabled. It's not necessary for us to use the
+ * `SphinxInitCode` contract in the production Foundry plugin because we deploy the Sphinx system
+ * contracts from TypeScript via the `ensureSphinxAndGnosisSafeDeployed` function.
+ */
+const writeConstants = async () => {
+  const sphinxConstants = getSphinxConstants()
+
+  const contractInfo = sphinxConstants.map(
+    ({ artifact, constructorArgs, expectedAddress }) => {
+      const { abi, bytecode } = artifact
+
+      const iface = new ethers.Interface(abi)
+
+      const creationCode = bytecode.concat(
+        remove0x(iface.encodeDeploy(constructorArgs))
+      )
+
+      return { creationCode, expectedAddress }
+    }
+  )
+
+  const solidityFile =
+    `// SPDX-License-Identifier: MIT\n` +
+    `pragma solidity >=0.6.2 <0.9.0;\n\n` +
+    `struct SphinxContractInfo {\n` +
+    `  bytes creationCode;\n` +
+    `  address expectedAddress;\n` +
+    `}\n\n` +
+    `contract SphinxInitCode {\n` +
+    `  function getSphinxContractInfo() public pure returns (SphinxContractInfo[] memory) {\n` +
+    `    SphinxContractInfo[] memory contracts = new SphinxContractInfo[](${contractInfo.length});\n` +
+    `${contractInfo
+      .map(
+        ({ creationCode, expectedAddress }, i) =>
+          `    contracts[${i}] = SphinxContractInfo(hex"${remove0x(
+            creationCode
+          )}", ${expectedAddress});`
+      )
+      .join('\n')}\n` +
+    `    return contracts;\n  }` +
+    `\n}`
+
+  process.stdout.write(solidityFile)
+}
+
+writeConstants()

packages/plugins/src/foundry/decode.ts

@@ -328,7 +328,6 @@ export const makeParsedConfig = (
             input.decodedAction = {
               referenceName: contractName,
               functionName: 'deploy',
-              // TODO: We could probably get the constructor args from the init code with some effort since we have the FQN
               variables: {
                 initCode: initCodeWithArgs,
               },
@@ -359,7 +358,6 @@ export const makeParsedConfig = (
               input.decodedAction = {
                 referenceName: fullyQualifiedName.split(':')[1],
                 functionName: 'deploy',
-                // TODO: We could probably get the constructor args from the init code with some effort since we have the FQN
                 variables: [
                   {
                     initCode: initCodeWithArgs,

packages/plugins/test/foundry/Sphinx.t.sol

@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "sphinx-forge-std/Test.sol";
+import { Sphinx } from "../../contracts/foundry/Sphinx.sol";
+import { IGnosisSafe } from "@sphinx-labs/contracts/contracts/foundry/interfaces/IGnosisSafe.sol";
+import { SphinxTestUtils } from "../../contracts/test/SphinxTestUtils.sol";
+
+contract Sphinx_Test is Test, Sphinx, SphinxTestUtils {
+
+    function setUp() public {
+        deploySphinxSystem();
+
+        sphinxConfig.projectName = "test_project";
+        sphinxConfig.owners = [0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266];
+        sphinxConfig.threshold = 1;
+        sphinxConfig.orgId = "test-org-id";
+    }
+
+    function test_sphinxModule_success_standard() external {
+        IGnosisSafe safeProxy = IGnosisSafe(deploySphinxModuleAndGnosisSafe(sphinxConfig));
+
+        (address[] memory modules, ) = safeProxy.getModulesPaginated(address(0x1), 1);
+        address sphinxModule = modules[0];
+
+        address expectedAddress = this.sphinxModule();
+
+        assertEq(expectedAddress, sphinxModule);
+    }
+
+    function test_sphinxModule_success_nonZeroSaltNonce() external {
+        sphinxConfig.saltNonce = 1;
+        IGnosisSafe safeProxy = IGnosisSafe(deploySphinxModuleAndGnosisSafe(sphinxConfig));
+
+        (address[] memory modules, ) = safeProxy.getModulesPaginated(address(0x1), 1);
+        address sphinxModule = modules[0];
+
+        address expectedAddress = this.sphinxModule();
+
+        assertEq(expectedAddress, sphinxModule);
+    }
+}