Audit feedback

contracts/core/GuardableModifier.sol

@@ -32,7 +32,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
                 address(0),
                 payable(0),
                 "",
-                msg.sender
+                _authorizer()
             );
         }
         success = IAvatar(target).execTransactionFromModule(
@@ -78,7 +78,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
                 address(0),
                 payable(0),
                 "",
-                msg.sender
+                _authorizer()
             );
         }
 
@@ -89,4 +89,13 @@ abstract contract GuardableModifier is Module, Guardable, Modifier {
             IGuard(currentGuard).checkAfterExecution(bytes32(0), success);
         }
     }
+
+    function _authorizer() private returns (address) {
+        if (modules[msg.sender] != address(0)) {
+            return msg.sender;
+        }
+
+        (, address signer) = moduleTxSignedBy();
+        return signer;
+    }
 }

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;
@@ -15,6 +21,12 @@ abstract contract Modifier is Module, SignatureChecker, IAvatar {
     /// @param sender The address of the sender.
     error NotAuthorized(address sender);
 
+    /// @param hash already executed.
+    error HashAlreadyExecuted(bytes32 hash);
+
+    /// @param hash already executed.
+    error HashInvalidated(bytes32 hash);
+
     /// `module` is invalid.
     error InvalidModule(address module);
 
@@ -70,10 +82,21 @@ 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();
+
+            if (modules[signer] == address(0)) {
                 revert NotAuthorized(msg.sender);
             }
-            moduleTxNonceBump();
+
+            if (executed[signer][hash]) {
+                revert HashAlreadyExecuted(hash);
+            }
+
+            if (invalidated[signer][hash]) {
+                revert HashInvalidated(hash);
+            }
+
+            executed[signer][hash] = true;
         }
 
         _;

contracts/signature/ExecutionTracker.sol

@@ -0,0 +1,12 @@
+// 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 {
+    mapping(address => mapping(bytes32 => bool)) public executed;
+    mapping(address => mapping(bytes32 => bool)) public invalidated;
+
+    function invalidate(bytes32 hash) external {
+        invalidated[msg.sender][hash] = true;
+    }
+}

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,11 +23,11 @@ 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));
         }
 
         (
@@ -50,7 +38,10 @@ abstract contract SignatureChecker {
             uint256 start
         ) = _splitSignature(data);
 
-        bytes32 hash = moduleTxHash(data[:start], nonce);
+        bytes32 hash = moduleTxHash(
+            data[:start], // slice the appended signature out
+            bytes32(data[data.length - 65 - 32:]) // get the salt
+        );
 
         if (isContractSignature) {
             address signer = address(uint160(uint256(r)));
@@ -60,23 +51,23 @@ abstract contract SignatureChecker {
                     hash,
                     data[start:data.length - 65]
                 )
-                    ? signer
-                    : address(0);
+                    ? (hash, signer)
+                    : (bytes32(0), address(0));
         } else {
-            return ecrecover(hash, v, r, s);
+            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 +76,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);
     }
@@ -122,7 +113,7 @@ abstract contract SignatureChecker {
          * 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
+         *      salt and delimits a non-zero length buffer
          */
         uint256 length = data.length;
         v = uint8(bytes1(data[length - 1:]));
@@ -131,8 +122,8 @@ abstract contract SignatureChecker {
         isEIP1271Signature =
             v == 0 &&
             uint256(s) > 3 &&
-            uint256(s) < (length - 65);
-        start = isEIP1271Signature ? uint256(s) : length - 65;
+            uint256(s) < (length - 65 - 32);
+        start = isEIP1271Signature ? uint256(s) : length - 65 - 32;
     }
 
     /**
@@ -173,10 +164,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 {

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,

contracts/test/TestModifier.sol

@@ -6,15 +6,15 @@ pragma solidity >=0.7.0 <0.9.0;
 import "../core/Modifier.sol";
 
 contract TestModifier is Modifier {
-    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,
@@ -41,7 +41,7 @@ contract TestModifier is Modifier {
         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.
@@ -62,7 +62,7 @@ contract TestModifier is Modifier {
         returns (bool success, bytes memory returnData)
     {
         (success, returnData) = execAndReturnData(to, value, data, operation);
-        emit executedAndReturnedData(
+        emit ExecutedAndReturnedData(
             to,
             value,
             data,

contracts/test/TestSignature.sol

@@ -38,10 +38,12 @@ contract TestSignature is SignatureChecker {
     event Goodbye(address signer);
 
     function hello() public {
-        emit Hello(moduleTxSignedBy());
+        (, address signer) = moduleTxSignedBy();
+        emit Hello(signer);
     }
 
     function goodbye(uint256, bytes memory) public {
-        emit Goodbye(moduleTxSignedBy());
+        (, address signer) = moduleTxSignedBy();
+        emit Goodbye(signer);
     }
 }

test/02_Module.spec.ts

@@ -98,9 +98,7 @@ describe("Module", async () => {
       await module.setGuard(guard.address);
       await expect(
         module.executeTransaction(tx.to, tx.value, tx.data, tx.operation)
-      )
-        .to.emit(guard, "PreChecked")
-        .withArgs(true);
+      ).to.emit(guard, "PreChecked");
     });
 
     it("executes a transaction", async () => {
@@ -151,9 +149,7 @@ describe("Module", async () => {
           tx.data,
           tx.operation
         )
-      )
-        .to.emit(guard, "PreChecked")
-        .withArgs(true);
+      ).to.emit(guard, "PreChecked");
     });
 
     it("executes a transaction", async () => {