Add a test for checking return data in execTransactionFromModuleReturnData

Author: mmv08

contracts/base/ModuleManager.sol

@@ -40,6 +40,29 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
         }
     }
 
+    function runPreExecutionChecks(
+        address to,
+        uint256 value,
+        bytes memory data,
+        Enum.Operation operation,
+        address guard
+    ) internal returns (bytes32 guardHash) {
+        // Only whitelisted modules are allowed.
+        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104");
+
+        if (guard != address(0)) {
+            guardHash = Guard(guard).checkModuleTransaction(to, value, data, operation, msg.sender);
+        }
+    }
+
+    function postExecutionChecksWithEventEmissions(bytes32 guardHash, bool success, address guard) internal {
+        if (guard != address(0)) {
+            Guard(guard).checkAfterExecution(guardHash, success);
+        }
+        if (success) emit ExecutionFromModuleSuccess(msg.sender);
+        else emit ExecutionFromModuleFailure(msg.sender);
+    }
+
     /**
      * @notice Enables the module `module` for the Safe.
      * @dev This can only be done via a Safe transaction.
@@ -85,22 +108,10 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
         bytes memory data,
         Enum.Operation operation
     ) public virtual returns (bool success) {
-        // Only whitelisted modules are allowed.
-        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104");
-        // Execute transaction without further confirmations.
         address guard = getGuard();
-
-        bytes32 guardHash;
-        if (guard != address(0)) {
-            guardHash = Guard(guard).checkModuleTransaction(to, value, data, operation, msg.sender);
-        }
+        bytes32 guardHash = runPreExecutionChecks(to, value, data, operation, guard);
         success = execute(to, value, data, operation, type(uint256).max);
-
-        if (guard != address(0)) {
-            Guard(guard).checkAfterExecution(guardHash, success);
-        }
-        if (success) emit ExecutionFromModuleSuccess(msg.sender);
-        else emit ExecutionFromModuleFailure(msg.sender);
+        postExecutionChecksWithEventEmissions(guardHash, success, guard);
     }
 
     /**
@@ -118,7 +129,9 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
         bytes memory data,
         Enum.Operation operation
     ) public returns (bool success, bytes memory returnData) {
-        success = execTransactionFromModule(to, value, data, operation);
+        address guard = getGuard();
+        bytes32 guardHash = runPreExecutionChecks(to, value, data, operation, guard);
+        success = execute(to, value, data, operation, type(uint256).max);
         /* solhint-disable no-inline-assembly */
         /// @solidity memory-safe-assembly
         assembly {
@@ -135,6 +148,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
             returnData := ptr
         }
         /* solhint-enable no-inline-assembly */
+        postExecutionChecksWithEventEmissions(guardHash, success, guard);
     }
 
     /**

test/core/Safe.GuardManager.spec.ts

@@ -298,6 +298,35 @@ describe("GuardManager", () => {
     });
 
     describe("execTransactionFromModuleReturnData", () => {
+        it("correctly returns the return data if the guard allows the transaction", async () => {
+            const {
+                safe,
+                validGuardMock,
+                signers: [user1, user2],
+            } = await setupWithTemplate();
+            const validGuardMockAddress = await validGuardMock.getAddress();
+            await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]);
+
+            const mock = await getMock();
+            const callData = "0xbeef73";
+            const returnBytes32 = "0x" + crypto.randomBytes(32).toString("hex");
+            await mock.givenCalldataReturnBytes32(callData, returnBytes32);
+            const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMockAddress)).interface;
+            const checkModuleTxData = guardInterface.encodeFunctionData("checkModuleTransaction", [
+                await mock.getAddress(),
+                0,
+                callData,
+                0,
+                user1.address,
+            ]);
+
+            await validGuardMock.givenCalldataReturnBool(checkModuleTxData, true);
+
+            const returnData = await safe.execTransactionFromModuleReturnData.staticCall(await mock.getAddress(), 0, callData, 0);
+
+            expect(returnData[1]).to.equal(returnBytes32);
+        });
+
         it("reverts if the pre hook of the guard reverts", async () => {
             const {
                 safe,