Remove bytes from core contract methods and add them to the compatibi…

…lity fallback handler

certora/specs/NativeTokenRefund.spec

@@ -1,7 +1,7 @@
 // This spec is a separate file because we summarize checkSignatures here
 
 methods {
-    function checkSignatures(bytes32, bytes memory, bytes memory) internal => NONDET;
+    function checkSignatures(bytes32, bytes memory) internal => NONDET;
 
     function getNativeTokenBalanceFor(address) external returns (uint256) envfree;
     function getSafeGuard() external returns (address) envfree;

certora/specs/Safe.spec

@@ -17,7 +17,7 @@ methods {
     function execTransactionFromModule(address,uint256,bytes,Enum.Operation) external returns (bool);
     function execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool);
 
-    function checkSignatures(bytes32, bytes memory, bytes memory) internal => NONDET;
+    function checkSignatures(bytes32, bytes memory) internal => NONDET;
 }
 
 definition reachableOnly(method f) returns bool =

certora/specs/Signatures.spec

@@ -27,7 +27,7 @@ methods {
     // ) external returns (bytes32) => transactionHashGhost(to, value, callKeccak256(data), operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce) ;
 
     // optional
-    function checkSignatures(bytes32,bytes,bytes) external;
+    function checkSignatures(bytes32,bytes) external;
     function execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool);
 }
 

contracts/Safe.sol

@@ -166,7 +166,7 @@ contract Safe is
                 // We use the post-increment here, so the current nonce value is used and incremented afterwards.
                 nonce++
             );
-            checkSignatures(txHash, "", signatures);
+            checkSignatures(txHash, signatures);
         }
         address guard = getGuard();
         {
@@ -285,25 +285,20 @@ contract Safe is
     /**
      * @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
      * @param dataHash Hash of the data (could be either a message hash or transaction hash)
-     * @param data NOT USED. That should be signed (this is passed to an external validator contract)
-     *             For some reason, removing it from the parameters and passing empty bytes ("") slightly increases the gas costs, so we keep it.
      * @param signatures Signature data that should be verified.
      *                   Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
      */
-    function checkSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures) public view {
+    function checkSignatures(bytes32 dataHash, bytes memory signatures) public view {
         // Load threshold to avoid multiple storage loads
         uint256 _threshold = threshold;
         // Check that a threshold is set
         require(_threshold > 0, "GS001");
-        checkNSignatures(msg.sender, dataHash, data, signatures, _threshold);
+        checkNSignatures(msg.sender, dataHash, signatures, _threshold);
     }
 
     /**
      * @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
      * @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
-     *      The data parameter (bytes) is not used since v1.5.0 as it is not required anymore. Prior to v1.5.0,
-     *      data parameter was used in contract signature validation flow using legacy EIP-1271.
-     *      Version v1.5.0, uses dataHash parameter instead of data with updated EIP-1271 implementation.
      * @param executor Address that executing the transaction.
      *        ⚠️⚠️⚠️ Make sure that the executor address is a legitmate executor.
      *        Incorrectly passed the executor might reduce the threshold by 1 signature. ⚠️⚠️⚠️
@@ -312,13 +307,7 @@ contract Safe is
      *                   Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
      * @param requiredSignatures Amount of required valid signatures.
      */
-    function checkNSignatures(
-        address executor,
-        bytes32 dataHash,
-        bytes memory /* data */,
-        bytes memory signatures,
-        uint256 requiredSignatures
-    ) public view {
+    function checkNSignatures(address executor, bytes32 dataHash, bytes memory signatures, uint256 requiredSignatures) public view {
         // Check that the provided signature data is not too short
         require(signatures.length >= requiredSignatures.mul(65), "GS020");
         // There cannot be an owner with address 0.

contracts/handler/CompatibilityFallbackHandler.sol

@@ -62,7 +62,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
         if (_signature.length == 0) {
             require(safe.signedMessages(messageHash) != 0, "Hash not approved");
         } else {
-            safe.checkSignatures(messageHash, messageData, _signature);
+            safe.checkSignatures(messageHash, _signature);
         }
         return EIP1271_MAGIC_VALUE;
     }
@@ -162,13 +162,35 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
      * @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
      *      The function was moved to the fallback handler as a part of
      *      1.5.0 contract upgrade. It used to be a part of the Safe core contract, but
-     *      was replaced by the same function that accepts the executor as a parameter.
+     *      was replaced by the same function that also accepts an executor address.
      * @param dataHash Hash of the data (could be either a message hash or transaction hash)
      * @param signatures Signature data that should be verified.
      *                   Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
      * @param requiredSignatures Amount of required valid signatures.
      */
-    function checkNSignatures(bytes32 dataHash, bytes memory, bytes memory signatures, uint256 requiredSignatures) public view {
-        Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, "", signatures, requiredSignatures);
+    function checkNSignatures(
+        bytes32 dataHash,
+        bytes memory /* IGNORED */,
+        bytes memory signatures,
+        uint256 requiredSignatures
+    ) public view {
+        Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, signatures, requiredSignatures);
+    }
+
+    /**
+     * @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
+     * @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
+     *      The function was moved to the fallback handler as a part of
+     *      1.5.0 contract upgrade. It used to be a part of the Safe core contract, but
+     *      was replaced by the same function that removes the raw encoded bytes data parameter.
+     * @param dataHash Hash of the data (could be either a message hash or transaction hash)
+     * @param signatures Signature data that should be verified.
+     *                   Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
+     */
+    function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) public view {
+        Safe safe = Safe(payable(_manager()));
+
+        uint256 threshold = safe.getThreshold();
+        safe.checkNSignatures(_msgSender(), dataHash, signatures, threshold);
     }
 }