Merge pull request #10 from dodger213/fix-754-signature-length-check

Fix 754 signature length check

.github/workflows/certora.yml

@@ -15,7 +15,8 @@ jobs:
         runs-on: ubuntu-latest
         strategy:
             matrix:
-                rule: ["owner", "safe", "module", "nativeTokenRefund", "signatures"]
+
+                rule: ["verifyOwners.sh", "verifySafe.sh", "verifyModules.sh", "verifyNativeTokenRefund.sh"]
 
         steps:
             - uses: actions/checkout@v4

benchmark/utils/setup.ts

@@ -12,6 +12,7 @@ import {
 } from "../../src/utils/execution";
 import { AddressZero } from "@ethersproject/constants";
 import { Safe, SafeL2 } from "../../typechain-types";
+import { sign } from "crypto";
 
 type SafeSingleton = Safe | SafeL2;
 
@@ -30,8 +31,9 @@ const generateTarget = async (owners: number, threshold: number, guardAddress: s
         fallbackHandler: fallbackHandlerAddress,
         logGasUsage,
         saltNumber,
-    });
-    await executeContractCallWithSigners(safe, safe, "setGuard", [guardAddress], signers);
+
+    );
+    await executeContractCallWithSigners(safe, safe, "setGuard", [guardAddress], signers.slice(0, threshold));
     return safe;
 };
 

certora/specs/Signatures.spec

@@ -11,7 +11,7 @@ methods {
 
     // summaries
     function SignatureDecoder.signatureSplit(bytes memory signatures, uint256 pos) internal returns (uint8,bytes32,bytes32) => signatureSplitGhost(signatures,pos);
-    function Safe.checkContractSignature(address, bytes32, bytes memory, uint256) internal => NONDET;
+    function Safe.checkContractSignature(address, bytes32, bytes memory, uint256) internal returns (uint256) => NONDET;
     // needed for the execTransaction <> signatures rule
     function Safe.getTransactionHash(
         address to,

contracts/Safe.sol

@@ -240,8 +240,14 @@ contract Safe is
      * @param dataHash Hash of the data (could be either a message hash or transaction hash)
      * @param signatures Signature data that should be verified.
      * @param offset Offset to the start of the contract signature in the signatures byte array
+     * @return newOffset The new offset that points to the end of the contract signature
      */
-    function checkContractSignature(address owner, bytes32 dataHash, bytes memory signatures, uint256 offset) internal view {
+    function checkContractSignature(
+        address owner,
+        bytes32 dataHash,
+        bytes memory signatures,
+        uint256 offset
+    ) internal view returns (uint256 newOffset) {
         // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
         if (offset.add(32) > signatures.length) revertWithError("GS022");
 
@@ -252,8 +258,9 @@ contract Safe is
         assembly {
             contractSignatureLen := mload(add(add(signatures, offset), 0x20))
         }
+        newOffset = offset.add(32).add(contractSignatureLen);
         /* solhint-enable no-inline-assembly */
-        if (offset.add(32).add(contractSignatureLen) > signatures.length) revertWithError("GS023");
+        if (newOffset > signatures.length) revertWithError("GS023");
 
         // Check signature
         bytes memory contractSignature;
@@ -288,8 +295,9 @@ contract Safe is
         bytes memory signatures,
         uint256 requiredSignatures
     ) public view override {
+        uint256 offset = requiredSignatures.mul(65);
         // Check that the provided signature data is not too short
-        if (signatures.length < requiredSignatures.mul(65)) revertWithError("GS020");
+        if (signatures.length < offset) revertWithError("GS020");
         // There cannot be an owner with address 0.
         address lastOwner = address(0);
         address currentOwner;
@@ -304,16 +312,14 @@ contract Safe is
                 // When handling contract signatures the address of the contract is encoded into r
                 currentOwner = address(uint160(uint256(r)));
 
-                // Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
-                // This check is not completely accurate, since it is possible that more signatures than the threshold are send.
-                // Here we only check that the pointer is not pointing inside the part that is being processed
-                if (uint256(s) < requiredSignatures.mul(65)) revertWithError("GS021");
+                // Require that the signature data pointer is pointing to the expected location, at the end of processed contract signatures.
+                if (uint256(s) != offset) revertWithError("GS021");
 
                 // The contract signature check is extracted to a separate function for better compatibility with formal verification
                 // A quote from the Certora team:
                 // "The assembly code broke the pointer analysis, which switched the prover in failsafe mode, where it is (a) much slower and (b) computes different hashes than in the normal mode."
                 // More info here: https://github.com/safe-global/safe-smart-account/pull/661
-                checkContractSignature(currentOwner, dataHash, signatures, uint256(s));
+                offset = checkContractSignature(currentOwner, dataHash, signatures, uint256(s));
             } 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
@@ -333,6 +339,9 @@ contract Safe is
                 revertWithError("GS026");
             lastOwner = currentOwner;
         }
+        // if the signature is longer than the offset, it means that there are extra bytes not used in the signature
+        // A side effect of this check is that it will fail if the signatures count sent in the transaction is more than the required threshold
+        if (signatures.length != offset) revertWithError("GS028");
     }
 
     /**

docs/error_codes.md

@@ -19,6 +19,9 @@
 - `GS024`: `Invalid contract signature provided`
 - `GS025`: `Hash has not been approved`
 - `GS026`: `Invalid owner provided`
+- `GS028`: `Invalid signature length`
+
+Deprecated:  `GS027`: `Invalid signature provided`
 
 ### General auth related
 - `GS030`: `Only owners can approve a hash`

test/core/Safe.Signatures.spec.ts

@@ -426,6 +426,35 @@ describe("Safe", () => {
             await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS026");
         });
 
+        it("should not be able pass more signatures than required threshold", async () => {
+            const {
+                signers: [user1, user2, user3, user4],
+            } = await setupTests();
+            const compatFallbackHandler = await getCompatFallbackHandler();
+            const compatFallbackHandlerAddress = await compatFallbackHandler.getAddress();
+            const signerSafe = await getSafeWithOwners([user4.address], 1, compatFallbackHandlerAddress);
+            const signerSafeAddress = await signerSafe.getAddress();
+            const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, signerSafeAddress], 1);
+            const safeAddress = await safe.getAddress();
+            const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
+            const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
+
+            const safeMessageHash = calculateSafeMessageHash(signerSafeAddress, txHash, await chainId());
+            const signerSafeOwnerSignature = await signHash(user4, safeMessageHash);
+            const signerSafeSig = buildContractSignature(signerSafeAddress, signerSafeOwnerSignature.data);
+
+            const signatures = buildSignatureBytes([
+                await safeApproveHash(user1, safe, tx, true),
+                await safeSignTypedData(user2, safeAddress, tx),
+            ]);
+
+            await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS028");
+
+            const signatures2 = buildSignatureBytes([signerSafeSig, await safeSignTypedData(user3, safeAddress, tx)]);
+
+            await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures2)).to.be.revertedWith(new RegExp("GS028|GS021"));
+        });
+
         it("should be able to mix all signature types", async () => {
             const {
                 signers: [user1, user2, user3, user4, user5],
@@ -692,6 +721,31 @@ describe("Safe", () => {
             );
         });
 
+        it("should revert if signature contains additional bytes than required", async () => {
+            const {
+                safe,
+                signers: [user1],
+            } = await setupTests();
+            const safeAddress = await safe.getAddress();
+            const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
+            const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
+
+            const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]);
+
+            await expect(
+                safe["checkNSignatures(address,bytes32,bytes,uint256)"](user1.address, txHash, signatures.concat("00"), 1),
+            ).to.be.revertedWith("GS028");
+
+            await expect(
+                safe["checkNSignatures(address,bytes32,bytes,uint256)"](
+                    user1.address,
+                    txHash,
+                    signatures.concat(ethers.hexlify(ethers.randomBytes(Math.random() * 100)).slice(2)),
+                    1,
+                ),
+            ).to.be.revertedWith("GS028");
+        });
+
         it("should not be able to use different chainId for signing", async () => {
             const {
                 safe,

test/handlers/CompatibilityFallbackHandler.spec.ts

@@ -121,12 +121,19 @@ describe("CompatibilityFallbackHandler", () => {
             const signerSafeMessageHash = calculateSafeMessageHash(signerSafeAddress, validatorSafeMessageHash, await chainId());
 
             const signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash);
-
             const signerSafeSig = buildContractSignature(signerSafeAddress, signerSafeOwnerSignature.data);
 
-            expect(
-                await validator.isValidSignature.staticCall(dataHash, buildSignatureBytes([typedDataSig, ethSignSig, signerSafeSig])),
-            ).to.be.eq("0x1626ba7e");
+            expect(await validator.isValidSignature.staticCall(dataHash, buildSignatureBytes([typedDataSig, ethSignSig]))).to.be.eq(
+                "0x1626ba7e",
+            );
+
+            expect(await validator.isValidSignature.staticCall(dataHash, buildSignatureBytes([signerSafeSig, ethSignSig]))).to.be.eq(
+                "0x1626ba7e",
+            );
+
+            expect(await validator.isValidSignature.staticCall(dataHash, buildSignatureBytes([typedDataSig, signerSafeSig]))).to.be.eq(
+                "0x1626ba7e",
+            );
         });
     });
 

test/integration/Safe.0xExploit.spec.ts

@@ -1,8 +1,8 @@
 import { expect } from "chai";
 import hre, { deployments, ethers } from "hardhat";
 import { AddressZero } from "@ethersproject/constants";
-import { defaultAbiCoder } from "@ethersproject/abi";
-import { getSafe, deployContract, getCompatFallbackHandler } from "../utils/setup";
+import { getSafeWithOwners, deployContract, getCompatFallbackHandler } from "../utils/setup";
+
 import { buildSignatureBytes, executeContractCallWithSigners, signHash } from "../../src/utils/execution";
 
 describe("Safe", () => {
@@ -53,9 +53,8 @@ describe("Safe", () => {
             // Use off-chain Safe signature
             const transactionHash = await safe.getTransactionHash(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce);
             const messageHash = await messageHandler.getMessageHash(transactionHash);
-            const ownerSigs = await buildSignatureBytes([await signHash(user1, messageHash), await signHash(user2, messageHash)]);
-            const encodedOwnerSigns = defaultAbiCoder.encode(["bytes"], [ownerSigs]).slice(66);
-
+            const ownerSigs = buildSignatureBytes([await signHash(user1, messageHash), await signHash(user2, messageHash)]);
+            const encodedOwnerSigns = ((ownerSigs.length - 2) / 2).toString(16).padStart(64, "00") + ownerSigs.slice(2);
             // Use EOA owner
             let sigs =
                 "0x" +
@@ -80,7 +79,6 @@ describe("Safe", () => {
                 "0000000000000000000000000000000000000000000000000000000000000041" +
                 "00" + // r, s, v
                 encodedOwnerSigns;
-
             await safe.execTransaction(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, sigs);
 
             // Safe should be empty again
@@ -131,8 +129,8 @@ describe("Safe", () => {
             // Use off-chain Safe signature
             const transactionHash = await safe.getTransactionHash(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce);
             const messageHash = await messageHandler.getMessageHash(transactionHash);
-            const ownerSigs = await buildSignatureBytes([await signHash(user1, messageHash), await signHash(user2, messageHash)]);
-            const encodedOwnerSigns = defaultAbiCoder.encode(["bytes"], [ownerSigs]).slice(66);
+            const ownerSigs = buildSignatureBytes([await signHash(user1, messageHash), await signHash(user2, messageHash)]);
+            const encodedOwnerSigns = ((ownerSigs.length - 2) / 2).toString(16).padStart(64, "00") + ownerSigs.slice(2);
 
             // Use Safe owner
             const sigs =