Merge e36fe25 into 9d188d3

contracts/Safe.sol

@@ -255,6 +255,7 @@ 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)));

docs/error_codes.md

@@ -19,6 +19,7 @@
 - `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`

src/utils/execution.ts

@@ -51,6 +51,8 @@ export interface SafeTransaction extends MetaTransaction {
 export interface SafeSignature {
     signer: string,
     data: string
+    // a flag to indicate if the signature is a contract signature and the data has to be appended to the dynamic part of signature bytes
+    dynamic?: true
 }
 
 export const calculateSafeDomainSeparator = (safe: Contract, chainId: BigNumberish): string => {
@@ -108,13 +110,45 @@ export const safeSignMessage = async (signer: Signer, safe: Contract, safeTx: Sa
     return signHash(signer, calculateSafeTransactionHash(safe, safeTx, cid))
 }
 
+export const buildContractSignature = (signerAddress: string, signature: string): SafeSignature => {
+    return {
+        signer: signerAddress,
+        data: signature,
+        dynamic: true,
+    }
+}
+
 export const buildSignatureBytes = (signatures: SafeSignature[]): string => {
+    const SIGNATURE_LENGTH_BYTES = 65
     signatures.sort((left, right) => left.signer.toLowerCase().localeCompare(right.signer.toLowerCase()))
+
     let signatureBytes = "0x"
+    let dynamicBytes = ""
     for (const sig of signatures) {
-        signatureBytes += sig.data.slice(2)
+        if (sig.dynamic) {
+            /* 
+                A contract signature has a static part of 65 bytes and the dynamic part that needs to be appended at the end of 
+                end signature bytes.
+                The signature format is
+                Signature type == 0
+                Constant part: 65 bytes
+                {32-bytes signature verifier}{32-bytes dynamic data position}{1-byte signature type}
+                Dynamic part (solidity bytes): 32 bytes + signature data length
+                {32-bytes signature length}{bytes signature data}
+            */
+            const dynamicPartPosition = (signatures.length * SIGNATURE_LENGTH_BYTES + dynamicBytes.length / 2).toString(16).padStart(64, "0")
+            const dynamicPartLength = (sig.data.slice(2).length / 2).toString(16).padStart(64, "0")
+            const staticSignature = `${sig.signer.slice(2).padStart(64, "0")}${dynamicPartPosition}00`
+            const dynamicPartWithLength = `${dynamicPartLength}${sig.data.slice(2)}`
+
+            signatureBytes += staticSignature
+            dynamicBytes += dynamicPartWithLength
+        } else {
+            signatureBytes += sig.data.slice(2)
+        }
     }
-    return signatureBytes
+
+    return signatureBytes + dynamicBytes
 }
 
 export const logGas = async (message: string, tx: Promise<any>, skip?: boolean): Promise<any> => {

test/core/Safe.Signatures.spec.ts

@@ -1,14 +1,17 @@
+import { getCompatFallbackHandler } from './../utils/setup';
+import { calculateSafeMessageHash, signHash, buildContractSignature } from './../../src/utils/execution';
 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, executeTx, safeSignMessage, calculateSafeTransactionHash, safeApproveHash, buildSafeTransaction, logGas, calculateSafeDomainSeparator, preimageSafeTransactionHash, buildSignatureBytes } from "../../src/utils/execution";
 import { chainId } from "../utils/encoding";
 
 describe("Safe", async () => {
 
-    const [user1, user2, user3, user4] = waffle.provider.getWallets()
+    const [user1, user2, user3, user4, user5] = waffle.provider.getWallets()
 
     const setupTests = deployments.createFixture(async ({ deployments }) => {
         await deployments.fixture();
@@ -429,18 +432,29 @@ describe("Safe", async () => {
 
         it('should be able to mix all signature types', async () => {
             await setupTests()
-            const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address])
+            const compatFallbackHandler = await getCompatFallbackHandler()
+            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 signerSafeOwnerSignature = await signHash(user5, safeMessageHash)
+            const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data)
+
             const signatures = buildSignatureBytes([
                 await safeApproveHash(user1, safe, tx, true),
                 await safeApproveHash(user4, safe, tx),
                 await safeSignTypedData(user2, safe, tx),
-                await safeSignTypedData(user3, safe, tx)
+                await safeSignTypedData(user3, safe, tx),
+                signerSafeSig,
             ])
 
-            await safe.checkNSignatures(txHash, txHashData, signatures, 3)
+            await safe.checkNSignatures(txHash, txHashData, signatures, 5)
         })
 
         it('should be able to require no signatures', async () => {
@@ -477,12 +491,28 @@ describe("Safe", async () => {
                 await safeApproveHash(user4, safe, tx),
                 await safeSignTypedData(user2, safe, tx)
             ])
-            // Should fail as only 3 signaures are provided
+            // Should fail as only 3 signatures are provided
             await expect(
                 safe.checkNSignatures(txHash, txHashData, signatures, 4)
             ).to.be.revertedWith("GS020")
 
             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 tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
+            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")
+        })
     })
 })