fix(ct): DeputyPauseModule audit fixes (#13594)

Includes a number of low/informational fixes from the audits of
the DeputyPauseModule.

- Additional validation on the DeputyGuardianModule.
- Small additional comments.
- Proper encoding of EIP-712 signature digest.

packages/contracts-bedrock/interfaces/safe/IDeputyPauseModule.sol

@@ -8,6 +8,7 @@ import { ISuperchainConfig } from "interfaces/L1/ISuperchainConfig.sol";
 
 interface IDeputyPauseModule is ISemver {
     error DeputyPauseModule_InvalidDeputy();
+    error DeputyPauseModule_InvalidDeputyGuardianModule();
     error DeputyPauseModule_ExecutionFailed(string);
     error DeputyPauseModule_SuperchainNotPaused();
     error DeputyPauseModule_Unauthorized();

packages/contracts-bedrock/snapshots/abi/DeputyPauseModule.json

@@ -299,6 +299,11 @@
     "name": "DeputyPauseModule_InvalidDeputy",
     "type": "error"
   },
+  {
+    "inputs": [],
+    "name": "DeputyPauseModule_InvalidDeputyGuardianModule",
+    "type": "error"
+  },
   {
     "inputs": [],
     "name": "DeputyPauseModule_NonceAlreadyUsed",

packages/contracts-bedrock/snapshots/semver-lock.json

@@ -184,8 +184,8 @@
     "sourceCodeHash": "0x17236a91c4171ae9525eae0e59fa65bb2dc320d62677cfc7d7eb942f182619fb"
   },
   "src/safe/DeputyPauseModule.sol": {
-    "initCodeHash": "0x55f0b6c1a102114b8c30157e7295da7058ba8d1b93f7d4070028286968611279",
-    "sourceCodeHash": "0xec92b5ccbd3ee337897464546ccb8306fca245bbb6ce4b0941d86bc82e0f4cfe"
+    "initCodeHash": "0xa3b7bf0c93b41f39ebc18a81322b90127a633d684ae9f86c2f2a1c48fe7f1372",
+    "sourceCodeHash": "0xfbe7f5733aa57de7557605e40e03e9708fbdec872bc8739c551905c4b1e1b61b"
   },
   "src/safe/LivenessGuard.sol": {
     "initCodeHash": "0xc8e29e8b12f423c8cd229a38bc731240dd815d96f1b0ab96c71494dde63f6a81",

packages/contracts-bedrock/src/safe/DeputyPauseModule.sol

@@ -22,9 +22,12 @@ import { ISuperchainConfig } from "interfaces/L1/ISuperchainConfig.sol";
 ///         Superchain-wide pause functionality. Significantly simplifies the process of triggering
 ///         a Superchain-wide pause without changing the existing security model.
 contract DeputyPauseModule is ISemver, EIP712 {
-    /// @notice Error message for invalid deputy.
+    /// @notice Error message for deputy being invalid.
     error DeputyPauseModule_InvalidDeputy();
 
+    /// @notice Error message for the DeputyGuardianModule being invalid.
+    error DeputyPauseModule_InvalidDeputyGuardianModule();
+
     /// @notice Error message for unauthorized calls.
     error DeputyPauseModule_Unauthorized();
 
@@ -83,8 +86,8 @@ contract DeputyPauseModule is ISemver, EIP712 {
     mapping(bytes32 => bool) public usedNonces;
 
     /// @notice Semantic version.
-    /// @custom:semver 1.0.0-beta.1
-    string public constant version = "1.0.0-beta.1";
+    /// @custom:semver 1.0.0-beta.2
+    string public constant version = "1.0.0-beta.2";
 
     /// @param _foundationSafe Address of the Foundation Safe.
     /// @param _deputyGuardianModule Address of the DeputyGuardianModule used by the SC Safe.
@@ -101,7 +104,7 @@ contract DeputyPauseModule is ISemver, EIP712 {
         EIP712("DeputyPauseModule", "1")
     {
         _setDeputy(_deputy, _deputySignature);
-        deputyGuardianModule = _deputyGuardianModule;
+        _setDeputyGuardianModule(_deputyGuardianModule);
         FOUNDATION_SAFE = _foundationSafe;
         SUPERCHAIN_CONFIG = _superchainConfig;
     }
@@ -146,11 +149,13 @@ contract DeputyPauseModule is ISemver, EIP712 {
     /// @notice Sets the DeputyGuardianModule.
     /// @param _deputyGuardianModule DeputyGuardianModule address.
     function setDeputyGuardianModule(IDeputyGuardianModule _deputyGuardianModule) external {
+        // Can only be called by the Foundation Safe itself.
         if (msg.sender != address(FOUNDATION_SAFE)) {
             revert DeputyPauseModule_NotFromSafe();
         }
-        deputyGuardianModule = _deputyGuardianModule;
-        emit DeputyGuardianModuleSet(_deputyGuardianModule);
+
+        // Set the DeputyGuardianModule.
+        _setDeputyGuardianModule(_deputyGuardianModule);
     }
 
     /// @notice Calls the Foundation Safe's `execTransactionFromModuleReturnData()` function with
@@ -161,7 +166,7 @@ contract DeputyPauseModule is ISemver, EIP712 {
     /// @param _signature ECDSA signature.
     function pause(bytes32 _nonce, bytes memory _signature) external {
         // Verify the signature.
-        bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(PAUSE_MESSAGE_TYPEHASH, PauseMessage(_nonce))));
+        bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(PAUSE_MESSAGE_TYPEHASH, _nonce)));
         if (ECDSA.recover(digest, _signature) != deputy) {
             revert DeputyPauseModule_Unauthorized();
         }
@@ -175,6 +180,7 @@ contract DeputyPauseModule is ISemver, EIP712 {
         usedNonces[_nonce] = true;
 
         // Attempt to trigger the call.
+        // Will succeed if the DeputyGuardianModule has no code.
         (bool success, bytes memory returnData) = FOUNDATION_SAFE.execTransactionFromModuleReturnData(
             address(deputyGuardianModule), 0, abi.encodeCall(IDeputyGuardianModule.pause, ()), Enum.Operation.Call
         );
@@ -198,8 +204,7 @@ contract DeputyPauseModule is ISemver, EIP712 {
     /// @param _deputySignature Deputy signature.
     function _setDeputy(address _deputy, bytes memory _deputySignature) internal {
         // Check that the deputy signature is valid.
-        bytes32 digest =
-            _hashTypedDataV4(keccak256(abi.encode(DEPUTY_AUTH_MESSAGE_TYPEHASH, DeputyAuthMessage(_deputy))));
+        bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(DEPUTY_AUTH_MESSAGE_TYPEHASH, _deputy)));
         if (ECDSA.recover(digest, _deputySignature) != _deputy) {
             revert DeputyPauseModule_InvalidDeputy();
         }
@@ -210,4 +215,26 @@ contract DeputyPauseModule is ISemver, EIP712 {
         // Emit the DeputySet event.
         emit DeputySet(_deputy);
     }
+
+    /// @notice Internal function to set the DeputyGuardianModule.
+    /// @param _deputyGuardianModule DeputyGuardianModule address.
+    function _setDeputyGuardianModule(IDeputyGuardianModule _deputyGuardianModule) internal {
+        // Cannot set the DeputyGuardianModule to be the Foundation Safe. This prevents the
+        // Foundation Safe from unexpectedly triggering itself if there's some important function
+        // that happens to have the "pause()" selector.
+        if (address(_deputyGuardianModule) == address(FOUNDATION_SAFE)) {
+            revert DeputyPauseModule_InvalidDeputyGuardianModule();
+        }
+
+        // Make sure that the DeputyGuardianModule actually has code.
+        if (address(_deputyGuardianModule).code.length == 0) {
+            revert DeputyPauseModule_InvalidDeputyGuardianModule();
+        }
+
+        // Set the DeputyGuardianModule.
+        deputyGuardianModule = _deputyGuardianModule;
+
+        // Emit the DeputyGuardianModuleSet event.
+        emit DeputyGuardianModuleSet(_deputyGuardianModule);
+    }
 }

packages/contracts-bedrock/test/safe/DeputyPauseModule.t.sol

@@ -646,6 +646,14 @@ contract DeputyPauseModule_SetDeputyGuardianModule_Test is DeputyPauseModule_Tes
     function testFuzz_setDeputyGuardianModule_fromSafe_succeeds(address _newModule) external {
         vm.assume(_newModule != address(0));
         vm.assume(_newModule != address(deputyGuardianModule));
+        vm.assume(_newModule != address(foundationSafeInstance.safe));
+        assumeNotPrecompile(_newModule);
+
+        // Write code to the module address if it has no code.
+        // vm.assume would throw out too many inputs.
+        if (_newModule.code.length == 0) {
+            vm.etch(_newModule, hex"FF");
+        }
 
         // Set the new DeputyGuardianModule
         vm.expectEmit(address(deputyPauseModule));
@@ -674,4 +682,22 @@ contract DeputyPauseModule_SetDeputyGuardianModule_TestFail is DeputyPauseModule
         // Make sure DeputyGuardianModule has not changed
         assertEq(address(deputyPauseModule.deputyGuardianModule()), address(deputyGuardianModule));
     }
+
+    /// @notice Tests that setDeputyGuardianModule() reverts when the DeputyGuardianModule has no code.
+    /// @param _newModule The new DeputyGuardianModule.
+    function testFuzz_setDeputyGuardianModule_hasNoCode_reverts(address _newModule) external {
+        vm.assume(_newModule.code.length == 0);
+
+        // Expect a revert.
+        vm.expectRevert(IDeputyPauseModule.DeputyPauseModule_InvalidDeputyGuardianModule.selector);
+        vm.prank(address(foundationSafeInstance.safe));
+        deputyPauseModule.setDeputyGuardianModule(IDeputyGuardianModule(_newModule));
+    }
+
+    /// @notice Tests that setDeputyGuardianModule() reverts when the DeputyGuardianModule is the Foundation Safe.
+    function test_setDeputyGuardianModule_isFoundationSafe_reverts() external {
+        vm.expectRevert(IDeputyPauseModule.DeputyPauseModule_InvalidDeputyGuardianModule.selector);
+        vm.prank(address(foundationSafeInstance.safe));
+        deputyPauseModule.setDeputyGuardianModule(IDeputyGuardianModule(address(foundationSafeInstance.safe)));
+    }
 }