[#754] WIP

contracts/Safe.sol

@@ -228,7 +228,12 @@ contract Safe is
      * @param signatures Signature data that should be verified.
      * @param offset Offset to the start of the contract signature in the signatures byte array
      */
-    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");
 
@@ -239,8 +244,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;
@@ -275,8 +281,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;
@@ -294,13 +301,13 @@ contract Safe is
                 // 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");
+                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
@@ -320,6 +327,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
+
+        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/integration/Safe.0xExploit.spec.ts

@@ -1,7 +1,6 @@
 import { expect } from "chai";
 import hre, { deployments, ethers } from "hardhat";
 import { AddressZero } from "@ethersproject/constants";
-import { defaultAbiCoder } from "@ethersproject/abi";
 import { getSafeWithOwners, deployContract, getCompatFallbackHandler } from "../utils/setup";
 import { buildSignatureBytes, executeContractCallWithSigners, signHash } from "../../src/utils/execution";
 
@@ -49,9 +48,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" +
@@ -76,7 +74,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
@@ -127,8 +124,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 =