Merge d51a2bf into e870f51

safe-global · Jun 13, 2023 · c3cb033 · c3cb033
2 parents e870f51 + d51a2bf
commit c3cb033
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 120 deletions.
diff --git a/contracts/Safe.sol b/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,7 @@ 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(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

diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol
@@ -16,27 +16,6 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
     bytes4 internal constant SIMULATE_SELECTOR = bytes4(keccak256("simulate(address,bytes)"));
 
     address internal constant SENTINEL_MODULES = address(0x1);
-    bytes4 internal constant UPDATED_MAGIC_VALUE = 0x1626ba7e;
-
-    /**
-     * @notice Legacy EIP-1271 signature validation method.
-     * @dev Implementation of ISignatureValidator (see `interfaces/ISignatureValidator.sol`)
-     * @param _data Arbitrary length data signed on the behalf of address(msg.sender).
-     * @param _signature Signature byte array associated with _data.
-     * @return The EIP-1271 magic value.
-     */
-    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
-        // Caller should be a Safe
-        Safe safe = Safe(payable(msg.sender));
-        bytes memory messageData = encodeMessageDataForSafe(safe, _data);
-        bytes32 messageHash = keccak256(messageData);
-        if (_signature.length == 0) {
-            require(safe.signedMessages(messageHash) != 0, "Hash not approved");
-        } else {
-            safe.checkSignatures(messageHash, messageData, _signature);
-        }
-        return EIP1271_MAGIC_VALUE;
-    }
 
     /**
      * @dev Returns the hash of a message to be signed by owners.
@@ -74,10 +53,17 @@ 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) {
-        ISignatureValidator validator = ISignatureValidator(msg.sender);
-        bytes4 value = validator.isValidSignature(abi.encode(_dataHash), _signature);
-        return (value == EIP1271_MAGIC_VALUE) ? UPDATED_MAGIC_VALUE : bytes4(0);
+    function isValidSignature(bytes32 _dataHash, bytes calldata _signature) public view override returns (bytes4) {
+        // Caller should be a Safe
+        Safe safe = Safe(payable(msg.sender));
+        bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash));
+        bytes32 messageHash = keccak256(messageData);
+        if (_signature.length == 0) {
+            require(safe.signedMessages(messageHash) != 0, "Hash not approved");
+        } else {
+            safe.checkSignatures(messageHash, messageData, _signature);
+        }
+        return EIP1271_MAGIC_VALUE;
     }
 
     /**

diff --git a/contracts/interfaces/ISignatureValidator.sol b/contracts/interfaces/ISignatureValidator.sol
@@ -2,19 +2,19 @@
 pragma solidity >=0.7.0 <0.9.0;
 
 contract ISignatureValidatorConstants {
-    // bytes4(keccak256("isValidSignature(bytes,bytes)")
-    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
+    // bytes4(keccak256("isValidSignature(bytes32,bytes)")
+    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e;
 }
 
 abstract contract ISignatureValidator is ISignatureValidatorConstants {
     /**
-     * @notice Legacy EIP1271 method to validate a signature.
-     * @param _data Arbitrary length data signed on the behalf of address(this).
+     * @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 0x20c13b0b when function passes.
+     * 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(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);
+    function isValidSignature(bytes32 _hash, bytes memory _signature) external view virtual returns (bytes4);
 }
diff --git a/docs/error_codes.md b/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`

diff --git a/test/core/Safe.Signatures.spec.ts b/test/core/Safe.Signatures.spec.ts
@@ -4,7 +4,6 @@ import { expect } from "chai";
 import { deployments, waffle } from "hardhat";
 import "@nomiclabs/hardhat-ethers";
 import { AddressZero } from "@ethersproject/constants";
-import crypto from "crypto";
 import { getSafeTemplate, getSafeWithOwners } from "../utils/setup";
 import {
     safeSignTypedData,
@@ -224,14 +223,15 @@ 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);
-
             await expect(
                 logGas(
                     "Execute cancel transaction with 5 owners (1 owner is another Safe)",
@@ -260,7 +260,7 @@ describe("Safe", async () => {
                 "0000000000000000000000000000000000000000000000000000000000000020" +
                 "00" + // r, s, v
                 "0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read
-            await expect(safe.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS021");
+            await expect(safe.checkSignatures(txHash, "0x", signatures)).to.be.revertedWith("GS021");
         });
 
         it("should fail if signatures data is not present", async () => {
@@ -354,12 +354,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 +368,7 @@ describe("Safe", async () => {
                 signerSafeSig,
             ]);
 
-            await safe.checkSignatures(txHash, txHashData, signatures);
+            await safe.checkSignatures(txHash, "0x", signatures);
         });
     });
 
@@ -473,12 +470,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 +484,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 +524,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");
-        });
     });
 });
diff --git a/test/handlers/CompatibilityFallbackHandler.spec.ts b/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";
@@ -8,7 +8,6 @@ import {
     buildSignatureBytes,
     executeContractCallWithSigners,
     calculateSafeMessageHash,
-    preimageSafeMessageHash,
     EIP712_SAFE_MESSAGE_TYPE,
     signHash,
 } from "../../src/utils/execution";
@@ -63,52 +62,6 @@ describe("CompatibilityFallbackHandler", async () => {
         });
     });
 
-    describe("isValidSignature(bytes,bytes)", async () => {
-        it("should revert if called directly", async () => {
-            const { handler } = await setupTests();
-            await expect(handler.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0x")).to.be.revertedWith(
-                "function call to a non-contract account",
-            );
-        });
-
-        it("should revert if message was not signed", async () => {
-            const { validator } = await setupTests();
-            await expect(validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0x")).to.be.revertedWith("Hash not approved");
-        });
-
-        it("should revert if signature is not valid", async () => {
-            const { validator } = await setupTests();
-            await expect(validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0xdeaddeaddeaddead")).to.be.reverted;
-        });
-
-        it("should return magic value if message was signed", async () => {
-            const { safe, validator, signLib } = await setupTests();
-            await executeContractCallWithSigners(safe, signLib, "signMessage", ["0xbaddad"], [user1, user2], true);
-            expect(await validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0x")).to.be.eq("0x20c13b0b");
-        });
-
-        it("should return magic value if enough owners signed and allow a mix different signature types", async () => {
-            const { validator, signerSafe } = await setupTests();
-            const sig1 = {
-                signer: user1.address,
-                data: await user1._signTypedData(
-                    { verifyingContract: validator.address, chainId: await chainId() },
-                    EIP712_SAFE_MESSAGE_TYPE,
-                    { message: "0xbaddad" },
-                ),
-            };
-            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 signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash);
-            const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);
-
-            expect(
-                await validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", buildSignatureBytes([sig1, sig2, signerSafeSig])),
-            ).to.be.eq("0x20c13b0b");
-        });
-    });
-
     describe("isValidSignature(bytes32,bytes)", async () => {
         it("should revert if called directly", async () => {
             const { handler } = await setupTests();
@@ -149,9 +102,11 @@ 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);
 
             expect(

diff --git a/test/integration/Safe.0xExploit.spec.ts b/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);
 
@@ -83,11 +83,11 @@ describe("Safe", async () => {
             contract Test {
                 bool public changeState;
                 uint256 public nonce;
-                function isValidSignature(bytes memory _data, bytes memory _signature) public returns (bytes4) {
+                function isValidSignature(bytes32 _data, bytes memory _signature) public returns (bytes4) {
                     if (changeState) {
                         nonce = nonce + 1;
                     }
-                    return 0x20c13b0b;
+                    return 0x1626ba7e;
                 }
     
                 function shouldChangeState(bool value) public {