Audit feedback

contracts/core/GuardableModifier.sol

@@ -32,7 +32,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
                 address(0),
                 payable(0),
                 "",
-                msg.sender
+                sentOrSignedBy()
             );
         }
         success = IAvatar(target).execTransactionFromModule(
@@ -78,7 +78,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
                 address(0),
                 payable(0),
                 "",
-                msg.sender
+                sentOrSignedBy()
             );
         }
 

contracts/core/Modifier.sol

@@ -2,11 +2,17 @@
 pragma solidity >=0.7.0 <0.9.0;
 
 import "../interfaces/IAvatar.sol";
+import "../signature/ExecutionTracker.sol";
 import "../signature/SignatureChecker.sol";
 import "./Module.sol";
 
 /// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforce some additional logic.
-abstract contract Modifier is Module, SignatureChecker, IAvatar {
+abstract contract Modifier is
+    Module,
+    ExecutionTracker,
+    SignatureChecker,
+    IAvatar
+{
     address internal constant SENTINEL_MODULES = address(0x1);
     /// Mapping of modules.
     mapping(address => address) internal modules;
@@ -70,15 +76,38 @@ abstract contract Modifier is Module, SignatureChecker, IAvatar {
 
     modifier moduleOnly() {
         if (modules[msg.sender] == address(0)) {
-            if (modules[moduleTxSignedBy()] == address(0)) {
+            (bytes32 hash, address signer) = moduleTxSignedBy();
+
+            // is the signer a module?
+            if (modules[signer] == address(0)) {
                 revert NotAuthorized(msg.sender);
             }
-            moduleTxNonceBump();
+
+            // is the provided signature fresh?
+            if (consumed[signer][hash]) {
+                revert HashAlreadyConsumed(hash);
+            }
+
+            consumed[signer][hash] = true;
+            emit HashExecuted(hash);
         }
 
         _;
     }
 
+    function sentOrSignedBy() internal view returns (address) {
+        if (modules[msg.sender] != address(0)) {
+            return msg.sender;
+        }
+
+        (, address signer) = moduleTxSignedBy();
+        if (modules[signer] != address(0)) {
+            return signer;
+        }
+
+        return address(0);
+    }
+
     /// @dev Disables a module on the modifier.
     /// @notice This can only be called by the owner.
     /// @param prevModule Module that pointed to the module to be removed in the linked list.

contracts/signature/ExecutionTracker.sol

@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity >=0.8.0 <0.9.0;
+
+/// @title ExecutionTracker - A contract that keeps track of executed and invalidated hashes
+contract ExecutionTracker {
+    error HashAlreadyConsumed(bytes32);
+
+    event HashExecuted(bytes32);
+    event HashInvalidated(bytes32);
+
+    mapping(address => mapping(bytes32 => bool)) public consumed;
+
+    function invalidate(bytes32 hash) external {
+        consumed[msg.sender][hash] = true;
+        emit HashInvalidated(hash);
+    }
+}

contracts/signature/SignatureChecker.sol

@@ -6,24 +6,12 @@ import "./IERC1271.sol";
 /// @title SignatureChecker - A contract that retrieves and validates signatures appended to transaction calldata.
 /// @dev currently supports eip-712 and eip-1271 signatures
 abstract contract SignatureChecker {
-    uint256 private nonce;
-
-    /// @notice Returns the current nonce.
-    function moduleTxNonce() public view returns (uint256) {
-        return nonce;
-    }
-
-    /// @notice Increments nonce.
-    function moduleTxNonceBump() internal {
-        nonce = nonce + 1;
-    }
-
     /**
      * @notice Searches for a signature, validates it, and returns the signer's address.
      * @dev When signature not found or invalid, zero address is returned
      * @return The address of the signer.
      */
-    function moduleTxSignedBy() internal view returns (address) {
+    function moduleTxSignedBy() internal view returns (bytes32, address) {
         bytes calldata data = msg.data;
 
         /*
@@ -35,48 +23,58 @@ abstract contract SignatureChecker {
          * and recover signer.
          *
          * As a result, we impose a minimum calldata length equal to a function
-         * selector plus a signature (i.e., 4 + 65 bytes), any shorter and
-         * calldata it guaranteed to not contain a signature.
+         * selector plus salt, plus a signature (i.e., 4 + 32 + 65 bytes), any
+         * shorter and calldata it guaranteed to not contain a signature.
          */
-        if (data.length < 4 + 65) {
-            return address(0);
+        if (data.length < 4 + 32 + 65) {
+            return (bytes32(0), address(0));
         }
 
-        (
-            uint8 v,
-            bytes32 r,
-            bytes32 s,
-            bool isContractSignature,
-            uint256 start
-        ) = _splitSignature(data);
+        (uint8 v, bytes32 r, bytes32 s) = _splitSignature(data);
 
-        bytes32 hash = moduleTxHash(data[:start], nonce);
+        uint256 end = data.length - (32 + 65);
+        bytes32 salt = bytes32(data[end:]);
 
-        if (isContractSignature) {
+        /*
+         * When handling contract signatures:
+         *  v - is zero
+         *  r - contains the signer
+         *  s - contains the offset within calldata where the signer specific
+         *      signature is located
+         *
+         * We detect contract signatures by checking:
+         *  1- `v` is zero
+         *  2- `s` points within the buffer, is after selector, is before
+         *      salt and delimits a non-zero length buffer
+         */
+        if (v == 0) {
+            uint256 start = uint256(s);
+            if (start < 4 || start > end) {
+                return (bytes32(0), address(0));
+            }
             address signer = address(uint160(uint256(r)));
+
+            bytes32 hash = moduleTxHash(data[:start], salt);
             return
-                _isValidContractSignature(
-                    signer,
-                    hash,
-                    data[start:data.length - 65]
-                )
-                    ? signer
-                    : address(0);
+                _isValidContractSignature(signer, hash, data[start:end])
+                    ? (hash, signer)
+                    : (bytes32(0), address(0));
         } else {
-            return ecrecover(hash, v, r, s);
+            bytes32 hash = moduleTxHash(data[:end], salt);
+            return (hash, ecrecover(hash, v, r, s));
         }
     }
 
     /**
      * @notice Hashes the transaction EIP-712 data structure.
      * @dev The produced hash is intended to be signed.
      * @param data The current transaction's calldata.
-     * @param _nonce The nonce value.
+     * @param salt The salt value.
      * @return The 32-byte hash that is to be signed.
      */
     function moduleTxHash(
         bytes calldata data,
-        uint256 _nonce
+        bytes32 salt
     ) public view returns (bytes32) {
         bytes32 domainSeparator = keccak256(
             abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this)
@@ -85,7 +83,7 @@ abstract contract SignatureChecker {
             bytes1(0x19),
             bytes1(0x01),
             domainSeparator,
-            keccak256(abi.encode(MODULE_TX_TYPEHASH, keccak256(data), _nonce))
+            keccak256(abi.encode(MODULE_TX_TYPEHASH, keccak256(data), salt))
         );
         return keccak256(moduleTxData);
     }
@@ -96,43 +94,13 @@ abstract contract SignatureChecker {
      * @return v The ECDSA v value
      * @return r The ECDSA r value
      * @return s The ECDSA s value
-     * @return isEIP1271Signature Indicates whether the split signature is a contract signature
-     * @return start offset in calldata where signature starts
      */
     function _splitSignature(
         bytes calldata data
-    )
-        private
-        pure
-        returns (
-            uint8 v,
-            bytes32 r,
-            bytes32 s,
-            bool isEIP1271Signature,
-            uint256 start
-        )
-    {
-        /*
-         * When handling contract signatures:
-         *  v - is zero
-         *  r - contains the signer
-         *  s - contains the offset within calldata where the signer specific
-         *      signature is located
-         *
-         * We detect contract signatures by checking:
-         *  1- `v` is zero
-         *  2- `s` points within the buffer, is after selector, is before
-         *      signature and delimits a non-zero length buffer
-         */
-        uint256 length = data.length;
-        v = uint8(bytes1(data[length - 1:]));
-        r = bytes32(data[length - 65:]);
-        s = bytes32(data[length - 33:]);
-        isEIP1271Signature =
-            v == 0 &&
-            uint256(s) > 3 &&
-            uint256(s) < (length - 65);
-        start = isEIP1271Signature ? uint256(s) : length - 65;
+    ) private pure returns (uint8 v, bytes32 r, bytes32 s) {
+        v = uint8(bytes1(data[data.length - 1:]));
+        r = bytes32(data[data.length - 65:]);
+        s = bytes32(data[data.length - 33:]);
     }
 
     /**
@@ -173,10 +141,10 @@ abstract contract SignatureChecker {
         0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;
 
     // keccak256(
-    //     "ModuleTx(bytes data,uint256 nonce)"
+    //     "ModuleTx(bytes data,bytes32 salt)"
     // );
     bytes32 private constant MODULE_TX_TYPEHASH =
-        0xd6c6b5df57eef4e79cab990a377d29dc4c5bbb016a6293120d53f49c54144227;
+        0x2939aeeda3ca260200c9f7b436b19e13207547ccc65cfedc857751c5ea6d91d4;
 
     // bytes4(keccak256(
     //     "isValidSignature(bytes32,bytes)"

contracts/test/TestGuard.sol

@@ -4,7 +4,7 @@ pragma solidity >=0.7.0 <0.9.0;
 import "../core/GuardableModule.sol";
 
 contract TestGuard is FactoryFriendly, BaseGuard {
-    event PreChecked(bool checked);
+    event PreChecked(address sender);
     event PostChecked(bool checked);
 
     address public module;
@@ -29,13 +29,13 @@ contract TestGuard is FactoryFriendly, BaseGuard {
         address,
         address payable,
         bytes memory,
-        address
+        address sender
     ) public override {
         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 != Enum.Operation(1), "No delegate calls");
-        emit PreChecked(true);
+        emit PreChecked(sender);
     }
 
     function checkAfterExecution(bytes32, bool) public override {
@@ -51,3 +51,25 @@ contract TestGuard is FactoryFriendly, BaseGuard {
         module = _module;
     }
 }
+
+contract TestNonCompliantGuard is IERC165 {
+    function supportsInterface(bytes4) external pure returns (bool) {
+        return false;
+    }
+
+    function checkTransaction(
+        address,
+        uint256,
+        bytes memory,
+        Enum.Operation,
+        uint256,
+        uint256,
+        uint256,
+        address,
+        address,
+        bytes memory,
+        address
+    ) public {}
+
+    function checkAfterExecution(bytes32, bool) public {}
+}

contracts/test/TestGuardableModifier.sol

@@ -4,15 +4,15 @@ pragma solidity >=0.7.0 <0.9.0;
 import "../core/GuardableModifier.sol";
 
 contract TestGuardableModifier is GuardableModifier {
-    event executed(
+    event Executed(
         address to,
         uint256 value,
         bytes data,
         Enum.Operation operation,
         bool success
     );
 
-    event executedAndReturnedData(
+    event ExecutedAndReturnedData(
         address to,
         uint256 value,
         bytes data,
@@ -39,7 +39,7 @@ contract TestGuardableModifier is GuardableModifier {
         Enum.Operation operation
     ) public override moduleOnly returns (bool success) {
         success = exec(to, value, data, operation);
-        emit executed(to, value, data, operation, success);
+        emit Executed(to, value, data, operation, success);
     }
 
     /// @dev Passes a transaction to the modifier, expects return data.
@@ -60,7 +60,7 @@ contract TestGuardableModifier is GuardableModifier {
         returns (bool success, bytes memory returnData)
     {
         (success, returnData) = execAndReturnData(to, value, data, operation);
-        emit executedAndReturnedData(
+        emit ExecutedAndReturnedData(
             to,
             value,
             data,