Audit Feedback

contracts/core/GuardableModifier.sol

@@ -3,7 +3,7 @@ pragma solidity >=0.7.0 <0.9.0;
 
 import {Guardable} from "../guard/Guardable.sol";
 import {IAvatar} from "../interfaces/IAvatar.sol";
-import {IGuard} from "../interfaces/IGuard.sol";
+import {IModuleGuard} from "../interfaces/IGuard.sol";
 import {Modifier} from "./Modifier.sol";
 import {Module} from "./Module.sol";
 
@@ -22,21 +22,14 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
     bytes memory data,
     Operation operation
   ) internal virtual override returns (bool success) {
+    bytes32 moduleTxHash;
     address currentGuard = guard;
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkTransaction(
-        /// Transaction info used by module transactions.
+      moduleTxHash = IModuleGuard(currentGuard).checkModuleTransaction(
         to,
         value,
         data,
         operation,
-        /// Zero out the redundant transaction information only used for Safe multisig transctions.
-        0,
-        0,
-        0,
-        address(0),
-        payable(0),
-        "",
         sentOrSignedByModule()
       );
     }
@@ -47,7 +40,10 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
       operation
     );
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkAfterExecution(bytes32(0), success);
+      IModuleGuard(currentGuard).checkAfterModuleExecution(
+        moduleTxHash,
+        success
+      );
     }
   }
 
@@ -63,22 +59,15 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
     bytes memory data,
     Operation operation
   ) internal virtual override returns (bool success, bytes memory returnData) {
+    bytes32 moduleTxHash;
     address currentGuard = guard;
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkTransaction(
-        /// Transaction info used by module transactions.
+      moduleTxHash = IModuleGuard(currentGuard).checkModuleTransaction(
         to,
         value,
         data,
         operation,
-        /// Zero out the redundant transaction information only used for Safe multisig transctions.
-        0,
-        0,
-        0,
-        address(0),
-        payable(0),
-        "",
-        sentOrSignedByModule()
+        address(this)
       );
     }
 
@@ -90,7 +79,10 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
     );
 
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkAfterExecution(bytes32(0), success);
+      IModuleGuard(currentGuard).checkAfterModuleExecution(
+        moduleTxHash,
+        success
+      );
     }
   }
 }

contracts/core/GuardableModule.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity >=0.7.0 <0.9.0;
 
-import {IGuard} from "../interfaces/IGuard.sol";
+import {IModuleGuard} from "../interfaces/IGuard.sol";
 import {Guardable} from "../guard/Guardable.sol";
 import {Module} from "./Module.sol";
 import {IAvatar} from "../interfaces/IAvatar.sol";
@@ -21,22 +21,15 @@ abstract contract GuardableModule is Module, Guardable {
     bytes memory data,
     Operation operation
   ) internal override returns (bool success) {
+    bytes32 moduleTxHash;
     address currentGuard = guard;
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkTransaction(
-        /// Transaction info used by module transactions.
+      moduleTxHash = IModuleGuard(currentGuard).checkModuleTransaction(
         to,
         value,
         data,
         operation,
-        /// Zero out the redundant transaction information only used for Safe multisig transctions.
-        0,
-        0,
-        0,
-        address(0),
-        payable(0),
-        "",
-        msg.sender
+        address(this)
       );
     }
     success = IAvatar(target).execTransactionFromModule(
@@ -46,7 +39,10 @@ abstract contract GuardableModule is Module, Guardable {
       operation
     );
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkAfterExecution(bytes32(0), success);
+      IModuleGuard(currentGuard).checkAfterModuleExecution(
+        moduleTxHash,
+        success
+      );
     }
   }
 
@@ -62,22 +58,15 @@ abstract contract GuardableModule is Module, Guardable {
     bytes memory data,
     Operation operation
   ) internal virtual override returns (bool success, bytes memory returnData) {
+    bytes32 moduleTxHash;
     address currentGuard = guard;
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkTransaction(
-        /// Transaction info used by module transactions.
+      moduleTxHash = IModuleGuard(currentGuard).checkModuleTransaction(
         to,
         value,
         data,
         operation,
-        /// Zero out the redundant transaction information only used for Safe multisig transctions.
-        0,
-        0,
-        0,
-        address(0),
-        payable(0),
-        "",
-        msg.sender
+        address(this)
       );
     }
 
@@ -89,7 +78,10 @@ abstract contract GuardableModule is Module, Guardable {
     );
 
     if (currentGuard != address(0)) {
-      IGuard(currentGuard).checkAfterExecution(bytes32(0), success);
+      IModuleGuard(currentGuard).checkAfterModuleExecution(
+        moduleTxHash,
+        success
+      );
     }
   }
 }

contracts/guard/BaseGuard.sol

@@ -3,11 +3,11 @@ pragma solidity >=0.7.0 <0.9.0;
 
 import {IERC165} from "../interfaces/IERC165.sol";
 
-import {IGuard} from "../interfaces/IGuard.sol";
+import {IGuard, IModuleGuard} from "../interfaces/IGuard.sol";
 
 import "../core/Operation.sol";
 
-abstract contract BaseGuard is IERC165 {
+abstract contract BaseGuard is IGuard, IERC165 {
   function supportsInterface(
     bytes4 interfaceId
   ) external pure override returns (bool) {
@@ -35,3 +35,26 @@ abstract contract BaseGuard is IERC165 {
 
   function checkAfterExecution(bytes32 txHash, bool success) external virtual;
 }
+
+abstract contract BaseModuleGuard is IModuleGuard, IERC165 {
+  function checkModuleTransaction(
+    address to,
+    uint256 value,
+    bytes memory data,
+    Operation operation,
+    address module
+  ) external virtual returns (bytes32 moduleTxHash);
+
+  function checkAfterModuleExecution(
+    bytes32 txHash,
+    bool success
+  ) external virtual;
+
+  function supportsInterface(
+    bytes4 interfaceId
+  ) external pure override returns (bool) {
+    return
+      interfaceId == type(IModuleGuard).interfaceId || // 0x58401ed8
+      interfaceId == type(IERC165).interfaceId; // 0x01ffc9a7
+  }
+}

contracts/guard/Guardable.sol

@@ -3,8 +3,8 @@ pragma solidity >=0.7.0 <0.9.0;
 
 import {Ownable} from "../factory/Ownable.sol";
 
-import {BaseGuard} from "../guard/BaseGuard.sol";
-import {IGuard} from "../interfaces/IGuard.sol";
+import {BaseModuleGuard} from "../guard/BaseGuard.sol";
+import {IModuleGuard} from "../interfaces/IGuard.sol";
 
 /// @title Guardable - A contract that manages fallback calls made to this contract
 contract Guardable is Ownable {
@@ -19,8 +19,11 @@ contract Guardable is Ownable {
   /// @param _guard The address of the guard to be used or the 0 address to disable the guard.
   function setGuard(address _guard) external onlyOwner {
     if (_guard != address(0)) {
-      if (!BaseGuard(_guard).supportsInterface(type(IGuard).interfaceId))
-        revert NotIERC165Compliant(_guard);
+      if (
+        !BaseModuleGuard(_guard).supportsInterface(
+          type(IModuleGuard).interfaceId
+        )
+      ) revert NotIERC165Compliant(_guard);
     }
     guard = _guard;
     emit ChangedGuard(guard);

contracts/interfaces/IGuard.sol

@@ -20,3 +20,18 @@ interface IGuard {
 
   function checkAfterExecution(bytes32 txHash, bool success) external;
 }
+
+interface IModuleGuard {
+  function checkModuleTransaction(
+    address to,
+    uint256 value,
+    bytes memory data,
+    Operation operation,
+    address module
+  ) external returns (bytes32 moduleTxHash);
+
+  function checkAfterModuleExecution(
+    bytes32 moduleTxHash,
+    bool success
+  ) external;
+}

contracts/signature/SignatureChecker.sol

@@ -52,16 +52,18 @@ abstract contract SignatureChecker {
       if (start < 4 || start > end) {
         return (bytes32(0), address(0));
       }
+      bytes32 hash = moduleTxHash(data[:start], salt);
       address signer = address(uint160(uint256(r)));
 
-      bytes32 hash = moduleTxHash(data[:start], salt);
       return
         _isValidContractSignature(signer, hash, data[start:end])
           ? (hash, signer)
           : (bytes32(0), address(0));
     } else {
       bytes32 hash = moduleTxHash(data[:end], salt);
-      return (hash, ecrecover(hash, v, r, s));
+      address signer = ecrecover(hash, v, r, s);
+
+      return signer != address(0) ? (hash, signer) : (bytes32(0), address(0));
     }
   }
 

contracts/test/TestGuard.sol

@@ -3,16 +3,16 @@ pragma solidity >=0.7.0 <0.9.0;
 
 import {IERC165} from "../interfaces/IERC165.sol";
 
-import {BaseGuard} from "../guard/BaseGuard.sol";
+import {BaseModuleGuard} from "../guard/BaseGuard.sol";
 import {FactoryFriendly} from "../factory/FactoryFriendly.sol";
 import {GuardableModule} from "../core/GuardableModule.sol";
 
 import "../core/Operation.sol";
 
 /* solhint-disable */
 
-contract TestGuard is FactoryFriendly, BaseGuard {
-  event PreChecked(address sender);
+contract TestGuard is FactoryFriendly, BaseModuleGuard {
+  event PreChecked(address module);
   event PostChecked(bool checked);
 
   address public module;
@@ -26,27 +26,22 @@ contract TestGuard is FactoryFriendly, BaseGuard {
     module = _module;
   }
 
-  function checkTransaction(
+  function checkModuleTransaction(
     address to,
     uint256 value,
     bytes memory data,
     Operation operation,
-    uint256,
-    uint256,
-    uint256,
-    address,
-    address payable,
-    bytes memory,
-    address sender
-  ) public override {
+    address _module
+  ) public override returns (bytes32) {
     require(to != address(0), "Cannot send to zero address");
     require(value != 1337, "Cannot send 1337");
     require(bytes3(data) != bytes3(0xbaddad), "Cannot call 0xbaddad");
     require(operation != Operation(1), "No delegate calls");
-    emit PreChecked(sender);
+    emit PreChecked(_module);
+    return keccak256(abi.encodePacked(to, value, data, operation, _module));
   }
 
-  function checkAfterExecution(bytes32, bool) public override {
+  function checkAfterModuleExecution(bytes32, bool) public override {
     require(
       GuardableModule(module).guard() == address(this),
       "Module cannot remove its own guard."
@@ -64,20 +59,4 @@ contract TestNonCompliantGuard is IERC165 {
   function supportsInterface(bytes4) external pure returns (bool) {
     return false;
   }
-
-  function checkTransaction(
-    address,
-    uint256,
-    bytes memory,
-    Operation,
-    uint256,
-    uint256,
-    uint256,
-    address,
-    address,
-    bytes memory,
-    address
-  ) public {}
-
-  function checkAfterExecution(bytes32, bool) public {}
 }