From 2fb16d87ff7a96076a1e394689a37c233d859e1c Mon Sep 17 00:00:00 2001 From: moisses89 Date: Tue, 14 Jun 2022 10:41:36 +0200 Subject: [PATCH 1/8] Add test for nested safes using EIP1271 signature --- .../integration/gnosisSafeNestedSafes.spec.ts | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 test/integration/gnosisSafeNestedSafes.spec.ts diff --git a/test/integration/gnosisSafeNestedSafes.spec.ts b/test/integration/gnosisSafeNestedSafes.spec.ts new file mode 100644 index 000000000..0858d3b16 --- /dev/null +++ b/test/integration/gnosisSafeNestedSafes.spec.ts @@ -0,0 +1,92 @@ +import { expect } from "chai"; +import hre, { deployments, waffle } from "hardhat"; +import "@nomiclabs/hardhat-ethers"; +import { AddressZero } from "@ethersproject/constants"; +import { parseEther } from "@ethersproject/units"; +import { defaultAbiCoder } from "@ethersproject/abi"; +import { getSafeWithOwners, getCompatFallbackHandler } from "../utils/setup"; +import { buildSignatureBytes, signHash} from "../../src/utils/execution"; + + +describe("NestedSafes", async () => { + const [user1, user2, user3, user4, user5] = waffle.provider.getWallets(); + + const setupTests = deployments.createFixture(async ({ deployments }) => { + await deployments.fixture(); + const handler = await getCompatFallbackHandler() + const safe1 = await getSafeWithOwners([user1.address, user2.address], 2, handler.address) + const safe2 = await getSafeWithOwners([user3.address, user4.address], 2, handler.address) + const parentSafe = await getSafeWithOwners([safe1.address, safe2.address], 2, handler.address) + const handlerSafe1 = handler.attach(safe1.address) + const handlerSafe2 = handler.attach(safe2.address) + return { + safe1, + safe2, + parentSafe, + handlerSafe1, + handlerSafe2 + } + }) + + it('should use EIP-1271 (contract signatures)', async () => { + const { safe1, safe2, parentSafe,handlerSafe1,handlerSafe2 } = await setupTests() + // Deposit some spare money for execution to owner safes + await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(0) + await user1.sendTransaction({to: safe1.address, value: parseEther("1")}) + await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(parseEther("1")) + + await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(0) + await user1.sendTransaction({to: safe2.address, value: parseEther("1")}) + await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(parseEther("1")) + + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) + await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) + + // Withdraw 1 ETH + const to = user5.address + const value = parseEther("1") + const data = "0x" + const operation = 0 + const nonce = await parentSafe.nonce() + const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) + + // Get hash transaction for each safe + const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address,messageData) + const messageHashSafe2 = await handlerSafe2.getMessageHashForSafe(safe2.address,messageData) + // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) + const sig1 = await signHash(user1, messageHashSafe1) + const sig2 = await signHash(user2, messageHashSafe1) + const sig3 = await signHash(user3, messageHashSafe2) + const sig4 = await signHash(user4, messageHashSafe2) + let signSafe1 = buildSignatureBytes([sig1, sig2]) + let signSafe2 = buildSignatureBytes([sig3, sig4]) + + + // Check if signature for each safe is correct + expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe1)).to.be.eq("0x20c13b0b") + expect(await handlerSafe2.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe2)).to.be.eq("0x20c13b0b") + + let staticPart = "0x" + // Pack signatures in correct order + if (safe1.address < safe2.address) { + staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + } else { + staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + } + + + let dynamicPart = + defaultAbiCoder.encode(['bytes'], [signSafe1]).slice(66)+ + defaultAbiCoder.encode(['bytes'], [signSafe2]).slice(66) + + let signature = staticPart + dynamicPart + // Should execute transaction withdraw 1 ether + expect(await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature)).to.be.ok + // Should be 0 + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) + }) + +}); \ No newline at end of file From f9d7218d3ddbe55591dc4f7c2c5be71e17d9d88e Mon Sep 17 00:00:00 2001 From: moisses89 Date: Tue, 14 Jun 2022 12:30:27 +0200 Subject: [PATCH 2/8] Add test to EIP1271 with pre-approve hash --- .../integration/gnosisSafeNestedSafes.spec.ts | 72 ++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/test/integration/gnosisSafeNestedSafes.spec.ts b/test/integration/gnosisSafeNestedSafes.spec.ts index 0858d3b16..438e99db6 100644 --- a/test/integration/gnosisSafeNestedSafes.spec.ts +++ b/test/integration/gnosisSafeNestedSafes.spec.ts @@ -5,7 +5,7 @@ import { AddressZero } from "@ethersproject/constants"; import { parseEther } from "@ethersproject/units"; import { defaultAbiCoder } from "@ethersproject/abi"; import { getSafeWithOwners, getCompatFallbackHandler } from "../utils/setup"; -import { buildSignatureBytes, signHash} from "../../src/utils/execution"; +import { buildSignatureBytes, signHash, executeContractCallWithSigners} from "../../src/utils/execution"; describe("NestedSafes", async () => { @@ -13,6 +13,7 @@ describe("NestedSafes", async () => { const setupTests = deployments.createFixture(async ({ deployments }) => { await deployments.fixture(); + const signLib = await (await hre.ethers.getContractFactory("SignMessageLib")).deploy(); const handler = await getCompatFallbackHandler() const safe1 = await getSafeWithOwners([user1.address, user2.address], 2, handler.address) const safe2 = await getSafeWithOwners([user3.address, user4.address], 2, handler.address) @@ -24,7 +25,8 @@ describe("NestedSafes", async () => { safe2, parentSafe, handlerSafe1, - handlerSafe2 + handlerSafe2, + signLib } }) @@ -89,4 +91,70 @@ describe("NestedSafes", async () => { await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) }) + it('should revert with hash not approved ', async () => { + const { safe1, safe2, parentSafe,handlerSafe1,handlerSafe2, signLib} = await setupTests() + // Deposit some spare money for execution to owner safes + await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(0) + await user1.sendTransaction({to: safe1.address, value: parseEther("1")}) + await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(parseEther("1")) + + await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(0) + await user1.sendTransaction({to: safe2.address, value: parseEther("1")}) + await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(parseEther("1")) + + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) + await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) + + // Withdraw 1 ETH + const to = user5.address + const value = parseEther("1") + const data = "0x" + const operation = 0 + const nonce = await parentSafe.nonce() + const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) + + // Get hash transaction for each safe + const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address,messageData) + + // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) + const sig1 = await signHash(user1, messageHashSafe1) + const sig2 = await signHash(user2, messageHashSafe1) + + let signSafe1 = buildSignatureBytes([sig1, sig2]) + + + // Check if signature for each safe is correct + expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe1)).to.be.eq("0x20c13b0b") + + let staticPart = "0x" + // Pack signatures in correct order + if (safe1.address < safe2.address) { + staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + } else { + staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + } + + + let dynamicPart = + defaultAbiCoder.encode(['bytes'], [signSafe1]).slice(66)+ + "0000000000000000000000000000000000000000000000000000000000000000" + + let signature = staticPart + dynamicPart + // Should revert with message hash not approved + await expect(parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature), + "Transaction should fail because hash is not approved").to.be.reverted; + + // Approve transaction from safe2 + await executeContractCallWithSigners(safe2, signLib, "signMessage", [messageData], [user3, user4], true) + + // Execute Transaction + await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature) + + // Should be 0 + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) + }) + }); \ No newline at end of file From d327e940b33dff45e40b075c3e008a2f12e29bb3 Mon Sep 17 00:00:00 2001 From: moisses89 Date: Tue, 14 Jun 2022 14:12:01 +0200 Subject: [PATCH 3/8] Fix some lint issues --- .../integration/gnosisSafeNestedSafes.spec.ts | 63 ++++++++----------- 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/test/integration/gnosisSafeNestedSafes.spec.ts b/test/integration/gnosisSafeNestedSafes.spec.ts index 438e99db6..be2c26277 100644 --- a/test/integration/gnosisSafeNestedSafes.spec.ts +++ b/test/integration/gnosisSafeNestedSafes.spec.ts @@ -20,18 +20,27 @@ describe("NestedSafes", async () => { const parentSafe = await getSafeWithOwners([safe1.address, safe2.address], 2, handler.address) const handlerSafe1 = handler.attach(safe1.address) const handlerSafe2 = handler.attach(safe2.address) + let staticPart = "0x" + if (safe1.address < safe2.address) { + staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + } else { + staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + } return { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, - signLib + signLib, + staticPart } }) it('should use EIP-1271 (contract signatures)', async () => { - const { safe1, safe2, parentSafe,handlerSafe1,handlerSafe2 } = await setupTests() + const { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, staticPart} = await setupTests() // Deposit some spare money for execution to owner safes await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(0) await user1.sendTransaction({to: safe1.address, value: parseEther("1")}) @@ -54,45 +63,36 @@ describe("NestedSafes", async () => { const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) // Get hash transaction for each safe - const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address,messageData) - const messageHashSafe2 = await handlerSafe2.getMessageHashForSafe(safe2.address,messageData) + const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData) + const messageHashSafe2 = await handlerSafe2.getMessageHashForSafe(safe2.address, messageData) + // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) const sig1 = await signHash(user1, messageHashSafe1) const sig2 = await signHash(user2, messageHashSafe1) const sig3 = await signHash(user3, messageHashSafe2) const sig4 = await signHash(user4, messageHashSafe2) - let signSafe1 = buildSignatureBytes([sig1, sig2]) - let signSafe2 = buildSignatureBytes([sig3, sig4]) + const signSafe1 = buildSignatureBytes([sig1, sig2]) + const signSafe2 = buildSignatureBytes([sig3, sig4]) // Check if signature for each safe is correct expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe1)).to.be.eq("0x20c13b0b") expect(await handlerSafe2.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe2)).to.be.eq("0x20c13b0b") - let staticPart = "0x" - // Pack signatures in correct order - if (safe1.address < safe2.address) { - staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v - staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v - } else { - staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v - staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v - } - - - let dynamicPart = + const dynamicPart = defaultAbiCoder.encode(['bytes'], [signSafe1]).slice(66)+ defaultAbiCoder.encode(['bytes'], [signSafe2]).slice(66) + const signature = staticPart + dynamicPart - let signature = staticPart + dynamicPart // Should execute transaction withdraw 1 ether expect(await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature)).to.be.ok + // Should be 0 await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) }) it('should revert with hash not approved ', async () => { - const { safe1, safe2, parentSafe,handlerSafe1,handlerSafe2, signLib} = await setupTests() + const { safe1, safe2, parentSafe, handlerSafe1, signLib, staticPart} = await setupTests() // Deposit some spare money for execution to owner safes await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(0) await user1.sendTransaction({to: safe1.address, value: parseEther("1")}) @@ -115,34 +115,21 @@ describe("NestedSafes", async () => { const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) // Get hash transaction for each safe - const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address,messageData) + const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData) // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) const sig1 = await signHash(user1, messageHashSafe1) const sig2 = await signHash(user2, messageHashSafe1) - - let signSafe1 = buildSignatureBytes([sig1, sig2]) - - + const signSafe1 = buildSignatureBytes([sig1, sig2]) + // Check if signature for each safe is correct expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe1)).to.be.eq("0x20c13b0b") - let staticPart = "0x" - // Pack signatures in correct order - if (safe1.address < safe2.address) { - staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v - staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v - } else { - staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v - staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v - } - - - let dynamicPart = + const dynamicPart = defaultAbiCoder.encode(['bytes'], [signSafe1]).slice(66)+ "0000000000000000000000000000000000000000000000000000000000000000" + const signature = staticPart + dynamicPart - let signature = staticPart + dynamicPart // Should revert with message hash not approved await expect(parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature), "Transaction should fail because hash is not approved").to.be.reverted; From 8bf9fd35bf02d129e715bc4ada3509c21c62894b Mon Sep 17 00:00:00 2001 From: moisses89 Date: Wed, 15 Jun 2022 13:53:18 +0200 Subject: [PATCH 4/8] Modify revert to check the revert message and refactor staticPart --- test/integration/gnosisSafeNestedSafes.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/integration/gnosisSafeNestedSafes.spec.ts b/test/integration/gnosisSafeNestedSafes.spec.ts index be2c26277..d0d945d59 100644 --- a/test/integration/gnosisSafeNestedSafes.spec.ts +++ b/test/integration/gnosisSafeNestedSafes.spec.ts @@ -21,12 +21,12 @@ describe("NestedSafes", async () => { const handlerSafe1 = handler.attach(safe1.address) const handlerSafe2 = handler.attach(safe2.address) let staticPart = "0x" + const staticPartSafe1 = "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + const staticPartSafe2 = "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v if (safe1.address < safe2.address) { - staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v - staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v + staticPart += staticPartSafe1 + staticPartSafe2 } else { - staticPart += "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v - staticPart += "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v + staticPart += staticPartSafe2 + staticPartSafe1 } return { safe1, @@ -132,7 +132,7 @@ describe("NestedSafes", async () => { // Should revert with message hash not approved await expect(parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature), - "Transaction should fail because hash is not approved").to.be.reverted; + "Transaction should fail because hash is not approved").to.be.revertedWith("Hash not approved"); // Approve transaction from safe2 await executeContractCallWithSigners(safe2, signLib, "signMessage", [messageData], [user3, user4], true) From eb001ad148728a60ef57bbdf5a110852d701c2f8 Mon Sep 17 00:00:00 2001 From: moisses89 Date: Fri, 10 Feb 2023 13:53:53 +0100 Subject: [PATCH 5/8] Generate contract signature using buildContractSignature --- .../integration/gnosisSafeNestedSafes.spec.ts | 70 ++++++------------- 1 file changed, 23 insertions(+), 47 deletions(-) diff --git a/test/integration/gnosisSafeNestedSafes.spec.ts b/test/integration/gnosisSafeNestedSafes.spec.ts index d0d945d59..94770ee6c 100644 --- a/test/integration/gnosisSafeNestedSafes.spec.ts +++ b/test/integration/gnosisSafeNestedSafes.spec.ts @@ -5,7 +5,7 @@ import { AddressZero } from "@ethersproject/constants"; import { parseEther } from "@ethersproject/units"; import { defaultAbiCoder } from "@ethersproject/abi"; import { getSafeWithOwners, getCompatFallbackHandler } from "../utils/setup"; -import { buildSignatureBytes, signHash, executeContractCallWithSigners} from "../../src/utils/execution"; +import { buildSignatureBytes, signHash, executeContractCallWithSigners, buildContractSignature, SafeSignature} from "../../src/utils/execution"; describe("NestedSafes", async () => { @@ -20,36 +20,19 @@ describe("NestedSafes", async () => { const parentSafe = await getSafeWithOwners([safe1.address, safe2.address], 2, handler.address) const handlerSafe1 = handler.attach(safe1.address) const handlerSafe2 = handler.attach(safe2.address) - let staticPart = "0x" - const staticPartSafe1 = "000000000000000000000000" + safe1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000082" + "00" // r, s, v - const staticPartSafe2 = "000000000000000000000000" + safe2.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000142" + "00" // r, s, v - if (safe1.address < safe2.address) { - staticPart += staticPartSafe1 + staticPartSafe2 - } else { - staticPart += staticPartSafe2 + staticPartSafe1 - } return { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, - signLib, - staticPart + signLib } }) it('should use EIP-1271 (contract signatures)', async () => { const { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, staticPart} = await setupTests() - // Deposit some spare money for execution to owner safes - await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(0) - await user1.sendTransaction({to: safe1.address, value: parseEther("1")}) - await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(parseEther("1")) - - await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(0) - await user1.sendTransaction({to: safe2.address, value: parseEther("1")}) - await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(parseEther("1")) - + // Deposit some spare money for execution to parent safe await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) @@ -71,19 +54,17 @@ describe("NestedSafes", async () => { const sig2 = await signHash(user2, messageHashSafe1) const sig3 = await signHash(user3, messageHashSafe2) const sig4 = await signHash(user4, messageHashSafe2) - const signSafe1 = buildSignatureBytes([sig1, sig2]) - const signSafe2 = buildSignatureBytes([sig3, sig4]) + + const ownersSignSafe1 = buildSignatureBytes([sig1, sig2]) + const ownersSignSafe2 = buildSignatureBytes([sig3, sig4]) - // Check if signature for each safe is correct - expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe1)).to.be.eq("0x20c13b0b") - expect(await handlerSafe2.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe2)).to.be.eq("0x20c13b0b") - - const dynamicPart = - defaultAbiCoder.encode(['bytes'], [signSafe1]).slice(66)+ - defaultAbiCoder.encode(['bytes'], [signSafe2]).slice(66) - const signature = staticPart + dynamicPart + expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, ownersSignSafe1)).to.be.eq("0x20c13b0b") + expect(await handlerSafe2.callStatic['isValidSignature(bytes,bytes)'](messageData, ownersSignSafe2)).to.be.eq("0x20c13b0b") + const signerSafe1 = buildContractSignature(safe1.address, ownersSignSafe1) + const signerSafe2 = buildContractSignature(safe2.address, ownersSignSafe2) + const signature = buildSignatureBytes([signerSafe1, signerSafe2]) // Should execute transaction withdraw 1 ether expect(await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature)).to.be.ok @@ -92,16 +73,8 @@ describe("NestedSafes", async () => { }) it('should revert with hash not approved ', async () => { - const { safe1, safe2, parentSafe, handlerSafe1, signLib, staticPart} = await setupTests() - // Deposit some spare money for execution to owner safes - await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(0) - await user1.sendTransaction({to: safe1.address, value: parseEther("1")}) - await expect(await hre.ethers.provider.getBalance(safe1.address)).to.be.equal(parseEther("1")) - - await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(0) - await user1.sendTransaction({to: safe2.address, value: parseEther("1")}) - await expect(await hre.ethers.provider.getBalance(safe2.address)).to.be.equal(parseEther("1")) - + const { safe1, safe2, parentSafe, handlerSafe1, signLib} = await setupTests() + // Deposit some spare money for execution to parent safe await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) @@ -120,16 +93,19 @@ describe("NestedSafes", async () => { // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) const sig1 = await signHash(user1, messageHashSafe1) const sig2 = await signHash(user2, messageHashSafe1) - const signSafe1 = buildSignatureBytes([sig1, sig2]) + const ownersSignSafe1 = buildSignatureBytes([sig1, sig2]) // Check if signature for each safe is correct - expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, signSafe1)).to.be.eq("0x20c13b0b") - - const dynamicPart = - defaultAbiCoder.encode(['bytes'], [signSafe1]).slice(66)+ - "0000000000000000000000000000000000000000000000000000000000000000" - const signature = staticPart + dynamicPart + expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, ownersSignSafe1)).to.be.eq("0x20c13b0b") + const signerSafe1 = buildContractSignature(safe1.address, ownersSignSafe1) + //Create an empty signature to with dynamic true to include this address in the staticpart + const emptySignSafe2: SafeSignature = { + signer: safe2.address, + data: "", + dynamic: true, + }; + const signature = buildSignatureBytes([signerSafe1, emptySignSafe2]) // Should revert with message hash not approved await expect(parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature), "Transaction should fail because hash is not approved").to.be.revertedWith("Hash not approved"); From 4862018c095806c0c162515a34f45cbe185ce6d9 Mon Sep 17 00:00:00 2001 From: moisses89 Date: Fri, 10 Feb 2023 16:13:56 +0100 Subject: [PATCH 6/8] Rename test file --- .../{gnosisSafeNestedSafes.spec.ts => Safe.NestedSafes.spec.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/integration/{gnosisSafeNestedSafes.spec.ts => Safe.NestedSafes.spec.ts} (100%) diff --git a/test/integration/gnosisSafeNestedSafes.spec.ts b/test/integration/Safe.NestedSafes.spec.ts similarity index 100% rename from test/integration/gnosisSafeNestedSafes.spec.ts rename to test/integration/Safe.NestedSafes.spec.ts From b67711559e752c507be1da4b3615ba320fae63a2 Mon Sep 17 00:00:00 2001 From: moisses89 Date: Fri, 10 Feb 2023 17:04:45 +0100 Subject: [PATCH 7/8] Fix unnecessary space Modify comments --- test/integration/Safe.NestedSafes.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/integration/Safe.NestedSafes.spec.ts b/test/integration/Safe.NestedSafes.spec.ts index 94770ee6c..1ab3e1082 100644 --- a/test/integration/Safe.NestedSafes.spec.ts +++ b/test/integration/Safe.NestedSafes.spec.ts @@ -32,7 +32,7 @@ describe("NestedSafes", async () => { it('should use EIP-1271 (contract signatures)', async () => { const { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, staticPart} = await setupTests() - // Deposit some spare money for execution to parent safe + // Deposit some spare money to test transfer await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) @@ -45,7 +45,7 @@ describe("NestedSafes", async () => { const nonce = await parentSafe.nonce() const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) - // Get hash transaction for each safe + // Get message hash for each safe const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData) const messageHashSafe2 = await handlerSafe2.getMessageHashForSafe(safe2.address, messageData) @@ -72,9 +72,9 @@ describe("NestedSafes", async () => { await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) }) - it('should revert with hash not approved ', async () => { + it('should revert with hash not approved', async () => { const { safe1, safe2, parentSafe, handlerSafe1, signLib} = await setupTests() - // Deposit some spare money for execution to parent safe + // Deposit some spare money to test transfer await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) @@ -87,7 +87,7 @@ describe("NestedSafes", async () => { const nonce = await parentSafe.nonce() const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) - // Get hash transaction for each safe + // Get message hash for safe const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData) // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) From a8c27b2d9ba25bbe1abe1bcb4eddb0c0d555df4a Mon Sep 17 00:00:00 2001 From: moisses89 Date: Fri, 10 Feb 2023 17:08:08 +0100 Subject: [PATCH 8/8] Lint NestedSafes test file --- test/integration/Safe.NestedSafes.spec.ts | 166 +++++++++++----------- 1 file changed, 86 insertions(+), 80 deletions(-) diff --git a/test/integration/Safe.NestedSafes.spec.ts b/test/integration/Safe.NestedSafes.spec.ts index 1ab3e1082..b7b89421b 100644 --- a/test/integration/Safe.NestedSafes.spec.ts +++ b/test/integration/Safe.NestedSafes.spec.ts @@ -5,119 +5,125 @@ import { AddressZero } from "@ethersproject/constants"; import { parseEther } from "@ethersproject/units"; import { defaultAbiCoder } from "@ethersproject/abi"; import { getSafeWithOwners, getCompatFallbackHandler } from "../utils/setup"; -import { buildSignatureBytes, signHash, executeContractCallWithSigners, buildContractSignature, SafeSignature} from "../../src/utils/execution"; - +import { + buildSignatureBytes, + signHash, + executeContractCallWithSigners, + buildContractSignature, + SafeSignature, +} from "../../src/utils/execution"; describe("NestedSafes", async () => { const [user1, user2, user3, user4, user5] = waffle.provider.getWallets(); - + const setupTests = deployments.createFixture(async ({ deployments }) => { await deployments.fixture(); const signLib = await (await hre.ethers.getContractFactory("SignMessageLib")).deploy(); - const handler = await getCompatFallbackHandler() - const safe1 = await getSafeWithOwners([user1.address, user2.address], 2, handler.address) - const safe2 = await getSafeWithOwners([user3.address, user4.address], 2, handler.address) - const parentSafe = await getSafeWithOwners([safe1.address, safe2.address], 2, handler.address) - const handlerSafe1 = handler.attach(safe1.address) - const handlerSafe2 = handler.attach(safe2.address) + const handler = await getCompatFallbackHandler(); + const safe1 = await getSafeWithOwners([user1.address, user2.address], 2, handler.address); + const safe2 = await getSafeWithOwners([user3.address, user4.address], 2, handler.address); + const parentSafe = await getSafeWithOwners([safe1.address, safe2.address], 2, handler.address); + const handlerSafe1 = handler.attach(safe1.address); + const handlerSafe2 = handler.attach(safe2.address); return { safe1, safe2, - parentSafe, + parentSafe, handlerSafe1, handlerSafe2, - signLib - } - }) + signLib, + }; + }); - it('should use EIP-1271 (contract signatures)', async () => { - const { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, staticPart} = await setupTests() - // Deposit some spare money to test transfer - await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) - await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) - await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) + it("should use EIP-1271 (contract signatures)", async () => { + const { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, staticPart } = await setupTests(); + // Deposit some spare money to test transfer + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0); + await user1.sendTransaction({ to: parentSafe.address, value: parseEther("1") }); + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")); // Withdraw 1 ETH - const to = user5.address - const value = parseEther("1") - const data = "0x" - const operation = 0 - const nonce = await parentSafe.nonce() - const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) - + const to = user5.address; + const value = parseEther("1"); + const data = "0x"; + const operation = 0; + const nonce = await parentSafe.nonce(); + const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce); + // Get message hash for each safe - const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData) - const messageHashSafe2 = await handlerSafe2.getMessageHashForSafe(safe2.address, messageData) - + const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData); + const messageHashSafe2 = await handlerSafe2.getMessageHashForSafe(safe2.address, messageData); + // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) - const sig1 = await signHash(user1, messageHashSafe1) - const sig2 = await signHash(user2, messageHashSafe1) - const sig3 = await signHash(user3, messageHashSafe2) - const sig4 = await signHash(user4, messageHashSafe2) - - const ownersSignSafe1 = buildSignatureBytes([sig1, sig2]) - const ownersSignSafe2 = buildSignatureBytes([sig3, sig4]) - + const sig1 = await signHash(user1, messageHashSafe1); + const sig2 = await signHash(user2, messageHashSafe1); + const sig3 = await signHash(user3, messageHashSafe2); + const sig4 = await signHash(user4, messageHashSafe2); + + const ownersSignSafe1 = buildSignatureBytes([sig1, sig2]); + const ownersSignSafe2 = buildSignatureBytes([sig3, sig4]); + // Check if signature for each safe is correct - expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, ownersSignSafe1)).to.be.eq("0x20c13b0b") - expect(await handlerSafe2.callStatic['isValidSignature(bytes,bytes)'](messageData, ownersSignSafe2)).to.be.eq("0x20c13b0b") + expect(await handlerSafe1.callStatic["isValidSignature(bytes,bytes)"](messageData, ownersSignSafe1)).to.be.eq("0x20c13b0b"); + expect(await handlerSafe2.callStatic["isValidSignature(bytes,bytes)"](messageData, ownersSignSafe2)).to.be.eq("0x20c13b0b"); - const signerSafe1 = buildContractSignature(safe1.address, ownersSignSafe1) - const signerSafe2 = buildContractSignature(safe2.address, ownersSignSafe2) - const signature = buildSignatureBytes([signerSafe1, signerSafe2]) + const signerSafe1 = buildContractSignature(safe1.address, ownersSignSafe1); + const signerSafe2 = buildContractSignature(safe2.address, ownersSignSafe2); + const signature = buildSignatureBytes([signerSafe1, signerSafe2]); // Should execute transaction withdraw 1 ether - expect(await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature)).to.be.ok - - // Should be 0 - await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) - }) - - it('should revert with hash not approved', async () => { - const { safe1, safe2, parentSafe, handlerSafe1, signLib} = await setupTests() - // Deposit some spare money to test transfer - await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0) - await user1.sendTransaction({to: parentSafe.address, value: parseEther("1")}) - await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")) + expect(await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature)).to.be.ok; + + // Should be 0 + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")); + }); + + it("should revert with hash not approved", async () => { + const { safe1, safe2, parentSafe, handlerSafe1, signLib } = await setupTests(); + // Deposit some spare money to test transfer + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(0); + await user1.sendTransaction({ to: parentSafe.address, value: parseEther("1") }); + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.equal(parseEther("1")); // Withdraw 1 ETH - const to = user5.address - const value = parseEther("1") - const data = "0x" - const operation = 0 - const nonce = await parentSafe.nonce() - const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) - + const to = user5.address; + const value = parseEther("1"); + const data = "0x"; + const operation = 0; + const nonce = await parentSafe.nonce(); + const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce); + // Get message hash for safe - const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData) + const messageHashSafe1 = await handlerSafe1.getMessageHashForSafe(safe1.address, messageData); // Get all signs for each owner Safe1 (user1, user2) Safe2 (user3, user4) - const sig1 = await signHash(user1, messageHashSafe1) - const sig2 = await signHash(user2, messageHashSafe1) - const ownersSignSafe1 = buildSignatureBytes([sig1, sig2]) - + const sig1 = await signHash(user1, messageHashSafe1); + const sig2 = await signHash(user2, messageHashSafe1); + const ownersSignSafe1 = buildSignatureBytes([sig1, sig2]); + // Check if signature for each safe is correct - expect(await handlerSafe1.callStatic['isValidSignature(bytes,bytes)'](messageData, ownersSignSafe1)).to.be.eq("0x20c13b0b") + expect(await handlerSafe1.callStatic["isValidSignature(bytes,bytes)"](messageData, ownersSignSafe1)).to.be.eq("0x20c13b0b"); - const signerSafe1 = buildContractSignature(safe1.address, ownersSignSafe1) + const signerSafe1 = buildContractSignature(safe1.address, ownersSignSafe1); //Create an empty signature to with dynamic true to include this address in the staticpart const emptySignSafe2: SafeSignature = { signer: safe2.address, data: "", dynamic: true, }; - const signature = buildSignatureBytes([signerSafe1, emptySignSafe2]) + const signature = buildSignatureBytes([signerSafe1, emptySignSafe2]); // Should revert with message hash not approved - await expect(parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature), - "Transaction should fail because hash is not approved").to.be.revertedWith("Hash not approved"); - + await expect( + parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature), + "Transaction should fail because hash is not approved", + ).to.be.revertedWith("Hash not approved"); + // Approve transaction from safe2 - await executeContractCallWithSigners(safe2, signLib, "signMessage", [messageData], [user3, user4], true) - - // Execute Transaction - await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature) + await executeContractCallWithSigners(safe2, signLib, "signMessage", [messageData], [user3, user4], true); - // Should be 0 - await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) - }) + // Execute Transaction + await parentSafe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, signature); -}); \ No newline at end of file + // Should be 0 + await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")); + }); +});