Merge a74784e into 9a1555a

safe-global · Jan 30, 2023 · b2a2c0b · b2a2c0b
2 parents 9a1555a + a74784e
commit b2a2c0b
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 16 deletions.
diff --git a/contracts/Safe.sol b/contracts/Safe.sol
@@ -318,7 +318,7 @@ contract Safe is
         return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this));
     }
 
-    /// @dev Returns the bytes that are hashed to be signed by owners.
+    /// @dev Returns the pre-image of the transaction hash (see getTransactionHash).
     /// @param to Destination address.
     /// @param value Ether value.
     /// @param data Data payload.

diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol
@@ -28,11 +28,12 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
     function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
         // Caller should be a Safe
         Safe safe = Safe(payable(msg.sender));
-        bytes32 messageHash = getMessageHashForSafe(safe, _data);
+        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, _data, _signature);
+            safe.checkSignatures(messageHash, messageData, _signature);
         }
         return EIP1271_MAGIC_VALUE;
     }
@@ -44,13 +45,21 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
         return getMessageHashForSafe(Safe(payable(msg.sender)), message);
     }
 
+    /// @dev Returns the pre-image of the message hash (see getMessageHashForSafe)
+    /// @param safe Safe to which the message is targeted
+    /// @param message Message that should be encoded
+    /// @return Encoded message.
+    function encodeMessageDataForSafe(Safe safe, bytes memory message) public view returns (bytes memory) {
+        bytes32 safeMessageHash = keccak256(abi.encode(SAFE_MSG_TYPEHASH, keccak256(message)));
+        return abi.encodePacked(bytes1(0x19), bytes1(0x01), safe.domainSeparator(), safeMessageHash);
+    }
+
     /// @dev Returns hash of a message that can be signed by owners.
     /// @param safe Safe to which the message is targeted
     /// @param message Message that should be hashed
     /// @return Message hash.
     function getMessageHashForSafe(Safe safe, bytes memory message) public view returns (bytes32) {
-        bytes32 safeMessageHash = keccak256(abi.encode(SAFE_MSG_TYPEHASH, keccak256(message)));
-        return keccak256(abi.encodePacked(bytes1(0x19), bytes1(0x01), safe.domainSeparator(), safeMessageHash));
+        return keccak256(encodeMessageDataForSafe(safe, message));
     }
 
     /**

diff --git a/src/utils/execution.ts b/src/utils/execution.ts
@@ -67,6 +67,10 @@ export const calculateSafeTransactionHash = (safe: Contract, safeTx: SafeTransac
     return utils._TypedDataEncoder.hash({ verifyingContract: safe.address, chainId }, EIP712_SAFE_TX_TYPE, safeTx)
 }
 
+export const preimageSafeMessageHash = (safe: Contract, message: string, chainId: BigNumberish): string => {
+    return utils._TypedDataEncoder.encode({ verifyingContract: safe.address, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message })
+}
+
 export const calculateSafeMessageHash = (safe: Contract, message: string, chainId: BigNumberish): string => {
     return utils._TypedDataEncoder.hash({ verifyingContract: safe.address, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message })
 }

diff --git a/test/handlers/CompatibilityFallbackHandler.spec.ts b/test/handlers/CompatibilityFallbackHandler.spec.ts
@@ -1,9 +1,10 @@
+import { buildContractSignature } from './../../src/utils/execution';
 import { expect } from "chai";
 import hre, { deployments, waffle, ethers } from "hardhat";
 import "@nomiclabs/hardhat-ethers";
 import { AddressZero } from "@ethersproject/constants";
 import { compatFallbackHandlerContract, getCompatFallbackHandler, getSafeWithOwners } from "../utils/setup";
-import { buildSignatureBytes, executeContractCallWithSigners, calculateSafeMessageHash, EIP712_SAFE_MESSAGE_TYPE, signHash } from "../../src/utils/execution";
+import { buildSignatureBytes, executeContractCallWithSigners, calculateSafeMessageHash, preimageSafeMessageHash, EIP712_SAFE_MESSAGE_TYPE, signHash } from "../../src/utils/execution";
 import { chainId } from "../utils/encoding";
 import { BigNumber } from "ethers";
 import { killLibContract } from "../utils/contracts";
@@ -16,15 +17,17 @@ describe("CompatibilityFallbackHandler", async () => {
         await deployments.fixture();
         const signLib = await (await hre.ethers.getContractFactory("SignMessageLib")).deploy();
         const handler = await getCompatFallbackHandler()
-        const safe = await getSafeWithOwners([user1.address, user2.address], 2, handler.address)
+        const signerSafe = await getSafeWithOwners([user1.address], 1, handler.address)
+        const safe = await getSafeWithOwners([user1.address, user2.address, signerSafe.address], 2, handler.address)
         const validator = (await compatFallbackHandlerContract()).attach(safe.address)
         const killLib = await killLibContract(user1);
         return {
             safe,
             validator,
             handler,
             killLib,
-            signLib
+            signLib,
+            signerSafe
         }
     })
 
@@ -83,14 +86,19 @@ describe("CompatibilityFallbackHandler", async () => {
             expect(await validator.callStatic['isValidSignature(bytes,bytes)']("0xbaddad", "0x")).to.be.eq("0x20c13b0b")
         })
 
-        it('should return magic value if enough owners signed', async () => {
-            const { validator } = await setupTests()
+        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()))
-            expect(await validator.callStatic['isValidSignature(bytes,bytes)']("0xbaddad", buildSignatureBytes([sig1, sig2]))).to.be.eq("0x20c13b0b")
+            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")
         })
     })
 
@@ -121,15 +129,20 @@ describe("CompatibilityFallbackHandler", async () => {
             expect(await validator.callStatic['isValidSignature(bytes32,bytes)'](dataHash, "0x")).to.be.eq("0x1626ba7e")
         })
 
-        it('should return magic value if enough owners signed', async () => {
-            const { validator } = await setupTests()
+        it('should return magic value if enough owners signed and allow a mix different signature types', async () => {
+            const { validator, signerSafe } = await setupTests()
             const dataHash = ethers.utils.keccak256("0xbaddad")
-            const sig1 = {
+            const typedDataSig = {
                 signer: user1.address,
                 data: await user1._signTypedData({ verifyingContract: validator.address, chainId: await chainId() }, EIP712_SAFE_MESSAGE_TYPE, { message: dataHash })
             }
-            const sig2 = await signHash(user2, calculateSafeMessageHash(validator,dataHash, await chainId()))
-            expect(await validator.callStatic['isValidSignature(bytes32,bytes)'](dataHash, buildSignatureBytes([sig1, sig2]))).to.be.eq("0x1626ba7e")
+            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 signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash)
+            const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data)
+
+            expect(await validator.callStatic['isValidSignature(bytes32,bytes)'](dataHash, buildSignatureBytes([typedDataSig, ethSignSig, signerSafeSig]))).to.be.eq("0x1626ba7e")
         })
     })