Merge 702fabd into e870f51

contracts/Safe.sol

@@ -266,12 +266,11 @@ contract Safe is
      * @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.
      * @param dataHash Hash of the data (could be either a message hash or transaction hash)
-     * @param data That should be signed (this is passed to an external validator contract)
      * @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 data, bytes memory signatures, uint256 requiredSignatures) public view {
+    function checkNSignatures(bytes32 dataHash, bytes memory /* data */, 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.
@@ -284,7 +283,6 @@ contract Safe is
         for (i = 0; i < requiredSignatures; i++) {
             (v, r, s) = signatureSplit(signatures, i);
             if (v == 0) {
-                require(keccak256(data) == dataHash, "GS027");
                 // If v is 0 then it is a contract signature
                 // When handling contract signatures the address of the contract is encoded into r
                 currentOwner = address(uint160(uint256(r)));
@@ -312,7 +310,10 @@ contract Safe is
                     // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
                     contractSignature := add(add(signatures, s), 0x20)
                 }
-                require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
+                require(
+                    ISignatureValidator(currentOwner).isValidSignature(abi.encode(dataHash), contractSignature) == EIP1271_MAGIC_VALUE,
+                    "GS024"
+                );
             } else if (v == 1) {
                 // If v is 1 then it is an approved hash
                 // When handling approved hashes the address of the approver is encoded into r

contracts/handler/CompatibilityFallbackHandler.sol

@@ -4,7 +4,7 @@ pragma solidity >=0.7.0 <0.9.0;
 import "./TokenCallbackHandler.sol";
 import "../interfaces/ISignatureValidator.sol";
 import "../Safe.sol";
-
+    
 /**
  * @title Compatibility Fallback Handler - Provides compatibility between pre 1.3.0 and 1.3.0+ Safe contracts.
  * @author Richard Meissner - @rmeissner
@@ -74,7 +74,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
      * @param _signature Signature byte array associated with _dataHash
      * @return Updated EIP1271 magic value if signature is valid, otherwise 0x0
      */
-    function isValidSignature(bytes32 _dataHash, bytes calldata _signature) external view returns (bytes4) {
+    function isValidSignature(bytes32 _dataHash, bytes calldata _signature) external view override returns (bytes4) {
         ISignatureValidator validator = ISignatureValidator(msg.sender);
         bytes4 value = validator.isValidSignature(abi.encode(_dataHash), _signature);
         return (value == EIP1271_MAGIC_VALUE) ? UPDATED_MAGIC_VALUE : bytes4(0);

contracts/interfaces/ISignatureValidator.sol

@@ -17,4 +17,15 @@ abstract contract ISignatureValidator is ISignatureValidatorConstants {
      * MUST allow external calls
      */
     function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);
+
+    /**
+     * @notice EIP1271 method to validate a signature.
+     * @param _hash Hash of the data signed on the behalf of address(this).
+     * @param _signature Signature byte array associated with _data.
+     *
+     * MUST return the bytes4 magic value 0x1626ba7e when function passes.
+     * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
+     * MUST allow external calls
+     */
+    function isValidSignature(bytes32 _hash, bytes memory _signature) external view virtual returns (bytes4);
 }

docs/error_codes.md

@@ -19,7 +19,6 @@
 - `GS024`: `Invalid contract signature provided`
 - `GS025`: `Hash has not been approved`
 - `GS026`: `Invalid owner provided`
-- `GS027`: `Data Hash and hash of the pre-image data do not match`
 
 ### General auth related
 - `GS030`: `Only owners can approve a hash`

test/core/Safe.Signatures.spec.ts

@@ -224,11 +224,12 @@ describe("Safe", async () => {
             const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address);
             const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]);
             const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
-            const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
 
-            // IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message
-            // we need to use the pre-image of the transaction hash to calculate the message hash
-            const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId());
+            const safeMessageHash = calculateSafeMessageHash(
+                signerSafe,
+                calculateSafeTransactionHash(safe, tx, await chainId()),
+                await chainId(),
+            );
             const signerSafeOwnerSignature = await signHash(user5, safeMessageHash);
             const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);
 
@@ -354,12 +355,9 @@ describe("Safe", async () => {
             const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address);
             const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]);
             const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
-            const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
             const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
 
-            // IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message
-            // we need to use the pre-image of the transaction hash to calculate the message hash
-            const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId());
+            const safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId());
             const signerSafeOwnerSignature = await signHash(user5, safeMessageHash);
             const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);
 
@@ -371,7 +369,7 @@ describe("Safe", async () => {
                 signerSafeSig,
             ]);
 
-            await safe.checkSignatures(txHash, txHashData, signatures);
+            await safe.checkSignatures(txHash, "0x", signatures);
         });
     });
 
@@ -473,12 +471,9 @@ describe("Safe", async () => {
             const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address);
             const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]);
             const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
-            const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
             const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
 
-            // IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message
-            // we need to use the pre-image of the transaction hash to calculate the message hash
-            const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId());
+            const safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId());
             const signerSafeOwnerSignature = await signHash(user5, safeMessageHash);
             const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);
 
@@ -490,7 +485,7 @@ describe("Safe", async () => {
                 signerSafeSig,
             ]);
 
-            await safe.checkNSignatures(txHash, txHashData, signatures, 5);
+            await safe.checkNSignatures(txHash, "0x", signatures, 5);
         });
 
         it("should be able to require no signatures", async () => {
@@ -530,18 +525,5 @@ describe("Safe", async () => {
 
             await safe.checkNSignatures(txHash, txHashData, signatures, 3);
         });
-
-        it("should revert if the hash of the pre-image data and dataHash do not match for EIP-1271 signature", async () => {
-            await setupTests();
-            const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address], 2);
-            const randomHash = `0x${crypto.pseudoRandomBytes(32).toString("hex")}`;
-            const randomBytes = `0x${crypto.pseudoRandomBytes(128).toString("hex")}`;
-            const randomAddress = `0x${crypto.pseudoRandomBytes(20).toString("hex")}`;
-            const randomSignature = `0x${crypto.pseudoRandomBytes(65).toString("hex")}`;
-
-            const eip1271Sig = buildContractSignature(randomAddress, randomSignature);
-            const signatures = buildSignatureBytes([eip1271Sig]);
-            await expect(safe.checkNSignatures(randomHash, randomBytes, signatures, 1)).to.be.revertedWith("GS027");
-        });
     });
 });

test/handlers/CompatibilityFallbackHandler.spec.ts

@@ -1,4 +1,4 @@
-import { buildContractSignature } from "./../../src/utils/execution";
+import { buildContractSignature, calculateSafeMessageHash } from "./../../src/utils/execution";
 import { expect } from "chai";
 import hre, { deployments, waffle, ethers } from "hardhat";
 import "@nomiclabs/hardhat-ethers";
@@ -98,8 +98,11 @@ describe("CompatibilityFallbackHandler", async () => {
                 ),
             };
             const sig2 = await signHash(user2, calculateSafeMessageHash(validator, "0xbaddad", await chainId()));
-            const validatorPreImageMessage = preimageSafeMessageHash(validator, "0xbaddad", await chainId());
-            const signerSafeMessageHash = calculateSafeMessageHash(signerSafe, validatorPreImageMessage, await chainId());
+            const signerSafeMessageHash = calculateSafeMessageHash(
+                signerSafe,
+                calculateSafeMessageHash(validator, "0xbaddad", await chainId()),
+                await chainId(),
+            );
             const signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash);
             const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);
 
@@ -149,8 +152,9 @@ describe("CompatibilityFallbackHandler", async () => {
                 ),
             };
             const ethSignSig = await signHash(user2, calculateSafeMessageHash(validator, dataHash, await chainId()));
-            const validatorPreImageMessage = preimageSafeMessageHash(validator, dataHash, await chainId());
-            const signerSafeMessageHash = calculateSafeMessageHash(signerSafe, validatorPreImageMessage, await chainId());
+            const validatorSafeMessageHash = calculateSafeMessageHash(validator, dataHash, await chainId());
+            const signerSafeMessageHash = calculateSafeMessageHash(signerSafe, validatorSafeMessageHash, await chainId());
+
             const signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash);
             const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);
 

test/integration/Safe.0xExploit.spec.ts

@@ -1,5 +1,5 @@
 import { expect } from "chai";
-import hre, { deployments, waffle } from "hardhat";
+import hre, { deployments, ethers, waffle } from "hardhat";
 import "@nomiclabs/hardhat-ethers";
 import { AddressZero } from "@ethersproject/constants";
 import { parseEther } from "@ethersproject/units";
@@ -41,7 +41,7 @@ describe("Safe", async () => {
 
             // Use off-chain Safe signature
             const messageData = await safe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce);
-            const messageHash = await messageHandler.getMessageHash(messageData);
+            const messageHash = await messageHandler.getMessageHash(ethers.utils.keccak256(messageData));
             const ownerSigs = await buildSignatureBytes([await signHash(user1, messageHash), await signHash(user2, messageHash)]);
             const encodedOwnerSigns = defaultAbiCoder.encode(["bytes"], [ownerSigs]).slice(66);