Ensure Inner Simulation Reverts

nlordell · nlordell · commit df7653264882 · 2025-04-15T11:30:00.000+02:00
This PR makes a small modification the `simulate` function such that it
ensures its inner `simulateAndRevert` reverts as expected. This is to
prevent calls to the `CompatibilityFallbackHandler` from making
unexpected state changes when not called by a Safe.

While using the `CompatibilityFallbackHandler` for something other than
a Safe fallback handler is explictely not supported, it makes sense to
ensure the inner call inverts and prevent unintended behaviours.
diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol
@@ -7,19 +7,28 @@ import {HandlerContext} from "./HandlerContext.sol";
 import {TokenCallbackHandler} from "./TokenCallbackHandler.sol";
 
 /**
- * @title Compatibility Fallback Handler - Provides compatibility between pre 1.3.0 and 1.3.0+ Safe Smart Account contracts.
+ * @title Compatibility Fallback Handler
+ * @notice Provides compatibility between pre 1.3.0 and 1.3.0+ Safe smart account contracts.
+ * @dev ⚠️⚠️⚠️ This contract is only intended for being used as a fallback handler for a {Safe}.
+ *      Using it in other ways may cause undefined behavior. ⚠️⚠️⚠️
  * @author Richard Meissner - @rmeissner
  */
 contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidator, HandlerContext {
-    // keccak256("SafeMessage(bytes message)");
+    /**
+     * @dev The precomputed EIP-712 type hash for the Safe message type.
+     *      Precomputed value of: `keccak256("SafeMessage(bytes message)")`.
+     */
     bytes32 private constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;
 
-    bytes4 internal constant SIMULATE_SELECTOR = bytes4(keccak256("simulate(address,bytes)"));
-
+    /**
+     * @dev The sentinel module value in the {ModuleManager.modules} linked list.
+     *      See {ModuleManager.SENTINEL_MODULES} for more information.
+     */
     address internal constant SENTINEL_MODULES = address(0x1);
 
     /**
-     * @dev Returns the hash of a message to be signed by owners.
+     * @notice Returns the hash of a message to be signed by owners.
+     * @dev This function assumes that the caller is a Safe contract.
      * @param message Raw message bytes.
      * @return Message hash.
      */
@@ -28,7 +37,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
     }
 
     /**
-     * @dev Returns the pre-image of the message hash (see getMessageHashForSafe).
+     * @dev Returns the pre-image of the message hash (see {getMessageHashForSafe}).
      * @param safe Safe to which the message is targeted.
      * @param message Message that should be encoded.
      * @return Encoded message.
@@ -49,13 +58,14 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
     }
 
     /**
-     * @notice Implementation of updated EIP-1271 signature validation method.
-     * @param _dataHash Hash of the data signed on the behalf of address(msg.sender)
-     * @param _signature Signature byte array associated with _dataHash
-     * @return Updated EIP1271 magic value if signature is valid, otherwise 0x0
+     * @notice Implementation of the EIP-1271 signature validation method.
+     * @dev This implementation verifies signatures for a `ISafe(msg.sender)`.
+     * @param _dataHash Hash of the data signed.
+     * @param _signature Signature data.
+     * @return The EIP-1271 magic value if the signature is valid, reverts otherwise.
      */
     function isValidSignature(bytes32 _dataHash, bytes calldata _signature) public view override returns (bytes4) {
-        // Caller should be a Safe
+        // Caller should be a Safe.
         ISafe safe = ISafe(payable(msg.sender));
         bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash));
         bytes32 messageHash = keccak256(messageData);
@@ -74,80 +84,72 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
      * @return Array of modules.
      */
     function getModules() external view returns (address[] memory) {
-        // Caller should be a Safe
+        // Caller should be a Safe.
         ISafe safe = ISafe(payable(msg.sender));
         (address[] memory array, ) = safe.getModulesPaginated(SENTINEL_MODULES, 10);
         return array;
     }
 
     /**
-     * @dev Performs a delegatecall on a targetContract in the context of self.
-     * Internally reverts execution to avoid side effects (making it static). Catches revert and returns encoded result as bytes.
-     * @dev Inspired by https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol
+     * @notice Performs a `DELEGATECALL` to a `targetContract` in the context of self.
+     * @dev Internally reverts execution to avoid side effects (making it effectively static).
+     *      Catches the internal revert and returns encoded result as bytes.
+     *      Inspired by <https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol>.
      * @param targetContract Address of the contract containing the code to execute.
      * @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments).
      */
     function simulate(address targetContract, bytes calldata calldataPayload) external returns (bytes memory response) {
-        /**
-         * Suppress compiler warnings about not using parameters, while allowing
-         * parameters to keep names for documentation purposes. This does not
-         * generate code.
-         */
+        // Suppress compiler warnings about not using parameters, while allowing
+        // parameters to keep names for documentation purposes. This does not
+        // generate code.
         targetContract;
         calldataPayload;
 
         /* solhint-disable no-inline-assembly */
         /// @solidity memory-safe-assembly
         assembly {
             let ptr := mload(0x40)
-            /**
-             * Store `simulateAndRevert.selector`.
-             * String representation is used to force right padding
-             */
+            // Store `simulateAndRevert.selector`.
+            // String representation is used to force right padding.
             mstore(ptr, "\xb4\xfa\xba\x09")
 
-            /**
-             * Abuse the fact that both this and the internal methods have the
-             * same signature, and differ only in symbol name (and therefore,
-             * selector) and copy calldata directly. This saves us approximately
-             * 250 bytes of code and 300 gas at runtime over the
-             * `abi.encodeWithSelector` builtin.
-             */
+            // Abuse the fact that both this and the internal methods have the
+            // same signature, and differ only in symbol name (and therefore,
+            // selector) and copy calldata directly. This saves us approximately
+            // 250 bytes of code and 300 gas at runtime over the
+            // `abi.encodeWithSelector` builtin.
             calldatacopy(add(ptr, 0x04), 0x04, sub(calldatasize(), 0x04))
 
-            /**
-             * `pop` is required here by the compiler, as top level expressions
-             * can't have return values in inline assembly. `call` typically
-             * returns a 0 or 1 value indicating whether or not it reverted, but
-             * since we know it will always revert, we can safely ignore it.
-             */
-            pop(
-                call(
-                    gas(),
-                    // address() has been changed to caller() to use the implementation of the Safe
-                    caller(),
-                    0,
-                    ptr,
-                    calldatasize(),
-                    /**
-                     * The `simulateAndRevert` call always reverts, and
-                     * instead encodes whether or not it was successful in the return
-                     * data. The first 32-byte word of the return data contains the
-                     * `success` value, so write it to memory address 0x00 (which is
-                     * reserved Solidity scratch space and OK to use).
-                     */
-                    0x00,
-                    0x20
-                )
+            let success := call(
+                gas(),
+                // `address()` has been changed to `caller()` to use the
+                // implementation of the calling Safe.
+                caller(),
+                0,
+                ptr,
+                calldatasize(),
+                // The `simulateAndRevert` call should always reverts, and
+                // instead encodes whether or not it was successful in the return
+                // data. The first 32-byte word of the return data contains the
+                // `success` value, so write it to memory address 0x00 (which is
+                // the Solidity scratch space and OK to use).
+                0x00,
+                0x20
             )
 
-            /**
-             * Allocate and copy the response bytes, making sure to increment
-             * the free memory pointer accordingly (in case this method is
-             * called as an internal function). The remaining `returndata[0x20:]`
-             * contains the ABI encoded response bytes, so we can just write it
-             * as is to memory.
-             */
+            // Double check that the call reverted as expected. It will always
+            // revert if the caller is a Safe, but check anyway to make sure this
+            // function does not make unexpected state changes when called by
+            // other contracts.
+            if success {
+                revert(0, 0)
+            }
+
+            // Allocate and copy the response bytes, making sure to increment
+            // the free memory pointer accordingly (in case this method is
+            // called as an internal function). The remaining `returndata[0x20:]`
+            // contains the ABI encoded response bytes, so we can just write it
+            // as is to memory.
             let responseSize := sub(returndatasize(), 0x20)
             response := mload(0x40)
             mstore(0x40, add(response, responseSize))
diff --git a/test/handlers/CompatibilityFallbackHandler.spec.ts b/test/handlers/CompatibilityFallbackHandler.spec.ts
@@ -11,7 +11,7 @@ import {
     signHash,
 } from "../../src/utils/execution";
 import { chainId } from "../utils/encoding";
-import { killLibContract } from "../utils/contracts";
+import { badSimulatorContract, killLibContract } from "../utils/contracts";
 
 describe("CompatibilityFallbackHandler", () => {
     const setupTests = hre.deployments.createFixture(async ({ deployments }) => {
@@ -31,11 +31,13 @@ describe("CompatibilityFallbackHandler", () => {
         const safeAddress = await safe.getAddress();
         const validator = await getCompatFallbackHandler(safeAddress);
         const killLib = await killLibContract(user1, hre.network.zksync);
+        const badSimulator = await badSimulatorContract(user1);
         return {
             safe,
             validator,
             handler,
             killLib,
+            badSimulator,
             signLib,
             signerSafe,
             signers,
@@ -200,8 +202,6 @@ describe("CompatibilityFallbackHandler", () => {
     });
 
     describe("simulate", () => {
-        it.skip("can be called for any Safe", async () => {});
-
         it("should revert changes", async function () {
             /**
              * ## Test not applicable for zkSync, therefore should skip.
@@ -253,5 +253,11 @@ describe("CompatibilityFallbackHandler", () => {
             expect(value).to.be.eq(1n);
             expect(await killLib.value()).to.be.eq(0n);
         });
+
+        it("should revert for unsupported callers", async () => {
+            const { handler, badSimulator } = await setupTests();
+            const handlerAddress = await handler.getAddress();
+            await expect(badSimulator.simulateFallbackHandler(handlerAddress)).to.be.reverted;
+        });
     });
 });
diff --git a/test/utils/contracts.ts b/test/utils/contracts.ts
@@ -68,6 +68,26 @@ export const killLibContract = async (deployer: Signer, zkSync?: boolean) => {
     return deployContractFromSource(deployer, killLibSource);
 };
 
+export const badSimulatorSource = `
+interface ICompatibilityFallbackHandler {
+    function simulate(address, bytes calldata) external returns (bytes memory);
+}
+
+contract Test {
+    function simulateFallbackHandler(address fallbackHandler) external {
+        ICompatibilityFallbackHandler(fallbackHandler).simulate(address(0), "");
+    }
+
+    function simulateAndRevert(address, bytes memory) external returns (bool, bytes memory) {
+        // Oops! We don't revert.
+        return (true, "");
+    }
+}`;
+
+export const badSimulatorContract = async (deployer: Signer) => {
+    return deployContractFromSource(deployer, badSimulatorSource);
+};
+
 /**
  * Retrieves the sender address from the contract runner.
  * It is useful when using methods like `hre.ethers.getContractAt` which automatically attach
diff --git a/test/utils/setup.ts b/test/utils/setup.ts
@@ -201,7 +201,16 @@ export const compile = async (source: string) => {
         throw Error("Could not compile contract");
     }
     const fileOutput = output["contracts"]["tmp.sol"];
-    const contractOutput = fileOutput[Object.keys(fileOutput)[0]];
+    // Find the first contract with bytecode in the output, this allows the
+    // compiled code to include interfaces.
+    const contractOutput = Object.values(fileOutput).find((output) => {
+        const bytecode = (output["evm"]["bytecode"] ?? {})["object"] ?? "";
+        return bytecode.length > 0;
+    });
+    if (!contractOutput) {
+        console.log(output);
+        throw Error("No contract with bytecode");
+    }
     const abi = contractOutput["abi"];
     const data = "0x" + contractOutput["evm"]["bytecode"]["object"];
     return {
diff --git a/test/utils/zkSync.ts b/test/utils/zkSync.ts
@@ -73,7 +73,16 @@ export async function zkCompile(
     }
 
     const fileOutput = output["contracts"]["tmp.sol"];
-    const contractOutput = fileOutput[Object.keys(fileOutput)[0]];
+    // Find the first contract with bytecode in the output, this allows the
+    // compiled code to include interfaces.
+    const contractOutput = Object.values(fileOutput).find((output) => {
+        const bytecode = (output["evm"]["bytecode"] ?? {})["object"] ?? "";
+        return bytecode.length > 0;
+    });
+    if (!contractOutput) {
+        console.log(output);
+        throw Error("No contract with bytecode");
+    }
     const abi = contractOutput["abi"];
     const bytecode = "0x" + contractOutput["evm"]["bytecode"]["object"];
 
