Update comments. Use v,s,r order. Add test cases

contracts/signature/SignatureChecker.sol

@@ -3,21 +3,17 @@ pragma solidity >=0.8.0 <0.9.0;
 
 import "./IERC1271.sol";
 
-/// @title SignatureChecker - A contract that extracts and inspects signatures appended to calldata.
-/// @notice currently supports eip-712 and eip-1271
+/// @title SignatureChecker - A contract that validates signatures appended to calldata.
+/// @dev currently supports eip-712 and eip-1271
 abstract contract SignatureChecker {
     uint256 private nonce;
 
-    /**
-     * @notice Returns the current nonce value.
-     */
+    /// @notice Returns the current nonce value.
     function moduleTxNonce() public view returns (uint256) {
         return nonce;
     }
 
-    /**
-     * @notice Increments nonce.
-     */
+    /// @notice Increments nonce.
     function moduleTxNonceBump() internal {
         nonce = nonce + 1;
     }
@@ -31,34 +27,32 @@ abstract contract SignatureChecker {
         bytes calldata data = msg.data;
 
         /*
-         * The overarching idea is to provide transparent checking to
-         * modules using `onlyModule`
+         * The idea is to extend `onlyModule` and provide signature checking
+         * to modules, without code changes.
          *
          * Since it's a generic mechanism we don't have a definitive way to
          * know if the trailling bytes are a signature. We just slice those
-         * bytes and recover a signature
+         * out and recover signer.
          *
          * Hence, we require the calldata to be at least as long as a function
-         * selector plus a signature - 4 + 65. Anything than that is guaranteed
-         * to not contain a signature
+         * selector plus a signature - 4 + 65. Anything less than that is
+         * guaranteed to not contain a signature
          */
         if (data.length < 4 + 65) {
             return address(0);
         }
 
         (
+            uint8 v,
             bytes32 r,
             bytes32 s,
-            uint8 v,
             bool isContractSignature,
             uint256 start
         ) = _splitSignature(data);
 
         bytes32 txHash = moduleTxHash(data[:start], nonce);
 
         if (isContractSignature) {
-            // When handling contract signatures the address
-            // of the contract is encoded into r
             address signer = address(uint160(uint256(r)));
             return
                 _isValidContractSignature(
@@ -74,7 +68,8 @@ abstract contract SignatureChecker {
     }
 
     /**
-     * @dev Hashes the EIP-712 data structure that will be signed.
+     * @notice Hashes the 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.
      * @return The 32-byte hash that is to be signed.
@@ -98,13 +93,24 @@ abstract contract SignatureChecker {
     /**
      * @dev Extracts signature from calldata, and divides it into `uint8 v, bytes32 r, bytes32 s`.
      * @param data The current transaction's calldata.
+     * @return v The ECDSA v value
+     * @return r The ECDSA r value
+     * @return s The ECDSA s value
+     * @return isEIP1271Signature Indicates whether it is a contract signature
+     * @return start offset in calldata where signature starts
      */
     function _splitSignature(
         bytes calldata data
     )
         private
         pure
-        returns (bytes32 r, bytes32 s, uint8 v, bool isEIP1271, uint256 start)
+        returns (
+            uint8 v,
+            bytes32 r,
+            bytes32 s,
+            bool isEIP1271Signature,
+            uint256 start
+        )
     {
         /*
          * When handling contract signatures:
@@ -113,22 +119,26 @@ abstract contract SignatureChecker {
          *  s - contains the offset within calldata where the signer specific
          *      signature is located
          *
-         *  We detect contract signatures by the following three conditions:
-         *  1- v is zero
-         *  2- s points within te buffer
-         *  3- the position pointed by S has non zero length
+         * We detect contract signatures by the following two conditions:
+         *  1- `v` is zero
+         *  2- `s` points within the buffer, is after selector and before sig
+         *
+         * The data pointed by s should be non zero, as EIP-1271 describes a
+         * a non-zero length signer specific signature
          */
-
         uint256 length = data.length;
+        v = uint8(bytes1(data[length - 1:]));
         r = bytes32(data[length - 65:]);
         s = bytes32(data[length - 33:]);
-        v = uint8(bytes1(data[length - 1:]));
-        isEIP1271 = v == 0 && (uint256(s) < (length - 65));
-        start = isEIP1271 ? uint256(s) : length - 65;
+        isEIP1271Signature =
+            v == 0 &&
+            uint256(s) > 3 &&
+            uint256(s) < (length - 65);
+        start = isEIP1271Signature ? uint256(s) : length - 65;
     }
 
     /**
-     * @dev Calls the signer contract, and validates the provided signature.
+     * @dev Calls the signer contract, and validates signature.
      * @param signer The address of the signer.
      * @param txHash The hash of the message that was signed.
      * @param signature The signature to validate.
@@ -147,15 +157,15 @@ abstract contract SignatureChecker {
             return false;
         }
 
-        (bool success, bytes memory returnData) = signer.staticcall(
+        (, bytes memory returnData) = signer.staticcall(
             abi.encodeWithSelector(
                 IERC1271.isValidSignature.selector,
                 txHash,
                 signature
             )
         );
 
-        return success == true && bytes4(returnData) == EIP1271_MAGIC_VALUE;
+        return bytes4(returnData) == EIP1271_MAGIC_VALUE;
     }
 
     // keccak256(