diff --git a/contracts/Safe.sol b/contracts/Safe.sol index dca6bd605..f3984e621 100644 --- a/contracts/Safe.sol +++ b/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"); } /** diff --git a/docs/error_codes.md b/docs/error_codes.md index e84d69449..a88653cbf 100644 --- a/docs/error_codes.md +++ b/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` diff --git a/test/integration/Safe.0xExploit.spec.ts b/test/integration/Safe.0xExploit.spec.ts index fe4d6818c..62f9127ab 100644 --- a/test/integration/Safe.0xExploit.spec.ts +++ b/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 =