diff --git a/contracts/Safe.sol b/contracts/Safe.sol index 850445049..77a124098 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -289,6 +289,17 @@ contract Safe is * Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash. */ function checkSignatures(bytes32 dataHash, bytes memory signatures) public view { + checkSignatures(dataHash, "", signatures); + } + + /** + * @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise. + * @param dataHash Hash of the data (could be either a message hash or transaction hash) + * @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. + * @dev This function makes it compatible with previous versions. + */ + function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) public view { // Load threshold to avoid multiple storage loads uint256 _threshold = threshold; // Check that a threshold is set diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol index 49f52e658..9fae6ab3c 100644 --- a/contracts/handler/CompatibilityFallbackHandler.sol +++ b/contracts/handler/CompatibilityFallbackHandler.sol @@ -176,21 +176,4 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat ) public view { Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, signatures, requiredSignatures); } - - /** - * @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. - * The function was moved to the fallback handler as a part of - * 1.5.0 contract upgrade. It used to be a part of the Safe core contract, but - * was replaced by the same function that removes the raw encoded bytes data parameter. - * @param dataHash Hash of the data (could be either a message hash or transaction hash) - * @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. - */ - function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) public view { - Safe safe = Safe(payable(_manager())); - - uint256 threshold = safe.getThreshold(); - safe.checkNSignatures(_msgSender(), dataHash, signatures, threshold); - } } diff --git a/package.json b/package.json index a9d41845f..0c102d09c 100644 --- a/package.json +++ b/package.json @@ -29,6 +29,7 @@ "lint:sol:prettier": "prettier 'contracts/**/*.sol' --list-different", "lint:ts": "eslint 'src/**/*.ts' 'test/**/*.ts' --fix", "lint:ts:prettier": "prettier 'src/**/*.ts' 'test/**/*.ts' --list-different", + "fmt": "npm run fmt:sol && npm run fmt:ts", "fmt:sol": "prettier 'contracts/**/*.sol' -w", "fmt:ts": "prettier 'src/**/*.ts' 'test/**/*.ts' -w", "prepack": "npm run build", diff --git a/test/core/Safe.Signatures.spec.ts b/test/core/Safe.Signatures.spec.ts index 819e54dae..c7ef0a255 100644 --- a/test/core/Safe.Signatures.spec.ts +++ b/test/core/Safe.Signatures.spec.ts @@ -457,7 +457,6 @@ describe("Safe", () => { it("should fail if signature points into static part", async () => { const { safe, - safeWithCompatFbHandlerIface, signers: [user1], } = await setupTests(); const safeAddress = await safe.getAddress(); @@ -471,13 +470,12 @@ describe("Safe", () => { "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v "0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read - await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, "0x", signatures)).to.be.revertedWith("GS021"); + await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, "0x", signatures)).to.be.revertedWith("GS021"); }); it("should fail if signatures data is not present", async () => { const { safe, - safeWithCompatFbHandlerIface, signers: [user1], } = await setupTests(); const safeAddress = await safe.getAddress(); @@ -492,14 +490,13 @@ describe("Safe", () => { "0000000000000000000000000000000000000000000000000000000000000041" + "00"; // r, s, v - await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS022"); + await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS022"); }); it("should fail if signatures data is too short", async () => { const { safe, signers: [user1], - safeWithCompatFbHandlerIface, } = await setupTests(); const safeAddress = await safe.getAddress(); const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); @@ -514,37 +511,34 @@ describe("Safe", () => { "00" + // r, s, v "0000000000000000000000000000000000000000000000000000000000000020"; // length - await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS023"); + await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS023"); }); it("should not be able to use different chainId for signing", async () => { const { signers: [user1], - compatFallbackHandler, } = await setupTests(); - const safe = await getSafeWithOwners([user1.address], 1, await compatFallbackHandler.getAddress()); + const safe = await getSafeWithOwners([user1.address]); const safeAddress = await safe.getAddress(); - const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress); const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId()); const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); const signatures = buildSignatureBytes([await safeSignTypedData(user1, safeAddress, tx, 1)]); - await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS026"); + await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS026"); }); it("if not msg.sender on-chain approval is required", async () => { const { safe, - safeWithCompatFbHandlerIface, signers: [user1, user2], } = await setupTests(); const safeAddress = await safe.getAddress(); - const user2Safe = safeWithCompatFbHandlerIface.connect(user2); + const user2Safe = safe.connect(user2); const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId()); const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]); - await expect(user2Safe.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS025"); + await expect(user2Safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS025"); }); it("should revert if not the required amount of signature data is provided", async () => { @@ -558,11 +552,10 @@ describe("Safe", () => { await compatFallbackHandler.getAddress(), ); const safeAddress = await safe.getAddress(); - const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress); const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId()); const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); - await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, "0x")).to.be.revertedWith("GS020"); + await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, "0x")).to.be.revertedWith("GS020"); }); it("should not be able to use different signature type of same owner", async () => { @@ -576,7 +569,6 @@ describe("Safe", () => { await compatFallbackHandler.getAddress(), ); const safeAddress = await safe.getAddress(); - const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress); const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId()); const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); @@ -585,7 +577,7 @@ describe("Safe", () => { await safeSignTypedData(user1, safeAddress, tx), await safeSignTypedData(user3, safeAddress, tx), ]); - await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS026"); + await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS026"); }); it("should be able to mix all signature types", async () => { @@ -604,7 +596,6 @@ describe("Safe", () => { const safeAddress = await safe.getAddress(); const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); - const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress); const safeMessageHash = calculateSafeMessageHash(signerSafeAddress, txHash, await chainId()); const signerSafeOwnerSignature = await signHash(user5, safeMessageHash); @@ -618,7 +609,7 @@ describe("Safe", () => { signerSafeSig, ]); - await safeWithCompatFbHandlerIface.checkSignatures(txHash, "0x", signatures); + await safe["checkSignatures(bytes32,bytes,bytes)"](txHash, "0x", signatures); }); });