Feat/fallback selector security check

.github/workflows/ci.yml

@@ -1,6 +1,6 @@
 name: Unified CI Workflow
 
-on: [push, pull_request]
+on: pull_request
 
 permissions: write-all
 
@@ -171,4 +171,4 @@ jobs:
               console.log("Slither report is empty. No comment will be posted.");
               return;
             }
-            await script({ github, context, header, body })
+            await script({ github, context, header, body })

contracts/base/ModuleManager.sol

@@ -213,11 +213,31 @@ contract ModuleManager is Storage, Receiver, IModuleManagerEventsAndErrors {
     /// @param handler The address of the fallback handler to install.
     /// @param params The initialization parameters including the selector and call type.
     function _installFallbackHandler(address handler, bytes calldata params) internal virtual {
+        // Extracting the function selector from the parameters
         bytes4 selector = bytes4(params[0:4]);
+
+        // Extracting the call type from the parameters
         CallType calltype = CallType.wrap(bytes1(params[4]));
+
+        // Extracting the initialization data from the parameters
         bytes memory initData = params[5:];
-        if (_isFallbackHandlerInstalled(selector)) revert FallbackAlreadyInstalledForSelector(selector);
+
+        // Revert if the selector is either `onInstall(bytes)` (0x6d61fe70) or `onUninstall(bytes)` (0x8a91b0e3)
+        // These selectors are forbidden as they can lead to security vulnerabilities
+        // and unexpected behavior during fallback handler installation.
+        if (selector == bytes4(0x6d61fe70) || selector == bytes4(0x8a91b0e3)) {
+            revert FallbackSelectorForbidden();
+        }
+
+        // Revert if a fallback handler is already installed for the given selector
+        if (_isFallbackHandlerInstalled(selector)) {
+            revert FallbackAlreadyInstalledForSelector(selector);
+        }
+
+        // Store the fallback handler and its call type in the account storage
         _getAccountStorage().fallbacks[selector] = FallbackHandler(handler, calltype);
+
+        // Invoke the `onInstall` function of the fallback handler with the provided initialization data
         IFallback(handler).onInstall(initData);
     }
 

contracts/interfaces/base/IModuleManagerEventsAndErrors.sol

@@ -69,4 +69,7 @@ interface IModuleManagerEventsAndErrors {
 
     /// @dev Thrown when no fallback handler is available for a given selector.
     error MissingFallbackHandler(bytes4 selector);
+
+    /// @dev Thrown when there is an attempt to install a forbidden selector as a fallback handler.
+    error FallbackSelectorForbidden();
 }

scripts/foundry/generateGasReport.js

@@ -1,121 +1,140 @@
-const fs = require('fs');
-const readline = require('readline');
-const { exec } = require('child_process');
+const fs = require("fs");
+const readline = require("readline");
+const { exec } = require("child_process");
 
 // Define the log file and the output markdown file
-const LOG_FILE = 'gas.log';
-const OUTPUT_FILE = 'gas_report.md';
+const LOG_FILE = "gas.log";
+const OUTPUT_FILE = "GAS_REPORT.md";
 
 // Function to execute the `forge test` command
 function runForgeTest() {
-    return new Promise((resolve, reject) => {
-        console.log('🚀 Running forge tests, this may take a few minutes...');
-        exec('forge test -vv --mt test_Gas > gas.log', (error, stdout, stderr) => {
-            if (error) {
-                console.error(`❌ Exec error: ${error}`);
-                reject(`exec error: ${error}`);
-            }
-            console.log('✅ Forge tests completed.');
-            resolve(stdout ? stdout : stderr);
-        });
+  return new Promise((resolve, reject) => {
+    console.log("🚀 Running forge tests, this may take a few minutes...");
+    exec("forge test -vv --mt test_Gas > gas.log", (error, stdout, stderr) => {
+      if (error) {
+        console.error(`❌ Exec error: ${error}`);
+        reject(`exec error: ${error}`);
+      }
+      console.log("✅ Forge tests completed.");
+      resolve(stdout ? stdout : stderr);
     });
+  });
 }
 
 // Function to parse the log file and generate the report
 async function generateReport() {
-    await runForgeTest();
-
-    const fileStream = fs.createReadStream(LOG_FILE);
-    const rl = readline.createInterface({
-        input: fileStream,
-        crlfDelay: Infinity
-    });
-
-    const results = [];
-
-    console.log('📄 Parsing log file, please wait...');
-    for await (const line of rl) {
-        console.log(line);
-        if (line.includes('::')) {
-            const parts = line.split('::');
-            const PROTOCOL = parts[0];
-            const ACTION_FUNCTION = parts[1];
-            let ACCOUNT_TYPE;
-            let IS_DEPLOYED;
-            if (line.includes('EOA')) {
-                ACCOUNT_TYPE = 'EOA';
-                IS_DEPLOYED = 'False';
-            } else if (line.includes('Nexus')) {
-                ACCOUNT_TYPE = 'Smart Account';
-                IS_DEPLOYED = 'True';
-            } else {
-                ACCOUNT_TYPE = 'Smart Account';
-                IS_DEPLOYED = 'False';
-            }
-
-            const WITH_PAYMASTER = line.includes('WithPaymaster') ? 'True' : 'False';
-
-            const GAS_INFO = parts[4];
-            const ACCESS_TYPE = GAS_INFO.split(': ')[0];
-            const GAS_USED = GAS_INFO.split(': ')[1];
-
-            let RECEIVER_ACCESS;
-            if (ACCESS_TYPE === 'ColdAccess') {
-                RECEIVER_ACCESS = '🧊 ColdAccess';
-            } else if (ACCESS_TYPE === 'WarmAccess') {
-                RECEIVER_ACCESS = '🔥 WarmAccess';
-            } else {
-                RECEIVER_ACCESS = 'N/A';
-            }
-
-            results.push({
-                PROTOCOL,
-                ACTION_FUNCTION,
-                ACCOUNT_TYPE,
-                IS_DEPLOYED,
-                WITH_PAYMASTER,
-                RECEIVER_ACCESS,
-                GAS_USED,
-                FULL_LOG: line.trim()
-            });
-        }
+  await runForgeTest();
+
+  const fileStream = fs.createReadStream(LOG_FILE);
+  const rl = readline.createInterface({
+    input: fileStream,
+    crlfDelay: Infinity,
+  });
+
+  const results = [];
+
+  console.log("📄 Parsing log file, please wait...");
+  for await (const line of rl) {
+    console.log(line);
+    if (line.includes("::")) {
+      const parts = line.split("::");
+      const PROTOCOL = parts[0];
+      const ACTION_FUNCTION = parts[1];
+      let ACCOUNT_TYPE;
+      let IS_DEPLOYED;
+      if (line.includes("EOA")) {
+        ACCOUNT_TYPE = "EOA";
+        IS_DEPLOYED = "False";
+      } else if (line.includes("Nexus")) {
+        ACCOUNT_TYPE = "Smart Account";
+        IS_DEPLOYED = "True";
+      } else {
+        ACCOUNT_TYPE = "Smart Account";
+        IS_DEPLOYED = "False";
+      }
+
+      const WITH_PAYMASTER = line.includes("WithPaymaster") ? "True" : "False";
+
+      const GAS_INFO = parts[4];
+      const ACCESS_TYPE = GAS_INFO.split(": ")[0];
+      const GAS_USED = GAS_INFO.split(": ")[1];
+
+      let RECEIVER_ACCESS;
+      if (ACCESS_TYPE === "ColdAccess") {
+        RECEIVER_ACCESS = "🧊 ColdAccess";
+      } else if (ACCESS_TYPE === "WarmAccess") {
+        RECEIVER_ACCESS = "🔥 WarmAccess";
+      } else {
+        RECEIVER_ACCESS = "N/A";
+      }
+
+      results.push({
+        PROTOCOL,
+        ACTION_FUNCTION,
+        ACCOUNT_TYPE,
+        IS_DEPLOYED,
+        WITH_PAYMASTER,
+        RECEIVER_ACCESS,
+        GAS_USED,
+        FULL_LOG: line.trim(),
+      });
     }
-
-    console.log('🔄 Sorting results...');
-    // Custom sort: Group by protocol alphabetically, then by EOA first, Smart Account with Is Deployed=True next, then the rest
-    results.sort((a, b) => {
-        if (a.PROTOCOL < b.PROTOCOL) return -1;
-        if (a.PROTOCOL > b.PROTOCOL) return 1;
-        if (a.ACCOUNT_TYPE === 'EOA' && b.ACCOUNT_TYPE !== 'EOA') return -1;
-        if (a.ACCOUNT_TYPE !== 'EOA' && b.ACCOUNT_TYPE === 'EOA') return 1;
-        if (a.IS_DEPLOYED === 'True' && b.IS_DEPLOYED !== 'True') return -1;
-        if (a.IS_DEPLOYED !== 'True' && b.IS_DEPLOYED === 'True') return 1;
-        return 0;
+  }
+
+  console.log("🔄 Sorting results...");
+  // Custom sort: Group by protocol alphabetically, then by EOA first, Smart Account with Is Deployed=True next, then the rest
+  results.sort((a, b) => {
+    if (a.PROTOCOL < b.PROTOCOL) return -1;
+    if (a.PROTOCOL > b.PROTOCOL) return 1;
+    if (a.ACCOUNT_TYPE === "EOA" && b.ACCOUNT_TYPE !== "EOA") return -1;
+    if (a.ACCOUNT_TYPE !== "EOA" && b.ACCOUNT_TYPE === "EOA") return 1;
+    if (a.IS_DEPLOYED === "True" && b.IS_DEPLOYED !== "True") return -1;
+    if (a.IS_DEPLOYED !== "True" && b.IS_DEPLOYED === "True") return 1;
+    return 0;
+  });
+
+  console.log("🖋️ Writing report...");
+  // Write the report
+  const outputStream = fs.createWriteStream(OUTPUT_FILE);
+  outputStream.write("# Gas Report\n");
+  outputStream.write(
+    "| **Protocol** | **Actions / Function** | **Account Type** | **Is Deployed** | **With Paymaster?** | **Receiver Access** | **Gas Used** | **Full Log** |\n",
+  );
+  outputStream.write(
+    "|:------------:|:---------------------:|:----------------:|:--------------:|:-------------------:|:-------------------:|:------------:|:-------------:|\n",
+  );
+
+  results.forEach((result) => {
+    outputStream.write(
+      `| ${result.PROTOCOL} | ${result.ACTION_FUNCTION} | ${result.ACCOUNT_TYPE} | ${result.IS_DEPLOYED} | ${result.WITH_PAYMASTER} | ${result.RECEIVER_ACCESS} | ${result.GAS_USED} | ${result.FULL_LOG} |\n`,
+    );
+  });
+
+  console.log(`📊 Gas report generated and saved to ${OUTPUT_FILE}`);
+
+  // Run prettier to format the generated markdown file
+  return new Promise((resolve, reject) => {
+    console.log("✨ Running prettier to format the gas report...");
+    exec(`npx prettier --write ${OUTPUT_FILE}`, (error, stdout, stderr) => {
+      if (error) {
+        console.error(`❌ Prettier error: ${error}`);
+        reject(`prettier error: ${error}`);
+      }
+      console.log("✅ Prettier formatting completed.");
+      resolve(stdout ? stdout : stderr);
     });
-
-    console.log('🖋️ Writing report...');
-    // Write the report
-    const outputStream = fs.createWriteStream(OUTPUT_FILE);
-    outputStream.write("# Gas Report\n");
-    outputStream.write("| **Protocol** | **Actions / Function** | **Account Type** | **Is Deployed** | **With Paymaster?** | **Receiver Access** | **Gas Used** | **Full Log** |\n");
-    outputStream.write("|:------------:|:---------------------:|:----------------:|:--------------:|:-------------------:|:-------------------:|:------------:|:-------------:|\n");
-
-    results.forEach(result => {
-        outputStream.write(`| ${result.PROTOCOL} | ${result.ACTION_FUNCTION} | ${result.ACCOUNT_TYPE} | ${result.IS_DEPLOYED} | ${result.WITH_PAYMASTER} | ${result.RECEIVER_ACCESS} | ${result.GAS_USED} | ${result.FULL_LOG} |\n`);
-    });
-
-    console.log(`📊 Gas report generated and saved to ${OUTPUT_FILE}`);
+  });
 }
 
 // Function to clean up temporary files
 function cleanUp() {
-    fs.unlink(LOG_FILE, (err) => {
-        if (err) console.error(`❌ Error deleting ${LOG_FILE}: ${err}`);
-        else console.log(`🗑️ ${LOG_FILE} deleted successfully.`);
-    });
+  fs.unlink(LOG_FILE, (err) => {
+    if (err) console.error(`❌ Error deleting ${LOG_FILE}: ${err}`);
+    else console.log(`🗑️ ${LOG_FILE} deleted successfully.`);
+  });
 }
 
-// Run the function to generate the report and then clean up
+// Run the function to generate the report, format it with prettier, and then clean up
 generateReport()
-    .then(cleanUp)
-    .catch(console.error);
+  .then(cleanUp)
+  .catch(console.error);

test/foundry/fork/base/TestNexusSwapETH_Integration.t.sol

@@ -208,46 +208,45 @@ contract TestNexusSwapETH_Integration is BaseSettings {
     }
 
     /// @notice Tests gas consumption for swapping ETH for USDC using a deployed Nexus account with Paymaster
-function test_Gas_Swap_DeployedNexus_SwapEthForTokens_WithPaymaster()
-    public
-    checkERC20Balance(preComputedAddress, SWAP_AMOUNT)
-    checkPaymasterBalance(address(paymaster))
-{
-    // Prepare the swap execution details
-    Execution[] memory executions = prepareSingleExecution(
-        address(uniswapV2Router), // Uniswap V2 Router address
-        SWAP_AMOUNT, // Amount of ETH to swap
-        abi.encodeWithSignature(
-            "swapExactETHForTokens(uint256,address[],address,uint256)", // Function signature
-            0, // Minimum amount of tokens to receive (set to 0 for simplicity)
-            getPathForETHtoUSDC(), // Path for the swap (ETH to USDC)
-            preComputedAddress, // Recipient of the USDC
-            block.timestamp // Deadline for the swap
-        )
-    );
-
-    // Deploy the Nexus account
-    Nexus deployedNexus = deployNexus(user, 100 ether, address(VALIDATOR_MODULE));
-
-    // Build the PackedUserOperation array
-    PackedUserOperation[] memory userOps = buildPackedUserOperation(
-        user, // Wallet initiating the operation
-        deployedNexus, // Deployed Nexus account
-        EXECTYPE_DEFAULT, // Execution type
-        executions, // Execution details
-        address(VALIDATOR_MODULE) // Validator module address
-    );
-
-    // Generate and sign paymaster data
-    userOps[0].paymasterAndData = generateAndSignPaymasterData(userOps[0], BUNDLER, paymaster);
-
-    // Sign the entire user operation with the user's wallet
-    userOps[0].signature = signUserOp(user, userOps[0]);
-
-    // Measure and log gas usage for the operation
-    measureAndLogGas("UniswapV2::swapExactETHForTokens::Nexus::WithPaymaster::N/A", userOps);
-}
+    function test_Gas_Swap_DeployedNexus_SwapEthForTokens_WithPaymaster()
+        public
+        checkERC20Balance(preComputedAddress, SWAP_AMOUNT)
+        checkPaymasterBalance(address(paymaster))
+    {
+        // Prepare the swap execution details
+        Execution[] memory executions = prepareSingleExecution(
+            address(uniswapV2Router), // Uniswap V2 Router address
+            SWAP_AMOUNT, // Amount of ETH to swap
+            abi.encodeWithSignature(
+                "swapExactETHForTokens(uint256,address[],address,uint256)", // Function signature
+                0, // Minimum amount of tokens to receive (set to 0 for simplicity)
+                getPathForETHtoUSDC(), // Path for the swap (ETH to USDC)
+                preComputedAddress, // Recipient of the USDC
+                block.timestamp // Deadline for the swap
+            )
+        );
 
+        // Deploy the Nexus account
+        Nexus deployedNexus = deployNexus(user, 100 ether, address(VALIDATOR_MODULE));
+
+        // Build the PackedUserOperation array
+        PackedUserOperation[] memory userOps = buildPackedUserOperation(
+            user, // Wallet initiating the operation
+            deployedNexus, // Deployed Nexus account
+            EXECTYPE_DEFAULT, // Execution type
+            executions, // Execution details
+            address(VALIDATOR_MODULE) // Validator module address
+        );
+
+        // Generate and sign paymaster data
+        userOps[0].paymasterAndData = generateAndSignPaymasterData(userOps[0], BUNDLER, paymaster);
+
+        // Sign the entire user operation with the user's wallet
+        userOps[0].signature = signUserOp(user, userOps[0]);
+
+        // Measure and log gas usage for the operation
+        measureAndLogGas("UniswapV2::swapExactETHForTokens::Nexus::WithPaymaster::N/A", userOps);
+    }
 
     /// @notice Helper function to get the path for ETH to USDC swap
     /// @return path The array containing the swap path