From 2fb54f1839b2c689f142e0011cc9ad9ec043aaf6 Mon Sep 17 00:00:00 2001 From: Akshay Date: Fri, 30 Jun 2023 15:29:35 +0200 Subject: [PATCH 1/8] Support updated EIP-1271 implementation for signature validation (#581) * WIP: EIP-1271 fix * Fix lint issue * Fix bytes32 test by signing hash instead of pre-image * EIP-1271: Fix failing test case for 0xExploit * EIP-1271: Comment data param in function call * EIP-1271: Fix lint issue * Remove use of outdated EIP-1271 method specification. * [#391] Add comment in natspec regarding unused data parameter --------- Co-authored-by: Mikhail --- contracts/Safe.sol | 9 +-- .../handler/CompatibilityFallbackHandler.sol | 36 ++++-------- contracts/interfaces/ISignatureValidator.sol | 12 ++-- docs/error_codes.md | 1 - test/core/Safe.Signatures.spec.ts | 41 ++++---------- .../CompatibilityFallbackHandler.spec.ts | 55 ++----------------- test/integration/Safe.0xExploit.spec.ts | 8 +-- 7 files changed, 42 insertions(+), 120 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index bef58b795..77235afa8 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -265,13 +265,15 @@ contract Safe is /** * @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 data parameter (bytes) is not used since v1.5.0 as it is not required anymore. Prior to v1.5.0, + * data parameter was used in contract signature validation flow using legacy EIP-1271. + * Version v1.5.0, uses dataHash parameter instead of data with updated EIP-1271 implementation. * @param dataHash Hash of the data (could be either a message hash or transaction hash) - * @param data That should be signed (this is passed to an external validator contract) * @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. * @param requiredSignatures Amount of required valid signatures. */ - function checkNSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures, uint256 requiredSignatures) public view { + function checkNSignatures(bytes32 dataHash, bytes memory /* data */, bytes memory signatures, uint256 requiredSignatures) public view { // Check that the provided signature data is not too short require(signatures.length >= requiredSignatures.mul(65), "GS020"); // There cannot be an owner with address 0. @@ -284,7 +286,6 @@ contract Safe is for (i = 0; i < requiredSignatures; i++) { (v, r, s) = signatureSplit(signatures, i); if (v == 0) { - require(keccak256(data) == dataHash, "GS027"); // If v is 0 then it is a contract signature // When handling contract signatures the address of the contract is encoded into r currentOwner = address(uint160(uint256(r))); @@ -312,7 +313,7 @@ contract Safe is // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s contractSignature := add(add(signatures, s), 0x20) } - require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024"); + require(ISignatureValidator(currentOwner).isValidSignature(dataHash, contractSignature) == EIP1271_MAGIC_VALUE, "GS024"); } 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 diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol index 4d4f75b2c..0bb0f6437 100644 --- a/contracts/handler/CompatibilityFallbackHandler.sol +++ b/contracts/handler/CompatibilityFallbackHandler.sol @@ -16,27 +16,6 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat bytes4 internal constant SIMULATE_SELECTOR = bytes4(keccak256("simulate(address,bytes)")); address internal constant SENTINEL_MODULES = address(0x1); - bytes4 internal constant UPDATED_MAGIC_VALUE = 0x1626ba7e; - - /** - * @notice Legacy EIP-1271 signature validation method. - * @dev Implementation of ISignatureValidator (see `interfaces/ISignatureValidator.sol`) - * @param _data Arbitrary length data signed on the behalf of address(msg.sender). - * @param _signature Signature byte array associated with _data. - * @return The EIP-1271 magic value. - */ - function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) { - // Caller should be a Safe - Safe safe = Safe(payable(msg.sender)); - bytes memory messageData = encodeMessageDataForSafe(safe, _data); - bytes32 messageHash = keccak256(messageData); - if (_signature.length == 0) { - require(safe.signedMessages(messageHash) != 0, "Hash not approved"); - } else { - safe.checkSignatures(messageHash, messageData, _signature); - } - return EIP1271_MAGIC_VALUE; - } /** * @dev Returns the hash of a message to be signed by owners. @@ -74,10 +53,17 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat * @param _signature Signature byte array associated with _dataHash * @return Updated EIP1271 magic value if signature is valid, otherwise 0x0 */ - function isValidSignature(bytes32 _dataHash, bytes calldata _signature) external view returns (bytes4) { - ISignatureValidator validator = ISignatureValidator(msg.sender); - bytes4 value = validator.isValidSignature(abi.encode(_dataHash), _signature); - return (value == EIP1271_MAGIC_VALUE) ? UPDATED_MAGIC_VALUE : bytes4(0); + function isValidSignature(bytes32 _dataHash, bytes calldata _signature) public view override returns (bytes4) { + // Caller should be a Safe + Safe safe = Safe(payable(msg.sender)); + bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash)); + bytes32 messageHash = keccak256(messageData); + if (_signature.length == 0) { + require(safe.signedMessages(messageHash) != 0, "Hash not approved"); + } else { + safe.checkSignatures(messageHash, messageData, _signature); + } + return EIP1271_MAGIC_VALUE; } /** diff --git a/contracts/interfaces/ISignatureValidator.sol b/contracts/interfaces/ISignatureValidator.sol index d2cd2e5fe..4c141bb40 100644 --- a/contracts/interfaces/ISignatureValidator.sol +++ b/contracts/interfaces/ISignatureValidator.sol @@ -2,19 +2,19 @@ pragma solidity >=0.7.0 <0.9.0; contract ISignatureValidatorConstants { - // bytes4(keccak256("isValidSignature(bytes,bytes)") - bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b; + // bytes4(keccak256("isValidSignature(bytes32,bytes)") + bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e; } abstract contract ISignatureValidator is ISignatureValidatorConstants { /** - * @notice Legacy EIP1271 method to validate a signature. - * @param _data Arbitrary length data signed on the behalf of address(this). + * @notice EIP1271 method to validate a signature. + * @param _hash Hash of the data signed on the behalf of address(this). * @param _signature Signature byte array associated with _data. * - * MUST return the bytes4 magic value 0x20c13b0b when function passes. + * MUST return the bytes4 magic value 0x1626ba7e when function passes. * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5) * MUST allow external calls */ - function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4); + function isValidSignature(bytes32 _hash, bytes memory _signature) external view virtual returns (bytes4); } diff --git a/docs/error_codes.md b/docs/error_codes.md index 65a1dba20..e30d2debd 100644 --- a/docs/error_codes.md +++ b/docs/error_codes.md @@ -19,7 +19,6 @@ - `GS024`: `Invalid contract signature provided` - `GS025`: `Hash has not been approved` - `GS026`: `Invalid owner provided` -- `GS027`: `Data Hash and hash of the pre-image data do not match` ### General auth related - `GS030`: `Only owners can approve a hash` diff --git a/test/core/Safe.Signatures.spec.ts b/test/core/Safe.Signatures.spec.ts index 770ec8df0..acee692f6 100644 --- a/test/core/Safe.Signatures.spec.ts +++ b/test/core/Safe.Signatures.spec.ts @@ -4,7 +4,6 @@ import { expect } from "chai"; import { deployments, waffle } from "hardhat"; import "@nomiclabs/hardhat-ethers"; import { AddressZero } from "@ethersproject/constants"; -import crypto from "crypto"; import { getSafeTemplate, getSafeWithOwners } from "../utils/setup"; import { safeSignTypedData, @@ -224,14 +223,15 @@ describe("Safe", async () => { const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); - // IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message - // we need to use the pre-image of the transaction hash to calculate the message hash - const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId()); + const safeMessageHash = calculateSafeMessageHash( + signerSafe, + calculateSafeTransactionHash(safe, tx, await chainId()), + await chainId(), + ); + const signerSafeOwnerSignature = await signHash(user5, safeMessageHash); const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data); - await expect( logGas( "Execute cancel transaction with 5 owners (1 owner is another Safe)", @@ -260,7 +260,7 @@ describe("Safe", async () => { "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v "0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read - await expect(safe.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS021"); + await expect(safe.checkSignatures(txHash, "0x", signatures)).to.be.revertedWith("GS021"); }); it("should fail if signatures data is not present", async () => { @@ -354,12 +354,9 @@ describe("Safe", async () => { const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); - // IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message - // we need to use the pre-image of the transaction hash to calculate the message hash - const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId()); + const safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId()); const signerSafeOwnerSignature = await signHash(user5, safeMessageHash); const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data); @@ -371,7 +368,7 @@ describe("Safe", async () => { signerSafeSig, ]); - await safe.checkSignatures(txHash, txHashData, signatures); + await safe.checkSignatures(txHash, "0x", signatures); }); }); @@ -473,12 +470,9 @@ describe("Safe", async () => { const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); - // IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message - // we need to use the pre-image of the transaction hash to calculate the message hash - const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId()); + const safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId()); const signerSafeOwnerSignature = await signHash(user5, safeMessageHash); const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data); @@ -490,7 +484,7 @@ describe("Safe", async () => { signerSafeSig, ]); - await safe.checkNSignatures(txHash, txHashData, signatures, 5); + await safe.checkNSignatures(txHash, "0x", signatures, 5); }); it("should be able to require no signatures", async () => { @@ -530,18 +524,5 @@ describe("Safe", async () => { await safe.checkNSignatures(txHash, txHashData, signatures, 3); }); - - it("should revert if the hash of the pre-image data and dataHash do not match for EIP-1271 signature", async () => { - await setupTests(); - const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address], 2); - const randomHash = `0x${crypto.pseudoRandomBytes(32).toString("hex")}`; - const randomBytes = `0x${crypto.pseudoRandomBytes(128).toString("hex")}`; - const randomAddress = `0x${crypto.pseudoRandomBytes(20).toString("hex")}`; - const randomSignature = `0x${crypto.pseudoRandomBytes(65).toString("hex")}`; - - const eip1271Sig = buildContractSignature(randomAddress, randomSignature); - const signatures = buildSignatureBytes([eip1271Sig]); - await expect(safe.checkNSignatures(randomHash, randomBytes, signatures, 1)).to.be.revertedWith("GS027"); - }); }); }); diff --git a/test/handlers/CompatibilityFallbackHandler.spec.ts b/test/handlers/CompatibilityFallbackHandler.spec.ts index d6e8776d2..051f7829d 100644 --- a/test/handlers/CompatibilityFallbackHandler.spec.ts +++ b/test/handlers/CompatibilityFallbackHandler.spec.ts @@ -1,4 +1,4 @@ -import { buildContractSignature } from "./../../src/utils/execution"; +import { buildContractSignature, calculateSafeMessageHash } from "./../../src/utils/execution"; import { expect } from "chai"; import hre, { deployments, waffle, ethers } from "hardhat"; import "@nomiclabs/hardhat-ethers"; @@ -8,7 +8,6 @@ import { buildSignatureBytes, executeContractCallWithSigners, calculateSafeMessageHash, - preimageSafeMessageHash, EIP712_SAFE_MESSAGE_TYPE, signHash, } from "../../src/utils/execution"; @@ -63,52 +62,6 @@ describe("CompatibilityFallbackHandler", async () => { }); }); - describe("isValidSignature(bytes,bytes)", async () => { - it("should revert if called directly", async () => { - const { handler } = await setupTests(); - await expect(handler.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0x")).to.be.revertedWith( - "function call to a non-contract account", - ); - }); - - it("should revert if message was not signed", async () => { - const { validator } = await setupTests(); - await expect(validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0x")).to.be.revertedWith("Hash not approved"); - }); - - it("should revert if signature is not valid", async () => { - const { validator } = await setupTests(); - await expect(validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0xdeaddeaddeaddead")).to.be.reverted; - }); - - it("should return magic value if message was signed", async () => { - const { safe, validator, signLib } = await setupTests(); - await executeContractCallWithSigners(safe, signLib, "signMessage", ["0xbaddad"], [user1, user2], true); - expect(await validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", "0x")).to.be.eq("0x20c13b0b"); - }); - - it("should return magic value if enough owners signed and allow a mix different signature types", async () => { - const { validator, signerSafe } = await setupTests(); - const sig1 = { - signer: user1.address, - data: await user1._signTypedData( - { verifyingContract: validator.address, chainId: await chainId() }, - EIP712_SAFE_MESSAGE_TYPE, - { message: "0xbaddad" }, - ), - }; - const sig2 = await signHash(user2, calculateSafeMessageHash(validator, "0xbaddad", await chainId())); - const validatorPreImageMessage = preimageSafeMessageHash(validator, "0xbaddad", await chainId()); - const signerSafeMessageHash = calculateSafeMessageHash(signerSafe, validatorPreImageMessage, await chainId()); - const signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash); - const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data); - - expect( - await validator.callStatic["isValidSignature(bytes,bytes)"]("0xbaddad", buildSignatureBytes([sig1, sig2, signerSafeSig])), - ).to.be.eq("0x20c13b0b"); - }); - }); - describe("isValidSignature(bytes32,bytes)", async () => { it("should revert if called directly", async () => { const { handler } = await setupTests(); @@ -149,9 +102,11 @@ describe("CompatibilityFallbackHandler", async () => { ), }; const ethSignSig = await signHash(user2, calculateSafeMessageHash(validator, dataHash, await chainId())); - const validatorPreImageMessage = preimageSafeMessageHash(validator, dataHash, await chainId()); - const signerSafeMessageHash = calculateSafeMessageHash(signerSafe, validatorPreImageMessage, await chainId()); + const validatorSafeMessageHash = calculateSafeMessageHash(validator, dataHash, await chainId()); + const signerSafeMessageHash = calculateSafeMessageHash(signerSafe, validatorSafeMessageHash, await chainId()); + const signerSafeOwnerSignature = await signHash(user1, signerSafeMessageHash); + const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data); expect( diff --git a/test/integration/Safe.0xExploit.spec.ts b/test/integration/Safe.0xExploit.spec.ts index bae8ccb68..ea26ab5ed 100644 --- a/test/integration/Safe.0xExploit.spec.ts +++ b/test/integration/Safe.0xExploit.spec.ts @@ -1,5 +1,5 @@ import { expect } from "chai"; -import hre, { deployments, waffle } from "hardhat"; +import hre, { deployments, ethers, waffle } from "hardhat"; import "@nomiclabs/hardhat-ethers"; import { AddressZero } from "@ethersproject/constants"; import { parseEther } from "@ethersproject/units"; @@ -41,7 +41,7 @@ describe("Safe", async () => { // Use off-chain Safe signature const messageData = await safe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce); - const messageHash = await messageHandler.getMessageHash(messageData); + const messageHash = await messageHandler.getMessageHash(ethers.utils.keccak256(messageData)); const ownerSigs = await buildSignatureBytes([await signHash(user1, messageHash), await signHash(user2, messageHash)]); const encodedOwnerSigns = defaultAbiCoder.encode(["bytes"], [ownerSigs]).slice(66); @@ -83,11 +83,11 @@ describe("Safe", async () => { contract Test { bool public changeState; uint256 public nonce; - function isValidSignature(bytes memory _data, bytes memory _signature) public returns (bytes4) { + function isValidSignature(bytes32 _data, bytes memory _signature) public returns (bytes4) { if (changeState) { nonce = nonce + 1; } - return 0x20c13b0b; + return 0x1626ba7e; } function shouldChangeState(bool value) public { From 56f49e61505dba508eb39c865a80e6477f6eeab0 Mon Sep 17 00:00:00 2001 From: Mikhail Date: Fri, 30 Jun 2023 15:30:22 +0200 Subject: [PATCH 2/8] Feature: Mark assembly blocks as memory-safe (#545) * add setup and rules about modules * fix run script * use solc7.6 for fv * Mark assembly blocks as memory-safe * use solidity 0.8.19 for github action benchmark * Update makefile * fix the harness patch * properties doc skeleton * properties notes * use 10m optimizer runs * Write calldata/return data to the memory allocated via the free memory pointer * memory-safe simulateAndRevert * Update CLA github action to v2.3.0 * add certora workflow * Fix changelog mention of createChainSpecificProxyWithNonce * fix script path * use cvl2 * Remove gasleft in setupModules, add erc4337 compatibility test * Fix typechecking in test files (#573) * verify that guard can only be updated through setGuard * Verify functions that may change the fallback handler address (#566) * Add an invariant for singleton address (#565) * Add an optimistic assumption about DELEGATECALL, update nonce monotonicity rule (#574) * Pump version to 1.4.1 (#579) * Formal verification: native token balance updates (#582) * Add an optimistic assumption about DELEGATECALL, update nonce monotonicity rule * Add a rule for token balance * Add more rules for native token balance transition * Fix addresses for 1.4.1 in changelog (#590) * Fix certora CI action * Formal verification: No message can be signed through the core contract (#583) * Add a rule for no signed messages * Add a rule for no signed messages * fix munged patch --------- Co-authored-by: teryanarmen <61996358+teryanarmen@users.noreply.github.com> Co-authored-by: Uxio Fuentefria Co-authored-by: Francisco Giordano Co-authored-by: Richard Meissner Co-authored-by: Mikhail Mikheev --- .env.sample | 5 + .github/workflows/certora.yml | 53 ++++ .github/workflows/ci.yml | 6 +- .github/workflows/cla.yml | 4 +- .gitignore | 7 +- CHANGELOG.md | 48 ++- README.md | 20 +- certora/.gitignore | 1 + certora/Makefile | 25 ++ certora/applyHarness.patch | 32 ++ certora/harnesses/SafeHarness.sol | 30 ++ certora/scripts/verifySafe.sh | 19 ++ certora/specs/Safe.spec | 240 ++++++++++++++ certora/specs/properties.md | 63 ++++ contracts/Safe.sol | 6 +- contracts/accessors/SimulateTxAccessor.sol | 1 + contracts/base/Executor.sol | 2 + contracts/base/FallbackManager.sol | 30 +- contracts/base/GuardManager.sol | 2 + contracts/base/ModuleManager.sol | 5 +- contracts/common/SecuredTokenTransfer.sol | 1 + contracts/common/SignatureDecoder.sol | 1 + contracts/common/StorageAccessible.sol | 13 +- .../guards/ReentrancyTransactionGuard.sol | 1 + .../handler/CompatibilityFallbackHandler.sol | 1 + contracts/handler/HandlerContext.sol | 1 + contracts/libraries/CreateCall.sol | 2 + contracts/libraries/MultiSend.sol | 1 + contracts/libraries/MultiSendCallOnly.sol | 1 + contracts/proxies/SafeProxyFactory.sol | 4 + .../test/4337/Test4337ModuleAndHandler.sol | 65 ++++ contracts/test/ERC1155Token.sol | 1 + docs/audit_1_4_0.md | 7 + hardhat.config.ts | 5 +- package.json | 8 +- src/utils/execution.ts | 4 +- test/integration/Safe.ERC4337.spec.ts | 133 ++++++++ test/migration/UpgradeFromSafe111.spec.ts | 2 +- test/migration/UpgradeFromSafe120.spec.ts | 2 +- test/migration/subTests.spec.ts | 2 +- test/utils/setup.ts | 12 + tsconfig.json | 37 +-- tsconfig.prod.json | 4 + yarn.lock | 294 ++++++++++-------- 44 files changed, 1020 insertions(+), 181 deletions(-) create mode 100644 .github/workflows/certora.yml create mode 100644 certora/.gitignore create mode 100644 certora/Makefile create mode 100644 certora/applyHarness.patch create mode 100644 certora/harnesses/SafeHarness.sol create mode 100755 certora/scripts/verifySafe.sh create mode 100644 certora/specs/Safe.spec create mode 100644 certora/specs/properties.md create mode 100644 contracts/test/4337/Test4337ModuleAndHandler.sol create mode 100644 test/integration/Safe.ERC4337.spec.ts create mode 100644 tsconfig.prod.json diff --git a/.env.sample b/.env.sample index f2d066cb3..0f2a3ff2e 100644 --- a/.env.sample +++ b/.env.sample @@ -4,3 +4,8 @@ INFURA_KEY="" # Used for custom network NODE_URL="" ETHERSCAN_API_KEY="" +# (Optional) Used to run ERC-4337 compatibility test. MNEMONIC is also required. +ERC4337_TEST_BUNDLER_URL= +ERC4337_TEST_NODE_URL= +ERC4337_TEST_SINGLETON_ADDRESS= +ERC4337_TEST_SAFE_FACTORY_ADDRESS= \ No newline at end of file diff --git a/.github/workflows/certora.yml b/.github/workflows/certora.yml new file mode 100644 index 000000000..db054b7b0 --- /dev/null +++ b/.github/workflows/certora.yml @@ -0,0 +1,53 @@ +name: certora + +on: + push: + branches: + - main + pull_request: + branches: + - main + + workflow_dispatch: + +jobs: + verify: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + + - name: Install python + uses: actions/setup-python@v4 + with: { python-version: 3.11 } + + - name: Install java + uses: actions/setup-java@v3 + with: { java-version: "17", java-package: jre, distribution: semeru } + + - name: Install certora cli + run: pip install -Iv certora-cli + + - name: Install solc + run: | + wget https://github.com/ethereum/solidity/releases/download/v0.7.6/solc-static-linux + chmod +x solc-static-linux + sudo mv solc-static-linux /usr/local/bin/solc7.6 + + - name: Verify rule ${{ matrix.rule }} + run: | + cd certora + touch applyHarness.patch + make munged + cd .. + echo "key length" ${#CERTORAKEY} + ./certora/scripts/${{ matrix.rule }} + env: + CERTORAKEY: ${{ secrets.CERTORA_KEY }} + + strategy: + fail-fast: false + max-parallel: 16 + matrix: + rule: + - verifySafe.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index da5282541..c1259c611 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,10 +62,10 @@ jobs: strategy: fail-fast: false matrix: - solidity: ["0.7.6", "0.8.2"] + solidity: ["0.7.6", "0.8.19"] include: - - solidity: "0.8.2" - settings: '{"viaIR":true,"optimizer":{"enabled":true,"runs":10000}}' + - solidity: "0.8.19" + settings: '{"viaIR":true,"optimizer":{"enabled":true,"runs":1000000}}' env: SOLIDITY_VERSION: ${{ matrix.solidity }} SOLIDITY_SETTINGS: ${{ matrix.settings }} diff --git a/.github/workflows/cla.yml b/.github/workflows/cla.yml index dd720c857..487aa871e 100644 --- a/.github/workflows/cla.yml +++ b/.github/workflows/cla.yml @@ -12,7 +12,7 @@ jobs: - name: "CLA Assistant" if: (github.event.comment.body == 'recheck' || github.event.comment.body == 'I have read the CLA Document and I hereby sign the CLA') || github.event_name == 'pull_request_target' # Beta Release - uses: cla-assistant/github-action@v2.1.3-beta + uses: cla-assistant/github-action@v2.3.0 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # the below token should have repo scope and must be manually added by you in the repository's secret @@ -33,4 +33,4 @@ jobs: #custom-pr-sign-comment: 'The signature to be committed in order to sign the CLA' #custom-allsigned-prcomment: 'pull request comment when all contributors has signed, defaults to **CLA Assistant Lite bot** All Contributors have signed the CLA.' #lock-pullrequest-aftermerge: false - if you don't want this bot to automatically lock the pull request after merging (default - true) - #use-dco-flag: true - If you are using DCO instead of CLA \ No newline at end of file + #use-dco-flag: true - If you are using DCO instead of CLA diff --git a/.gitignore b/.gitignore index 18cdf9169..3d7b70cca 100644 --- a/.gitignore +++ b/.gitignore @@ -11,4 +11,9 @@ bin/ solc coverage/ coverage.json -yarn-error.log \ No newline at end of file +yarn-error.log + +# Certora Formal Verification related files +.certora_internal +.certora_recent_jobs.json +.zip-output-url.txt \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 267beb757..3b97e972e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,52 @@ This changelog only contains changes starting from version 1.3.0 +# Version 1.4.1 + +## Compiler settings + +Solidity compiler: [0.7.6](https://github.com/ethereum/solidity/releases/tag/v0.7.6) (for more info see issue [#251](https://github.com/safe-global/safe-contracts/issues/251)) + +Solidity optimizer: `disabled` + +## Expected addresses with [Safe Singleton Factory](https://github.com/safe-global/safe-singleton-factory) + +### Core contracts + +- `Safe` at `0x41675C099F32341bf84BFc5382aF534df5C7461a` +- `SafeL2` at `0x29fcB43b46531BcA003ddC8FCB67FFE91900C762` + +### Factory contracts + +- `SafeProxyFactory` at `0x4e1DCf7AD4e460CfD30791CCC4F9c8a4f820ec67` + +### Handler contracts + +- `TokenCallbackHandler` at `0xeDCF620325E82e3B9836eaaeFdc4283E99Dd7562` +- `CompatibilityFallbackHandler` at `0xfd0732Dc9E303f09fCEf3a7388Ad10A83459Ec99` + +### Lib contracts + +- `MultiSend` at `0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526` +- `MultiSendCallOnly` at `0x9641d764fc13c8B624c04430C7356C1C7C8102e2` +- `CreateCall` at `0x9b35Af71d77eaf8d7e40252370304687390A1A52` +- `SignMessageLib` at `0xd53cd0aB83D845Ac265BE939c57F53AD838012c9` + +### Storage reader contracts + +- `SimulateTxAccessor` at `0x3d4BA2E0884aa488718476ca2FB8Efc291A46199` + +## Changes + +### Bugfixes + +#### Remove `gasleft()` usage in `setupModules` + +Issue: [#568](https://github.com/safe-global/safe-contracts/issues/568) + +`setupModules` made a call to `gasleft()` that is invalid in the ERC-4337 standard. The call was replaced with `type(uint256).max` to forward all the available gas instead. + + # Version 1.4.0 ## Compiler settings @@ -107,7 +153,7 @@ This method uses the `CREATE` opcode, which is not counterfactual for a specific If the initializer data is provided, the Factory now checks that the Singleton contract exists and the success of the call to avoid a proxy being deployed uninitialized -#### Add `createNetworkSpecificProxy` +#### Add `createChainSpecificProxyWithNonce` This method will use the chain id in the `CREATE2` salt; therefore, deploying a proxy to the same address on other networks is impossible. This method should enable the creation of proxies that should exist only on one network (e.g. specific governance or admin accounts) diff --git a/README.md b/README.md index 199e5ce84..357dd900d 100644 --- a/README.md +++ b/README.md @@ -15,13 +15,29 @@ Usage yarn ``` -### Run all tests: +### Testing + +To run the tests: ```bash yarn build yarn test ``` +Optionally, if you want to run the ERC-4337 compatibility test, it uses a live bundler and node, so it contains some pre-requisites: + +1. Define the environment variables: + +``` +ERC4337_TEST_BUNDLER_URL= +ERC4337_TEST_NODE_URL= +ERC4337_TEST_SINGLETON_ADDRESS= +ERC4337_TEST_SAFE_FACTORY_ADDRESS= +MNEMONIC= +``` + +2. Pre-fund the executor account derived from the mnemonic with some Native Token to cover the deployment of an ERC4337 module and the pre-fund of the Safe for the test operation. + ### Deployments A collection of the different Safe contract deployments and their addresses can be found in the [Safe deployments](https://github.com/safe-global/safe-deployments) repository. @@ -92,7 +108,7 @@ Documentation Audits/ Formal Verification --------- -- [for Version 1.4.0 by Ackee Blockchain](docs/audit_1_4_0.md) +- [for Version 1.4.0/1.4.1 by Ackee Blockchain](docs/audit_1_4_0.md) - [for Version 1.3.0 by G0 Group](docs/audit_1_3_0.md) - [for Version 1.2.0 by G0 Group](docs/audit_1_2_0.md) - [for Version 1.1.1 by G0 Group](docs/audit_1_1_1.md) diff --git a/certora/.gitignore b/certora/.gitignore new file mode 100644 index 000000000..284379223 --- /dev/null +++ b/certora/.gitignore @@ -0,0 +1 @@ +munged \ No newline at end of file diff --git a/certora/Makefile b/certora/Makefile new file mode 100644 index 000000000..2afda13d2 --- /dev/null +++ b/certora/Makefile @@ -0,0 +1,25 @@ +default: help + +PATCH = applyHarness.patch +CONTRACTS_DIR = ../contracts +MUNGED_DIR = munged + +help: + @echo "usage:" + @echo " make clean: remove all generated files (those ignored by git)" + @echo " make munged: create $(MUNGED_DIR) directory by applying the patch file to $(CONTRACTS_DIR)" + @echo " make record: record a new patch file capturing the differences between $(CONTRACTS_DIR) and $(MUNGED_DIR)" + +munged: $(wildcard $(CONTRACTS_DIR)/*.sol) $(PATCH) + rm -rf $@ + cp -r $(CONTRACTS_DIR) $@ + patch -p0 -d $@ < $(PATCH) + +record: + diff -druN $(CONTRACTS_DIR) $(MUNGED_DIR) | sed 's+../contracts/++g' | sed 's+munged/++g' > $(PATCH) + +refresh: munged record + +clean: + git clean -fdX + touch $(PATCH) diff --git a/certora/applyHarness.patch b/certora/applyHarness.patch new file mode 100644 index 000000000..3aae94039 --- /dev/null +++ b/certora/applyHarness.patch @@ -0,0 +1,32 @@ +diff -druN base/Executor.sol base/Executor.sol +--- base/Executor.sol 2023-06-22 14:42:39.769357540 +0200 ++++ base/Executor.sol 2023-06-22 15:34:09.211725615 +0200 +@@ -26,11 +26,13 @@ + uint256 txGas + ) internal returns (bool success) { + if (operation == Enum.Operation.DelegateCall) { ++ // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true + // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly +- assembly { +- success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) +- } ++ // assembly { ++ // success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) ++ // } ++ return true; + } else { + // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly +diff -druN Safe.sol Safe.sol +--- Safe.sol 2023-06-22 14:43:53.738861263 +0200 ++++ Safe.sol 2023-06-22 15:28:00.436277677 +0200 +@@ -76,7 +76,7 @@ + * so we create a Safe with 0 owners and threshold 1. + * This is an unusable Safe, perfect for the singleton + */ +- threshold = 1; ++ // threshold = 1; MUNGED: remove and add to constructor of the harness + } + + /** diff --git a/certora/harnesses/SafeHarness.sol b/certora/harnesses/SafeHarness.sol new file mode 100644 index 000000000..822415d4f --- /dev/null +++ b/certora/harnesses/SafeHarness.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: LGPL-3.0-only +import "../munged/Safe.sol"; + +contract SafeHarness is Safe { + constructor( + address[] memory _owners, + uint256 _threshold, + address to, + bytes memory data, + address fallbackHandler, + address paymentToken, + uint256 payment, + address payable paymentReceiver + ) { + this.setup(_owners, _threshold, to, data, fallbackHandler, paymentToken, payment, paymentReceiver); + } + + // harnessed getters + function getModule(address module) public view returns (address) { + return modules[module]; + } + + function getSafeGuard() public view returns (address) { + return getGuard(); + } + + function getNativeTokenBalance() public view returns (uint256) { + return address(this).balance; + } +} diff --git a/certora/scripts/verifySafe.sh b/certora/scripts/verifySafe.sh new file mode 100755 index 000000000..fd14d87fa --- /dev/null +++ b/certora/scripts/verifySafe.sh @@ -0,0 +1,19 @@ +#!/bin/bash + +params=("--send_only") + +if [[ -n "$CI" ]]; then + params=() +fi + +certoraRun certora/harnesses/SafeHarness.sol \ + --verify SafeHarness:certora/specs/Safe.spec \ + --solc solc7.6 \ + --optimistic_loop \ + --settings -optimisticFallback=true \ + --loop_iter 3 \ + --optimistic_hashing \ + --hashing_length_bound 352 \ + --rule_sanity \ + "${params[@]}" \ + --msg "Safe $1 " \ No newline at end of file diff --git a/certora/specs/Safe.spec b/certora/specs/Safe.spec new file mode 100644 index 000000000..f09780902 --- /dev/null +++ b/certora/specs/Safe.spec @@ -0,0 +1,240 @@ +methods { + // + function getThreshold() external returns (uint256) envfree; + function disableModule(address,address) external; + function nonce() external returns (uint256) envfree; + function signedMessages(bytes32) external returns (uint256) envfree; + + // harnessed + function getModule(address) external returns (address) envfree; + function getSafeGuard() external returns (address) envfree; + function getNativeTokenBalance() external returns (uint256) envfree; + + // optional + function execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation) external returns (bool, bytes memory); + function execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation) external returns (bool); + function execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool); +} + +definition noHavoc(method f) returns bool = + f.selector != sig:execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation).selector + && f.selector != sig:execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation).selector + && f.selector != sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector; + +definition reachableOnly(method f) returns bool = + f.selector != sig:setup(address[],uint256,address,bytes,address,address,uint256,address).selector + && f.selector != sig:simulateAndRevert(address,bytes).selector + // getStorageAt cannot be used because we have a hook to sstore + // A quote from the Certora team: + // "If it’s called from an internal context it is fine but as a public function that can be called with any argument it cannot have hooks applied on." + && f.selector != sig:getStorageAt(uint256,uint256).selector; + +definition MAX_UINT256() returns uint256 = 0xffffffffffffffffffffffffffffffff; + +/// Nonce must never decrease +rule nonceMonotonicity(method f) filtered { + f -> reachableOnly(f) +} { + uint256 nonceBefore = nonce(); + + // The nonce may overflow, but since it's increased only by 1 with each transaction, it is not realistically possible to overflow it. + require nonceBefore < MAX_UINT256(); + + calldataarg args; env e; + f(e, args); + + uint256 nonceAfter = nonce(); + + assert nonceAfter != nonceBefore => + to_mathint(nonceAfter) == nonceBefore + 1 && f.selector == sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector; +} + + +/// The sentinel must never point to the zero address. +/// @notice It should either point to itself or some nonzero value +invariant liveSentinel() + getModule(1) != 0 + filtered { f -> noHavoc(f) && reachableOnly(f) } + { preserved { + requireInvariant noDeadEnds(getModule(1), 1); + }} + +/// Threshold must always be nonzero. +invariant nonzeroThreshold() + getThreshold() > 0 + filtered { f -> noHavoc(f) && reachableOnly(f) } + +/// Two different modules must not point to the same module/ +invariant uniquePrevs(address prev1, address prev2) + prev1 != prev2 && getModule(prev1) != 0 => getModule(prev1) != getModule(prev2) + filtered { f -> noHavoc(f) && reachableOnly(f) } + { + preserved { + requireInvariant noDeadEnds(getModule(prev1), prev1); + requireInvariant noDeadEnds(getModule(prev2), prev2); + requireInvariant uniquePrevs(prev1, 1); + requireInvariant uniquePrevs(prev2, 1); + requireInvariant uniquePrevs(prev1, getModule(prev2)); + requireInvariant uniquePrevs(prev2, getModule(prev1)); + } + } + +/// A module that points to the zero address must not have another module pointing to it. +invariant noDeadEnds(address dead, address lost) + dead != 0 && getModule(dead) == 0 => getModule(lost) != dead + filtered { f -> noHavoc(f) && reachableOnly(f) } + { + preserved { + requireInvariant liveSentinel(); + requireInvariant noDeadEnds(getModule(1), 1); + } + preserved disableModule(address prevModule, address module) with (env e) { + requireInvariant uniquePrevs(prevModule, lost); + requireInvariant uniquePrevs(prevModule, dead); + requireInvariant noDeadEnds(dead, module); + requireInvariant noDeadEnds(module, dead); + } + } + + +// The singleton is a private variable, so we need to use a ghost variable to track it. +ghost address ghostSingletonAddress { + init_state axiom ghostSingletonAddress == 0; +} + +hook Sstore SafeHarness.(slot 0) address newSingletonAddress STORAGE { + ghostSingletonAddress = newSingletonAddress; +} + +// This is EIP-1967's singleton storage slot: +// 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc +// converted to decimal because certora doesn't seem to support hex yet. +hook Sstore SafeHarness.(slot 24440054405305269366569402256811496959409073762505157381672968839269610695612) address newSingletonAddress STORAGE { + ghostSingletonAddress = newSingletonAddress; +} + +invariant sigletonAddressNeverChanges() + ghostSingletonAddress == 0 + filtered { f -> reachableOnly(f) && f.selector != sig:getStorageAt(uint256,uint256).selector } + +ghost address fallbackHandlerAddress { + init_state axiom fallbackHandlerAddress == 0; +} + +// This is Safe's fallback handler storage slot: +// 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5 +// converted to decimal because certora doesn't seem to support hex yet. +hook Sstore SafeHarness.(slot 49122629484629529244014240937346711770925847994644146912111677022347558721749) address newFallbackHandlerAddress STORAGE { + fallbackHandlerAddress = newFallbackHandlerAddress; +} + +rule fallbackHandlerAddressChange(method f) filtered { + f -> f.selector != sig:simulateAndRevert(address,bytes).selector && + f.selector != sig:getStorageAt(uint256,uint256).selector +} { + address fbHandlerBefore = fallbackHandlerAddress; + + calldataarg args; env e; + f(e, args); + + address fbHandlerAfter = fallbackHandlerAddress; + + assert fbHandlerBefore != fbHandlerAfter => + f.selector == sig:setup(address[],uint256,address,bytes,address,address,uint256,address).selector || f.selector == sig:setFallbackHandler(address).selector; +} + + +rule guardAddressChange(method f) filtered { + f -> f.selector != sig:simulateAndRevert(address,bytes).selector && + f.selector != sig:getStorageAt(uint256,uint256).selector +} { + address guardBefore = getSafeGuard(); + + calldataarg args; env e; + f(e, args); + + address guardAfter = getSafeGuard(); + + assert guardBefore != guardAfter => + f.selector == sig:setGuard(address).selector; +} + +invariant noSignedMessages(bytes32 message) + signedMessages(message) == 0 + filtered { f -> reachableOnly(f) } + +rule nativeTokenBalanceSpending(method f) filtered { + f -> reachableOnly(f) +} { + uint256 balanceBefore = getNativeTokenBalance(); + + calldataarg args; env e; + f(e, args); + + uint256 balanceAfter = getNativeTokenBalance(); + + assert balanceAfter < balanceBefore => + f.selector == sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector + || f.selector == sig:execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation).selector + || f.selector == sig:execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation).selector; +} + +rule nativeTokenBalanceSpendingExecTransaction( + address to, + uint256 value, + bytes data, + SafeHarness.Operation operation, + uint256 safeTxGas, + uint256 baseGas, + uint256 gasPrice, + address gasToken, + address refundReceiver, + bytes signatures + ) { + uint256 balanceBefore = getNativeTokenBalance(); + + env e; + execTransaction(e, to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures); + + uint256 balanceAfter = getNativeTokenBalance(); + + assert + gasPrice == 0 => to_mathint(balanceBefore - value) <= to_mathint(balanceAfter) + // When the gas price is non-zero and the gas token is zero (zero = native token), the refund params should also be taken into account. + || gasPrice > 0 && gasToken == 0 => to_mathint(balanceBefore - value - (gasPrice * (baseGas + safeTxGas))) <= to_mathint(balanceAfter); +} + +rule nativeTokenBalanceSpendingExecTransactionFromModule( + address to, + uint256 value, + bytes data, + SafeHarness.Operation operation + ) { + uint256 balanceBefore = getNativeTokenBalance(); + env e; + + execTransactionFromModule(e, to, value, data, operation); + + uint256 balanceAfter = getNativeTokenBalance(); + + assert balanceAfter < balanceBefore => + to_mathint(balanceBefore - value) <= to_mathint(balanceAfter); +} + + +rule nativeTokenBalanceSpendingExecTransactionFromModuleReturnData( + address to, + uint256 value, + bytes data, + SafeHarness.Operation operation +) { + uint256 balanceBefore = getNativeTokenBalance(); + env e; + + execTransactionFromModuleReturnData(e, to, value, data, operation); + + uint256 balanceAfter = getNativeTokenBalance(); + + assert balanceAfter < balanceBefore => + to_mathint(balanceBefore - value) <= to_mathint(balanceAfter); +} diff --git a/certora/specs/properties.md b/certora/specs/properties.md new file mode 100644 index 000000000..761112ec0 --- /dev/null +++ b/certora/specs/properties.md @@ -0,0 +1,63 @@ +# Safe Contract implementation properties + +## Reminder on property categories + +The categories are based on Certora's workshop [notes](https://github.com/Certora/Tutorials/blob/40ad7970bfafd081f6f416fe36b31981e48c6857/3DayWorkshop/SymbolicPool/properties.md). + +1. Valid states + Usually, there can be only one valid state at any given time. Such properties ensure the system is always in exactly one of its valid states. + +2. State transitions + Such properties verify the correctness of transactions between valid states. E.g., confirm valid states change according to their correct order or transitions only occur under the right conditions. + +3. Variable transitions + Similar to state transitions, but for variables. E.g., verify that Safe nonce is monotonically increasing. + +4. High-level properties + The most powerful type of properties covering the entire system. E.g., for any given operation, Safe threshold must remain lower or equal to the number of owners. + +5. Unit test + Such properties target specific function individually to verify their correctness. E.g., verify that a specific function can only be called by a specific address. + +6. Risk assessment + Such properties verify that worst cases that can happen to the system are handled correctly. E.g., verify that a transaction cannot be replayed. + +## Safe Contract Properties + +Verification doesn't hold for the `DELEGATECALL` operation. + +### Valid states + +### State transitions + +### Variable transitions + +### High-level properties + +### Unit test + +### Risk assessment + +- nonce monotonicity, it can only increase by 1 after execTransaction call + +- consistency of owner and module lists + +verify that `ownerCount` is in sync with the linked list. +always circular - each address for which `isModuleEnabled` returns true should be a part of the list + +- configuration changes to safe can only be done by the safe + who can swap owner? + module management + who should be able to? + +who should be allowed to make contract do delegate calls? +contract creator +address specified by contract creator + +- setup only be done once + +module states +enabled +cancelled + +execTransactionFromModuleReturnData diff --git a/contracts/Safe.sol b/contracts/Safe.sol index 77235afa8..bff476b15 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -44,7 +44,7 @@ contract Safe is { using SafeMath for uint256; - string public constant VERSION = "1.4.0"; + string public constant VERSION = "1.4.1"; // keccak256( // "EIP712Domain(uint256 chainId,address verifyingContract)" @@ -192,6 +192,7 @@ contract Safe is ); } } + // We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500) // We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150 require(gasleft() >= ((safeTxGas * 64) / 63).max(safeTxGas + 2500) + 500, "GS010"); @@ -301,6 +302,7 @@ contract Safe is // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length uint256 contractSignatureLen; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { contractSignatureLen := mload(add(add(signatures, s), 0x20)) } @@ -309,6 +311,7 @@ contract Safe is // Check signature bytes memory contractSignature; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s contractSignature := add(add(signatures, s), 0x20) @@ -353,6 +356,7 @@ contract Safe is function getChainId() public view returns (uint256) { uint256 id; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { id := chainid() } diff --git a/contracts/accessors/SimulateTxAccessor.sol b/contracts/accessors/SimulateTxAccessor.sol index f1d2b61d6..a5525bf73 100644 --- a/contracts/accessors/SimulateTxAccessor.sol +++ b/contracts/accessors/SimulateTxAccessor.sol @@ -49,6 +49,7 @@ contract SimulateTxAccessor is Executor { success = execute(to, value, data, operation, gasleft()); estimate = startGas - gasleft(); // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { // Load free memory location let ptr := mload(0x40) diff --git a/contracts/base/Executor.sol b/contracts/base/Executor.sol index 121834566..094b1c39c 100644 --- a/contracts/base/Executor.sol +++ b/contracts/base/Executor.sol @@ -27,11 +27,13 @@ abstract contract Executor { ) internal returns (bool success) { if (operation == Enum.Operation.DelegateCall) { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) } } else { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { success := call(txGas, to, value, add(data, 0x20), mload(data), 0, 0) } diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol index baf307a6c..586e4b620 100644 --- a/contracts/base/FallbackManager.sol +++ b/contracts/base/FallbackManager.sol @@ -35,6 +35,7 @@ abstract contract FallbackManager is SelfAuthorized { bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { sstore(slot, handler) } @@ -61,22 +62,39 @@ abstract contract FallbackManager is SelfAuthorized { fallback() external { bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { + // When compiled with the optimizer, the compiler relies on a certain assumptions on how the + // memory is used, therefore we need to guarantee memory safety (keeping the free memory point 0x40 slot intact, + // not going beyond the scratch space, etc) + // Solidity docs: https://docs.soliditylang.org/en/latest/assembly.html#memory-safety + function allocate(length) -> pos { + pos := mload(0x40) + mstore(0x40, add(pos, length)) + } + let handler := sload(slot) if iszero(handler) { return(0, 0) } - calldatacopy(0, 0, calldatasize()) + + let calldataPtr := allocate(calldatasize()) + calldatacopy(calldataPtr, 0, calldatasize()) + // The msg.sender address is shifted to the left by 12 bytes to remove the padding // Then the address without padding is stored right after the calldata - mstore(calldatasize(), shl(96, caller())) + let senderPtr := allocate(20) + mstore(senderPtr, shl(96, caller())) + // Add 20 bytes for the address appended add the end - let success := call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0) - returndatacopy(0, 0, returndatasize()) + let success := call(gas(), handler, 0, calldataPtr, add(calldatasize(), 20), 0, 0) + + let returnDataPtr := allocate(returndatasize()) + returndatacopy(returnDataPtr, 0, returndatasize()) if iszero(success) { - revert(0, returndatasize()) + revert(returnDataPtr, returndatasize()) } - return(0, returndatasize()) + return(returnDataPtr, returndatasize()) } } } diff --git a/contracts/base/GuardManager.sol b/contracts/base/GuardManager.sol index 4bd3cc0d3..0a4f4169f 100644 --- a/contracts/base/GuardManager.sol +++ b/contracts/base/GuardManager.sol @@ -56,6 +56,7 @@ abstract contract GuardManager is SelfAuthorized { } bytes32 slot = GUARD_STORAGE_SLOT; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { sstore(slot, guard) } @@ -72,6 +73,7 @@ abstract contract GuardManager is SelfAuthorized { function getGuard() internal view returns (address guard) { bytes32 slot = GUARD_STORAGE_SLOT; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { guard := sload(slot) } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index f260c0865..ec5d6f5ae 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -35,7 +35,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor { if (to != address(0)) { require(isContract(to), "GS002"); // Setup has to complete successfully or transaction fails. - require(execute(to, 0, data, Enum.Operation.DelegateCall, gasleft()), "GS000"); + require(execute(to, 0, data, Enum.Operation.DelegateCall, type(uint256).max), "GS000"); } } @@ -109,6 +109,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor { ) public returns (bool success, bytes memory returnData) { success = execTransactionFromModule(to, value, data, operation); // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { // Load free memory location let ptr := mload(0x40) @@ -169,6 +170,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor { } // Set correct size of returned array // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { mstore(array, moduleCount) } @@ -183,6 +185,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor { function isContract(address account) internal view returns (bool) { uint256 size; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { size := extcodesize(account) } diff --git a/contracts/common/SecuredTokenTransfer.sol b/contracts/common/SecuredTokenTransfer.sol index bddffd831..61344d30b 100644 --- a/contracts/common/SecuredTokenTransfer.sol +++ b/contracts/common/SecuredTokenTransfer.sol @@ -19,6 +19,7 @@ abstract contract SecuredTokenTransfer { // 0xa9059cbb - keccack("transfer(address,uint256)") bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount); // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { // We write the return value to scratch space. // See https://docs.soliditylang.org/en/v0.7.6/internals/layout_in_memory.html#layout-in-memory diff --git a/contracts/common/SignatureDecoder.sol b/contracts/common/SignatureDecoder.sol index b505cbf67..22e6c6214 100644 --- a/contracts/common/SignatureDecoder.sol +++ b/contracts/common/SignatureDecoder.sol @@ -20,6 +20,7 @@ abstract contract SignatureDecoder { */ function signatureSplit(bytes memory signatures, uint256 pos) internal pure returns (uint8 v, bytes32 r, bytes32 s) { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { let signaturePos := mul(0x41, pos) r := mload(add(signatures, add(signaturePos, 0x20))) diff --git a/contracts/common/StorageAccessible.sol b/contracts/common/StorageAccessible.sol index 0b72714ba..396a69f10 100644 --- a/contracts/common/StorageAccessible.sol +++ b/contracts/common/StorageAccessible.sol @@ -18,6 +18,7 @@ abstract contract StorageAccessible { bytes memory result = new bytes(length * 32); for (uint256 index = 0; index < length; index++) { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { let word := sload(add(offset, index)) mstore(add(add(result, 0x20), mul(index, 0x20)), word) @@ -39,13 +40,15 @@ abstract contract StorageAccessible { */ function simulateAndRevert(address targetContract, bytes memory calldataPayload) external { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { let success := delegatecall(gas(), targetContract, add(calldataPayload, 0x20), mload(calldataPayload), 0, 0) - - mstore(0x00, success) - mstore(0x20, returndatasize()) - returndatacopy(0x40, 0, returndatasize()) - revert(0, add(returndatasize(), 0x40)) + // Load free memory location + let ptr := mload(0x40) + mstore(ptr, success) + mstore(add(ptr, 0x20), returndatasize()) + returndatacopy(add(ptr, 0x40), 0, returndatasize()) + revert(ptr, add(returndatasize(), 0x40)) } } } diff --git a/contracts/examples/guards/ReentrancyTransactionGuard.sol b/contracts/examples/guards/ReentrancyTransactionGuard.sol index f1a136fe0..587f44671 100644 --- a/contracts/examples/guards/ReentrancyTransactionGuard.sol +++ b/contracts/examples/guards/ReentrancyTransactionGuard.sol @@ -30,6 +30,7 @@ contract ReentrancyTransactionGuard is BaseGuard { function getGuard() internal pure returns (GuardValue storage guard) { bytes32 slot = GUARD_STORAGE_SLOT; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { guard.slot := slot } diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol index 0bb0f6437..fdc515887 100644 --- a/contracts/handler/CompatibilityFallbackHandler.sol +++ b/contracts/handler/CompatibilityFallbackHandler.sol @@ -94,6 +94,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat calldataPayload; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { let internalCalldata := mload(0x40) /** diff --git a/contracts/handler/HandlerContext.sol b/contracts/handler/HandlerContext.sol index 1e6e3993b..92413c57e 100644 --- a/contracts/handler/HandlerContext.sol +++ b/contracts/handler/HandlerContext.sol @@ -20,6 +20,7 @@ abstract contract HandlerContext { function _msgSender() internal pure returns (address sender) { // The assembly code is more direct than the Solidity version using `abi.decode`. // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { sender := shr(96, calldataload(sub(calldatasize(), 20))) } diff --git a/contracts/libraries/CreateCall.sol b/contracts/libraries/CreateCall.sol index 44ae959d8..243f9ba91 100644 --- a/contracts/libraries/CreateCall.sol +++ b/contracts/libraries/CreateCall.sol @@ -20,6 +20,7 @@ contract CreateCall { */ function performCreate2(uint256 value, bytes memory deploymentData, bytes32 salt) public returns (address newContract) { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { newContract := create2(value, add(0x20, deploymentData), mload(deploymentData), salt) } @@ -35,6 +36,7 @@ contract CreateCall { */ function performCreate(uint256 value, bytes memory deploymentData) public returns (address newContract) { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { newContract := create(value, add(deploymentData, 0x20), mload(deploymentData)) } diff --git a/contracts/libraries/MultiSend.sol b/contracts/libraries/MultiSend.sol index 116e24ff4..889a63e26 100644 --- a/contracts/libraries/MultiSend.sol +++ b/contracts/libraries/MultiSend.sol @@ -30,6 +30,7 @@ contract MultiSend { function multiSend(bytes memory transactions) public payable { require(address(this) != multisendSingleton, "MultiSend should only be called via delegatecall"); // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { let length := mload(transactions) let i := 0x20 diff --git a/contracts/libraries/MultiSendCallOnly.sol b/contracts/libraries/MultiSendCallOnly.sol index 7399f1191..8d95f7437 100644 --- a/contracts/libraries/MultiSendCallOnly.sol +++ b/contracts/libraries/MultiSendCallOnly.sol @@ -24,6 +24,7 @@ contract MultiSendCallOnly { */ function multiSend(bytes memory transactions) public payable { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { let length := mload(transactions) let i := 0x20 diff --git a/contracts/proxies/SafeProxyFactory.sol b/contracts/proxies/SafeProxyFactory.sol index 57da0e15d..cb61cdbd9 100644 --- a/contracts/proxies/SafeProxyFactory.sol +++ b/contracts/proxies/SafeProxyFactory.sol @@ -28,6 +28,7 @@ contract SafeProxyFactory { bytes memory deploymentData = abi.encodePacked(type(SafeProxy).creationCode, uint256(uint160(_singleton))); // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt) } @@ -35,6 +36,7 @@ contract SafeProxyFactory { if (initializer.length > 0) { // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { if eq(call(gas(), proxy, 0, add(initializer, 0x20), mload(initializer), 0, 0), 0) { revert(0, 0) @@ -104,6 +106,7 @@ contract SafeProxyFactory { function isContract(address account) internal view returns (bool) { uint256 size; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { size := extcodesize(account) } @@ -117,6 +120,7 @@ contract SafeProxyFactory { function getChainId() public view returns (uint256) { uint256 id; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { id := chainid() } diff --git a/contracts/test/4337/Test4337ModuleAndHandler.sol b/contracts/test/4337/Test4337ModuleAndHandler.sol new file mode 100644 index 000000000..e0a1074e8 --- /dev/null +++ b/contracts/test/4337/Test4337ModuleAndHandler.sol @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.7.0 <0.9.0; +pragma abicoder v2; + +import "../../libraries/SafeStorage.sol"; + +struct UserOperation { + address sender; + uint256 nonce; + bytes initCode; + bytes callData; + uint256 callGasLimit; + uint256 verificationGasLimit; + uint256 preVerificationGas; + uint256 maxFeePerGas; + uint256 maxPriorityFeePerGas; + bytes paymasterAndData; + bytes signature; +} + +interface ISafe { + function execTransactionFromModule(address to, uint256 value, bytes memory data, uint8 operation) external returns (bool success); +} + +/// @dev A Dummy 4337 Module/Handler for testing purposes +/// ⚠️ ⚠️ ⚠️ DO NOT USE IN PRODUCTION ⚠️ ⚠️ ⚠️ +/// The module does not perform ANY validation, it just executes validateUserOp and execTransaction +/// to perform the opcode level compliance by the bundler. +contract Test4337ModuleAndHandler is SafeStorage { + address public immutable myAddress; + address public immutable entryPoint; + + address internal constant SENTINEL_MODULES = address(0x1); + + constructor(address entryPointAddress) { + entryPoint = entryPointAddress; + myAddress = address(this); + } + + function validateUserOp(UserOperation calldata userOp, bytes32, uint256 missingAccountFunds) external returns (uint256 validationData) { + address payable safeAddress = payable(userOp.sender); + ISafe senderSafe = ISafe(safeAddress); + + if (missingAccountFunds != 0) { + senderSafe.execTransactionFromModule(entryPoint, missingAccountFunds, "", 0); + } + + return 0; + } + + function execTransaction(address to, uint256 value, bytes calldata data) external payable { + address payable safeAddress = payable(msg.sender); + ISafe safe = ISafe(safeAddress); + require(safe.execTransactionFromModule(to, value, data, 0), "tx failed"); + } + + function enableMyself() public { + require(myAddress != address(this), "You need to DELEGATECALL, sir"); + + // Module cannot be added twice. + require(modules[myAddress] == address(0), "GS102"); + modules[myAddress] = modules[SENTINEL_MODULES]; + modules[SENTINEL_MODULES] = myAddress; + } +} diff --git a/contracts/test/ERC1155Token.sol b/contracts/test/ERC1155Token.sol index c74c3d690..fe2c0e325 100644 --- a/contracts/test/ERC1155Token.sol +++ b/contracts/test/ERC1155Token.sol @@ -75,6 +75,7 @@ contract ERC1155Token { function isContract(address account) internal view returns (bool) { uint256 size; // solhint-disable-next-line no-inline-assembly + /// @solidity memory-safe-assembly assembly { size := extcodesize(account) } diff --git a/docs/audit_1_4_0.md b/docs/audit_1_4_0.md index a4e15894b..38a19fdf2 100644 --- a/docs/audit_1_4_0.md +++ b/docs/audit_1_4_0.md @@ -8,6 +8,13 @@ The final audit was performed on commit [eb93dbb0f62e2dc1b308ac4c110038062df0a8c9](https://github.com/safe-global/safe-contracts/tree/eb93dbb0f62e2dc1b308ac4c110038062df0a8c9). +There has been one minor bugfix after the audit related to ERC-4337 opcode compatibility: + +- [Pull request](https://github.com/safe-global/safe-contracts/pull/572) +- [Commit](https://github.com/safe-global/safe-contracts/commit/f8bd2159b64392d5b594f4e056be258ade2fefab) + +A change of similar scope has been successfully audited in the 1.4.0 release. Therefore, after notifying the auditors, we concluded no re-audit was necessary. + ##### Files - [Final Audit Report 1.4.0](Safe_Audit_Report_1_4_0.pdf) diff --git a/hardhat.config.ts b/hardhat.config.ts index ffd6765ee..b8b99ba8b 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -1,3 +1,4 @@ +import "@nomiclabs/hardhat-ethers"; import type { HardhatUserConfig, HttpNetworkUserConfig } from "hardhat/types"; import "@nomiclabs/hardhat-etherscan"; import "@nomiclabs/hardhat-waffle"; @@ -41,7 +42,7 @@ import { BigNumber } from "@ethersproject/bignumber"; import { DeterministicDeploymentInfo } from "hardhat-deploy/dist/types"; const primarySolidityVersion = SOLIDITY_VERSION || "0.7.6"; -const soliditySettings = !!SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined; +const soliditySettings = SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined; const deterministicDeployment = (network: string): DeterministicDeploymentInfo => { const info = getSingletonFactoryInfo(parseInt(network)); @@ -132,7 +133,7 @@ const userConfig: HardhatUserConfig = { }, }; if (NODE_URL) { - userConfig.networks!!.custom = { + userConfig.networks!.custom = { ...sharedNetworkConfig, url: NODE_URL, }; diff --git a/package.json b/package.json index 45ccca43a..8f3f6d74c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@safe-global/safe-contracts", - "version": "1.4.0", + "version": "1.4.1", "description": "Ethereum multisig contract", "homepage": "https://github.com/safe-global/safe-contracts/", "license": "LGPL-3.0", @@ -15,7 +15,7 @@ ], "scripts": { "build": "hardhat compile", - "build:ts": "yarn rimraf dist && tsc", + "build:ts": "yarn rimraf dist && tsc -p tsconfig.prod.json", "test": "hardhat test", "coverage": "hardhat coverage", "benchmark": "yarn test benchmark/*.ts", @@ -46,7 +46,7 @@ }, "devDependencies": { "@gnosis.pm/mock-contract": "^4.0.0", - "@gnosis.pm/safe-singleton-factory": "^1.0.3", + "@gnosis.pm/safe-singleton-factory": "^1.0.14", "@nomiclabs/hardhat-ethers": "2.0.2", "@nomiclabs/hardhat-etherscan": "^3.1.7", "@nomiclabs/hardhat-waffle": "2.0.1", @@ -67,7 +67,7 @@ "eslint-plugin-prettier": "^4.2.1", "ethereum-waffle": "^3.3.0", "ethers": "5.4.0", - "hardhat": "^2.2.1", + "hardhat": "^2.14.0", "hardhat-deploy": "0.11.26", "husky": "^5.1.3", "prettier": "^2.8.4", diff --git a/src/utils/execution.ts b/src/utils/execution.ts index 3b03d27df..2d53af156 100644 --- a/src/utils/execution.ts +++ b/src/utils/execution.ts @@ -66,8 +66,8 @@ export const calculateSafeTransactionHash = (safe: Contract, safeTx: SafeTransac }; export const preimageSafeMessageHash = (safe: Contract, message: string, chainId: BigNumberish): string => { - return utils._TypedDataEncoder.encode({ verifyingContract: safe.address, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message }) -} + return utils._TypedDataEncoder.encode({ verifyingContract: safe.address, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message }); +}; export const calculateSafeMessageHash = (safe: Contract, message: string, chainId: BigNumberish): string => { return utils._TypedDataEncoder.hash({ verifyingContract: safe.address, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message }); diff --git a/test/integration/Safe.ERC4337.spec.ts b/test/integration/Safe.ERC4337.spec.ts new file mode 100644 index 000000000..8a943f830 --- /dev/null +++ b/test/integration/Safe.ERC4337.spec.ts @@ -0,0 +1,133 @@ +import hre from "hardhat"; +import { expect } from "chai"; +import { AddressZero } from "@ethersproject/constants"; +import { hexConcat } from "ethers/lib/utils"; +import { getFactoryContract, getSafeSingletonContract } from "../utils/setup"; +import { calculateProxyAddress } from "../../src/utils/proxies"; + +const ERC4337_TEST_ENV_VARIABLES_DEFINED = + typeof process.env.ERC4337_TEST_BUNDLER_URL !== "undefined" && + typeof process.env.ERC4337_TEST_NODE_URL !== "undefined" && + typeof process.env.ERC4337_TEST_SAFE_FACTORY_ADDRESS !== "undefined" && + typeof process.env.ERC4337_TEST_SINGLETON_ADDRESS !== "undefined" && + typeof process.env.MNEMONIC !== "undefined"; + +const itif = ERC4337_TEST_ENV_VARIABLES_DEFINED ? it : it.skip; +const SAFE_FACTORY_ADDRESS = process.env.ERC4337_TEST_SAFE_FACTORY_ADDRESS; +const SINGLETON_ADDRESS = process.env.ERC4337_TEST_SINGLETON_ADDRESS; +const BUNDLER_URL = process.env.ERC4337_TEST_BUNDLER_URL; +const NODE_URL = process.env.ERC4337_TEST_NODE_URL; +const MNEMONIC = process.env.MNEMONIC; + +type UserOperation = { + sender: string; + nonce: string; + initCode: string; + callData: string; + callGasLimit: string; + verificationGasLimit: string; + preVerificationGas: string; + maxFeePerGas: string; + maxPriorityFeePerGas: string; + paymasterAndData: string; + signature: string; +}; + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +describe("Safe.ERC4337", () => { + const setupTests = async () => { + const factory = await getFactoryContract(); + const singleton = await getSafeSingletonContract(); + const bundlerProvider = new hre.ethers.providers.JsonRpcProvider(BUNDLER_URL); + const provider = new hre.ethers.providers.JsonRpcProvider(NODE_URL); + const userWallet = hre.ethers.Wallet.fromMnemonic(MNEMONIC as string).connect(provider); + + const entryPoints = await bundlerProvider.send("eth_supportedEntryPoints", []); + if (entryPoints.length === 0) { + throw new Error("No entry points found"); + } + + return { + factory: factory.attach(SAFE_FACTORY_ADDRESS).connect(userWallet), + singleton: singleton.attach(SINGLETON_ADDRESS).connect(provider), + bundlerProvider, + provider, + userWallet, + entryPoints, + }; + }; + + /** + * This test verifies the ERC4337 based on gas estimation for a user operation + * The user operation deploys a Safe with the ERC4337 module and a handler + * and executes a transaction, thus verifying two things: + * 1. Deployment of the Safe with the ERC4337 module and handler is possible + * 2. Executing a transaction is possible + */ + itif("should pass the ERC4337 validation", async () => { + const { singleton, factory, provider, bundlerProvider, userWallet, entryPoints } = await setupTests(); + const ENTRYPOINT_ADDRESS = entryPoints[0]; + + const erc4337ModuleAndHandlerFactory = (await hre.ethers.getContractFactory("Test4337ModuleAndHandler")).connect(userWallet); + const erc4337ModuleAndHandler = await erc4337ModuleAndHandlerFactory.deploy(ENTRYPOINT_ADDRESS); + // The bundler uses a different node, so we need to allow it sometime to sync + await sleep(10000); + + const feeData = await provider.getFeeData(); + const maxFeePerGas = feeData.maxFeePerGas.toHexString(); + + const maxPriorityFeePerGas = feeData.maxPriorityFeePerGas.toHexString(); + + const moduleInitializer = erc4337ModuleAndHandler.interface.encodeFunctionData("enableMyself", []); + const encodedInitializer = singleton.interface.encodeFunctionData("setup", [ + [userWallet.address], + 1, + erc4337ModuleAndHandler.address, + moduleInitializer, + erc4337ModuleAndHandler.address, + AddressZero, + 0, + AddressZero, + ]); + const deployedAddress = await calculateProxyAddress(factory, singleton.address, encodedInitializer, 73); + + // The initCode contains 20 bytes of the factory address and the rest is the calldata to be forwarded + const initCode = hexConcat([ + factory.address, + factory.interface.encodeFunctionData("createProxyWithNonce", [singleton.address, encodedInitializer, 73]), + ]); + const userOpCallData = erc4337ModuleAndHandler.interface.encodeFunctionData("execTransaction", [userWallet.address, 0, 0]); + + // Native tokens for the pre-fund 💸 + await userWallet.sendTransaction({ to: deployedAddress, value: hre.ethers.utils.parseEther("0.001") }); + // The bundler uses a different node, so we need to allow it sometime to sync + await sleep(10000); + + const userOperation: UserOperation = { + sender: deployedAddress, + nonce: "0x0", + initCode, + callData: userOpCallData, + callGasLimit: "0x7A120", + verificationGasLimit: "0x7A120", + preVerificationGas: "0x186A0", + maxFeePerGas, + maxPriorityFeePerGas, + paymasterAndData: "0x", + signature: "0x", + }; + + const DEBUG_MESSAGE = ` + Using entry point: ${ENTRYPOINT_ADDRESS} + Deployed Safe address: ${deployedAddress} + Module/Handler address: ${erc4337ModuleAndHandler.address} + User operation: + ${JSON.stringify(userOperation, null, 2)} + `; + console.log(DEBUG_MESSAGE); + + const estimatedGas = await bundlerProvider.send("eth_estimateUserOperationGas", [userOperation, ENTRYPOINT_ADDRESS]); + expect(estimatedGas).to.not.be.undefined; + }); +}); diff --git a/test/migration/UpgradeFromSafe111.spec.ts b/test/migration/UpgradeFromSafe111.spec.ts index c4021ae93..43163abe1 100644 --- a/test/migration/UpgradeFromSafe111.spec.ts +++ b/test/migration/UpgradeFromSafe111.spec.ts @@ -33,7 +33,7 @@ describe("Upgrade from Safe 1.1.1", () => { const data = ChangeMasterCopyInterface.encodeFunctionData("changeMasterCopy", [singleton140]); const tx = buildSafeTransaction({ to: safe.address, data, nonce }); await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]); - expect(await safe.VERSION()).to.be.eq("1.4.0"); + expect(await safe.VERSION()).to.be.eq("1.4.1"); return { migratedSafe: safe, diff --git a/test/migration/UpgradeFromSafe120.spec.ts b/test/migration/UpgradeFromSafe120.spec.ts index 779ee46c9..9b8f01f07 100644 --- a/test/migration/UpgradeFromSafe120.spec.ts +++ b/test/migration/UpgradeFromSafe120.spec.ts @@ -33,7 +33,7 @@ describe("Upgrade from Safe 1.2.0", () => { const data = ChangeMasterCopyInterface.encodeFunctionData("changeMasterCopy", [singleton140]); const tx = buildSafeTransaction({ to: safe.address, data, nonce }); await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]); - expect(await safe.VERSION()).to.be.eq("1.4.0"); + expect(await safe.VERSION()).to.be.eq("1.4.1"); return { migratedSafe: safe, diff --git a/test/migration/subTests.spec.ts b/test/migration/subTests.spec.ts index 6fd75c9c0..5b0259654 100644 --- a/test/migration/subTests.spec.ts +++ b/test/migration/subTests.spec.ts @@ -1,5 +1,5 @@ import { BigNumber } from "ethers"; -import { Contract } from "@ethersproject/contracts"; +import { Contract } from "ethers"; import { parseEther } from "@ethersproject/units"; import { expect } from "chai"; import hre, { ethers, waffle } from "hardhat"; diff --git a/test/utils/setup.ts b/test/utils/setup.ts index a44644eb3..946f497cd 100644 --- a/test/utils/setup.ts +++ b/test/utils/setup.ts @@ -28,6 +28,18 @@ export const getSafeSingleton = async () => { return Safe.attach(SafeDeployment.address); }; +export const getSafeSingletonContract = async () => { + const safeSingleton = await hre.ethers.getContractFactory(safeContractUnderTest()); + + return safeSingleton; +}; + +export const getFactoryContract = async () => { + const factory = await hre.ethers.getContractFactory("SafeProxyFactory"); + + return factory; +}; + export const getFactory = async () => { const FactoryDeployment = await deployments.get("SafeProxyFactory"); const Factory = await hre.ethers.getContractFactory("SafeProxyFactory"); diff --git a/tsconfig.json b/tsconfig.json index fe78c009e..bcb8d7515 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,22 +1,23 @@ { "compilerOptions": { - "target": "es2017" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. */, - "module": "commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', 'es2020', or 'ESNext'. */, - "lib": ["ES2015"], - "allowJs": false /* Allow javascript files to be compiled. */, - "declaration": true /* Generates corresponding '.d.ts' file. */, - "sourceMap": false /* Generates corresponding '.map' file. */, - "outDir": "./dist" /* Redirect output structure to the directory. */, - "strict": true /* Enable all strict type-checking options. */, - "noUnusedParameters": true /* Report errors on unused parameters. */, - "noImplicitReturns": true /* Report error when not all code paths in function return a value. */, - "noFallthroughCasesInSwitch": true /* Report errors for fallthrough cases in switch statement. */, - "moduleResolution": "node" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */, - "esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */, - "experimentalDecorators": true /* Enables experimental support for ES7 decorators. */, - "forceConsistentCasingInFileNames": true /* Disallow inconsistently-cased references to the same file. */, - "resolveJsonModule": true + "target": "es2017" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. */, + "module": "commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', 'es2020', or 'ESNext'. */, + "lib": ["ES2015"], + "allowJs": false /* Allow javascript files to be compiled. */, + "declaration": true /* Generates corresponding '.d.ts' file. */, + "sourceMap": false /* Generates corresponding '.map' file. */, + "outDir": "./dist" /* Redirect output structure to the directory. */, + "strict": true /* Enable all strict type-checking options. */, + "noUnusedParameters": true /* Report errors on unused parameters. */, + "noImplicitReturns": true /* Report error when not all code paths in function return a value. */, + "noFallthroughCasesInSwitch": true /* Report errors for fallthrough cases in switch statement. */, + "moduleResolution": "node" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */, + "esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */, + "experimentalDecorators": true /* Enables experimental support for ES7 decorators. */, + "forceConsistentCasingInFileNames": true /* Disallow inconsistently-cased references to the same file. */, + "resolveJsonModule": true }, "exclude": ["dist", "node_modules"], - "include": ["./src/index.ts", "./types"] - } \ No newline at end of file + "include": ["./src/index.ts", "./types", "./test/**/*"], + "files": ["./hardhat.config.ts"] +} diff --git a/tsconfig.prod.json b/tsconfig.prod.json new file mode 100644 index 000000000..2e3140993 --- /dev/null +++ b/tsconfig.prod.json @@ -0,0 +1,4 @@ +{ + "extends": "./tsconfig", + "exclude": ["./test/**/*"] +} diff --git a/yarn.lock b/yarn.lock index ccb33ce94..76849e83b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -23,6 +23,42 @@ chalk "^2.0.0" js-tokens "^4.0.0" +"@chainsafe/as-sha256@^0.3.1": + version "0.3.1" + resolved "https://registry.yarnpkg.com/@chainsafe/as-sha256/-/as-sha256-0.3.1.tgz#3639df0e1435cab03f4d9870cc3ac079e57a6fc9" + integrity sha512-hldFFYuf49ed7DAakWVXSJODuq3pzJEguD8tQ7h+sGkM18vja+OFoJI9krnGmgzyuZC2ETX0NOIcCTy31v2Mtg== + +"@chainsafe/persistent-merkle-tree@^0.4.2": + version "0.4.2" + resolved "https://registry.yarnpkg.com/@chainsafe/persistent-merkle-tree/-/persistent-merkle-tree-0.4.2.tgz#4c9ee80cc57cd3be7208d98c40014ad38f36f7ff" + integrity sha512-lLO3ihKPngXLTus/L7WHKaw9PnNJWizlOF1H9NNzHP6Xvh82vzg9F2bzkXhYIFshMZ2gTCEz8tq6STe7r5NDfQ== + dependencies: + "@chainsafe/as-sha256" "^0.3.1" + +"@chainsafe/persistent-merkle-tree@^0.5.0": + version "0.5.0" + resolved "https://registry.yarnpkg.com/@chainsafe/persistent-merkle-tree/-/persistent-merkle-tree-0.5.0.tgz#2b4a62c9489a5739dedd197250d8d2f5427e9f63" + integrity sha512-l0V1b5clxA3iwQLXP40zYjyZYospQLZXzBVIhhr9kDg/1qHZfzzHw0jj4VPBijfYCArZDlPkRi1wZaV2POKeuw== + dependencies: + "@chainsafe/as-sha256" "^0.3.1" + +"@chainsafe/ssz@^0.10.0": + version "0.10.2" + resolved "https://registry.yarnpkg.com/@chainsafe/ssz/-/ssz-0.10.2.tgz#c782929e1bb25fec66ba72e75934b31fd087579e" + integrity sha512-/NL3Lh8K+0q7A3LsiFq09YXS9fPE+ead2rr7vM2QK8PLzrNsw3uqrif9bpRX5UxgeRjM+vYi+boCM3+GM4ovXg== + dependencies: + "@chainsafe/as-sha256" "^0.3.1" + "@chainsafe/persistent-merkle-tree" "^0.5.0" + +"@chainsafe/ssz@^0.9.2": + version "0.9.4" + resolved "https://registry.yarnpkg.com/@chainsafe/ssz/-/ssz-0.9.4.tgz#696a8db46d6975b600f8309ad3a12f7c0e310497" + integrity sha512-77Qtg2N1ayqs4Bg/wvnWfg5Bta7iy7IRh8XqXh7oNMeP2HBbBwx8m6yTpA8p0EHItWPEBkgZd5S5/LSlp3GXuQ== + dependencies: + "@chainsafe/as-sha256" "^0.3.1" + "@chainsafe/persistent-merkle-tree" "^0.4.2" + case "^1.6.3" + "@ensdomains/ens@^0.4.4": version "0.4.5" resolved "https://registry.yarnpkg.com/@ensdomains/ens/-/ens-0.4.5.tgz#e0aebc005afdc066447c6e22feb4eda89a5edbfc" @@ -560,7 +596,7 @@ bech32 "1.1.4" ws "7.4.6" -"@ethersproject/providers@5.7.2", "@ethersproject/providers@^5.7.2": +"@ethersproject/providers@5.7.2", "@ethersproject/providers@^5.7.1", "@ethersproject/providers@^5.7.2": version "5.7.2" resolved "https://registry.yarnpkg.com/@ethersproject/providers/-/providers-5.7.2.tgz#f8b1a4f275d7ce58cf0a2eec222269a08beb18cb" integrity sha512-g34EWZ1WWAVgr4aptGlVBF8mhl3VWjv+8hoAnzStu8Ah22VHBsuGzP17eb6xDVRzw895G4W7vvx60lFFur/1Rg== @@ -840,10 +876,10 @@ resolved "https://registry.yarnpkg.com/@gnosis.pm/mock-contract/-/mock-contract-4.0.0.tgz#eaf500fddcab81b5f6c22280a7b22ff891dd6f87" integrity sha512-SkRq2KwPx6vo0LAjSc8JhgQstrQFXRyn2yqquIfub7r2WHi5nUbF8beeSSXsd36hvBcQxQfmOIYNYRpj9JOhrQ== -"@gnosis.pm/safe-singleton-factory@^1.0.3": - version "1.0.12" - resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-singleton-factory/-/safe-singleton-factory-1.0.12.tgz#4a0b373dc5c4097b8166d8747df348a400fb72a7" - integrity sha512-7nUnXi35EiVKRxypTJ60UMFaf6xA5TLTYyVur2877RDRU02pTabWx1D0wCbdWBMRE0sAxIe302CbIBdpkZuMIg== +"@gnosis.pm/safe-singleton-factory@^1.0.14": + version "1.0.14" + resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-singleton-factory/-/safe-singleton-factory-1.0.14.tgz#42dae9a91fda21b605f94bfe310a7fccc6a4d738" + integrity sha512-xZ26c9uKzpd5Sm8ux0sZHt5QC8n+Q2z1/X5xjPnd8aT5EcKH5t1GgLbAqjrMFmXVIOkiWSc7wi2Bj4XfgtiyaQ== "@humanwhocodes/config-array@^0.11.8": version "0.11.8" @@ -911,29 +947,31 @@ "@nodelib/fs.scandir" "2.1.5" fastq "^1.6.0" -"@nomicfoundation/ethereumjs-block@^4.0.0": - version "4.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-block/-/ethereumjs-block-4.0.0.tgz#fdd5c045e7baa5169abeed0e1202bf94e4481c49" - integrity sha512-bk8uP8VuexLgyIZAHExH1QEovqx0Lzhc9Ntm63nCRKLHXIZkobaFaeCVwTESV7YkPKUk7NiK11s8ryed4CS9yA== - dependencies: - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-trie" "^5.0.0" - "@nomicfoundation/ethereumjs-tx" "^4.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" +"@nomicfoundation/ethereumjs-block@5.0.1": + version "5.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-block/-/ethereumjs-block-5.0.1.tgz#6f89664f55febbd723195b6d0974773d29ee133d" + integrity sha512-u1Yioemi6Ckj3xspygu/SfFvm8vZEO8/Yx5a1QLzi6nVU0jz3Pg2OmHKJ5w+D9Ogk1vhwRiqEBAqcb0GVhCyHw== + dependencies: + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-trie" "6.0.1" + "@nomicfoundation/ethereumjs-tx" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" ethereum-cryptography "0.1.3" + ethers "^5.7.1" -"@nomicfoundation/ethereumjs-blockchain@^6.0.0": - version "6.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-blockchain/-/ethereumjs-blockchain-6.0.0.tgz#1a8c243a46d4d3691631f139bfb3a4a157187b0c" - integrity sha512-pLFEoea6MWd81QQYSReLlLfH7N9v7lH66JC/NMPN848ySPPQA5renWnE7wPByfQFzNrPBuDDRFFULMDmj1C0xw== - dependencies: - "@nomicfoundation/ethereumjs-block" "^4.0.0" - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-ethash" "^2.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-trie" "^5.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" +"@nomicfoundation/ethereumjs-blockchain@7.0.1": + version "7.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-blockchain/-/ethereumjs-blockchain-7.0.1.tgz#80e0bd3535bfeb9baa29836b6f25123dab06a726" + integrity sha512-NhzndlGg829XXbqJEYrF1VeZhAwSPgsK/OB7TVrdzft3y918hW5KNd7gIZ85sn6peDZOdjBsAXIpXZ38oBYE5A== + dependencies: + "@nomicfoundation/ethereumjs-block" "5.0.1" + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-ethash" "3.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-trie" "6.0.1" + "@nomicfoundation/ethereumjs-tx" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" abstract-level "^1.0.3" debug "^4.3.3" ethereum-cryptography "0.1.3" @@ -941,105 +979,105 @@ lru-cache "^5.1.1" memory-level "^1.0.0" -"@nomicfoundation/ethereumjs-common@^3.0.0": - version "3.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-common/-/ethereumjs-common-3.0.0.tgz#f6bcc7753994555e49ab3aa517fc8bcf89c280b9" - integrity sha512-WS7qSshQfxoZOpHG/XqlHEGRG1zmyjYrvmATvc4c62+gZXgre1ymYP8ZNgx/3FyZY0TWe9OjFlKOfLqmgOeYwA== +"@nomicfoundation/ethereumjs-common@4.0.1": + version "4.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-common/-/ethereumjs-common-4.0.1.tgz#4702d82df35b07b5407583b54a45bf728e46a2f0" + integrity sha512-OBErlkfp54GpeiE06brBW/TTbtbuBJV5YI5Nz/aB2evTDo+KawyEzPjBlSr84z/8MFfj8wS2wxzQX1o32cev5g== dependencies: - "@nomicfoundation/ethereumjs-util" "^8.0.0" + "@nomicfoundation/ethereumjs-util" "9.0.1" crc-32 "^1.2.0" -"@nomicfoundation/ethereumjs-ethash@^2.0.0": - version "2.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-ethash/-/ethereumjs-ethash-2.0.0.tgz#11539c32fe0990e1122ff987d1b84cfa34774e81" - integrity sha512-WpDvnRncfDUuXdsAXlI4lXbqUDOA+adYRQaEezIkxqDkc+LDyYDbd/xairmY98GnQzo1zIqsIL6GB5MoMSJDew== +"@nomicfoundation/ethereumjs-ethash@3.0.1": + version "3.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-ethash/-/ethereumjs-ethash-3.0.1.tgz#65ca494d53e71e8415c9a49ef48bc921c538fc41" + integrity sha512-KDjGIB5igzWOp8Ik5I6QiRH5DH+XgILlplsHR7TEuWANZA759G6krQ6o8bvj+tRUz08YygMQu/sGd9mJ1DYT8w== dependencies: - "@nomicfoundation/ethereumjs-block" "^4.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" + "@nomicfoundation/ethereumjs-block" "5.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" abstract-level "^1.0.3" bigint-crypto-utils "^3.0.23" ethereum-cryptography "0.1.3" -"@nomicfoundation/ethereumjs-evm@^1.0.0": - version "1.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-evm/-/ethereumjs-evm-1.0.0.tgz#99cd173c03b59107c156a69c5e215409098a370b" - integrity sha512-hVS6qRo3V1PLKCO210UfcEQHvlG7GqR8iFzp0yyjTg2TmJQizcChKgWo8KFsdMw6AyoLgLhHGHw4HdlP8a4i+Q== +"@nomicfoundation/ethereumjs-evm@2.0.1": + version "2.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-evm/-/ethereumjs-evm-2.0.1.tgz#f35681e203363f69ce2b3d3bf9f44d4e883ca1f1" + integrity sha512-oL8vJcnk0Bx/onl+TgQOQ1t/534GKFaEG17fZmwtPFeH8S5soiBYPCLUrvANOl4sCp9elYxIMzIiTtMtNNN8EQ== dependencies: - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" - "@types/async-eventemitter" "^0.2.1" - async-eventemitter "^0.2.4" + "@ethersproject/providers" "^5.7.1" + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-tx" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" debug "^4.3.3" ethereum-cryptography "0.1.3" mcl-wasm "^0.7.1" rustbn.js "~0.2.0" -"@nomicfoundation/ethereumjs-rlp@^4.0.0", "@nomicfoundation/ethereumjs-rlp@^4.0.0-beta.2": - version "4.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-rlp/-/ethereumjs-rlp-4.0.0.tgz#d9a9c5f0f10310c8849b6525101de455a53e771d" - integrity sha512-GaSOGk5QbUk4eBP5qFbpXoZoZUj/NrW7MRa0tKY4Ew4c2HAS0GXArEMAamtFrkazp0BO4K5p2ZCG3b2FmbShmw== +"@nomicfoundation/ethereumjs-rlp@5.0.1": + version "5.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-rlp/-/ethereumjs-rlp-5.0.1.tgz#0b30c1cf77d125d390408e391c4bb5291ef43c28" + integrity sha512-xtxrMGa8kP4zF5ApBQBtjlSbN5E2HI8m8FYgVSYAnO6ssUoY5pVPGy2H8+xdf/bmMa22Ce8nWMH3aEW8CcqMeQ== -"@nomicfoundation/ethereumjs-statemanager@^1.0.0": - version "1.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-statemanager/-/ethereumjs-statemanager-1.0.0.tgz#14a9d4e1c828230368f7ab520c144c34d8721e4b" - integrity sha512-jCtqFjcd2QejtuAMjQzbil/4NHf5aAWxUc+CvS0JclQpl+7M0bxMofR2AJdtz+P3u0ke2euhYREDiE7iSO31vQ== +"@nomicfoundation/ethereumjs-statemanager@2.0.1": + version "2.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-statemanager/-/ethereumjs-statemanager-2.0.1.tgz#8824a97938db4471911e2d2f140f79195def5935" + integrity sha512-B5ApMOnlruVOR7gisBaYwFX+L/AP7i/2oAahatssjPIBVDF6wTX1K7Qpa39E/nzsH8iYuL3krkYeUFIdO3EMUQ== dependencies: - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-trie" "^5.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" debug "^4.3.3" ethereum-cryptography "0.1.3" - functional-red-black-tree "^1.0.1" + ethers "^5.7.1" + js-sdsl "^4.1.4" -"@nomicfoundation/ethereumjs-trie@^5.0.0": - version "5.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-trie/-/ethereumjs-trie-5.0.0.tgz#dcfbe3be53a94bc061c9767a396c16702bc2f5b7" - integrity sha512-LIj5XdE+s+t6WSuq/ttegJzZ1vliwg6wlb+Y9f4RlBpuK35B9K02bO7xU+E6Rgg9RGptkWd6TVLdedTI4eNc2A== +"@nomicfoundation/ethereumjs-trie@6.0.1": + version "6.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-trie/-/ethereumjs-trie-6.0.1.tgz#662c55f6b50659fd4b22ea9f806a7401cafb7717" + integrity sha512-A64It/IMpDVODzCgxDgAAla8jNjNtsoQZIzZUfIV5AY6Coi4nvn7+VReBn5itlxMiL2yaTlQr9TRWp3CSI6VoA== dependencies: - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" + "@types/readable-stream" "^2.3.13" ethereum-cryptography "0.1.3" readable-stream "^3.6.0" -"@nomicfoundation/ethereumjs-tx@^4.0.0": - version "4.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-tx/-/ethereumjs-tx-4.0.0.tgz#59dc7452b0862b30342966f7052ab9a1f7802f52" - integrity sha512-Gg3Lir2lNUck43Kp/3x6TfBNwcWC9Z1wYue9Nz3v4xjdcv6oDW9QSMJxqsKw9QEGoBBZ+gqwpW7+F05/rs/g1w== +"@nomicfoundation/ethereumjs-tx@5.0.1": + version "5.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-tx/-/ethereumjs-tx-5.0.1.tgz#7629dc2036b4a33c34e9f0a592b43227ef4f0c7d" + integrity sha512-0HwxUF2u2hrsIM1fsasjXvlbDOq1ZHFV2dd1yGq8CA+MEYhaxZr8OTScpVkkxqMwBcc5y83FyPl0J9MZn3kY0w== dependencies: - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" + "@chainsafe/ssz" "^0.9.2" + "@ethersproject/providers" "^5.7.2" + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" ethereum-cryptography "0.1.3" -"@nomicfoundation/ethereumjs-util@^8.0.0": - version "8.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-util/-/ethereumjs-util-8.0.0.tgz#deb2b15d2c308a731e82977aefc4e61ca0ece6c5" - integrity sha512-2emi0NJ/HmTG+CGY58fa+DQuAoroFeSH9gKu9O6JnwTtlzJtgfTixuoOqLEgyyzZVvwfIpRueuePb8TonL1y+A== +"@nomicfoundation/ethereumjs-util@9.0.1": + version "9.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-util/-/ethereumjs-util-9.0.1.tgz#530cda8bae33f8b5020a8f199ed1d0a2ce48ec89" + integrity sha512-TwbhOWQ8QoSCFhV/DDfSmyfFIHjPjFBj957219+V3jTZYZ2rf9PmDtNOeZWAE3p3vlp8xb02XGpd0v6nTUPbsA== dependencies: - "@nomicfoundation/ethereumjs-rlp" "^4.0.0-beta.2" + "@chainsafe/ssz" "^0.10.0" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" ethereum-cryptography "0.1.3" -"@nomicfoundation/ethereumjs-vm@^6.0.0": - version "6.0.0" - resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-vm/-/ethereumjs-vm-6.0.0.tgz#2bb50d332bf41790b01a3767ffec3987585d1de6" - integrity sha512-JMPxvPQ3fzD063Sg3Tp+UdwUkVxMoo1uML6KSzFhMH3hoQi/LMuXBoEHAoW83/vyNS9BxEe6jm6LmT5xdeEJ6w== - dependencies: - "@nomicfoundation/ethereumjs-block" "^4.0.0" - "@nomicfoundation/ethereumjs-blockchain" "^6.0.0" - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-evm" "^1.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-statemanager" "^1.0.0" - "@nomicfoundation/ethereumjs-trie" "^5.0.0" - "@nomicfoundation/ethereumjs-tx" "^4.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" - "@types/async-eventemitter" "^0.2.1" - async-eventemitter "^0.2.4" +"@nomicfoundation/ethereumjs-vm@7.0.1": + version "7.0.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/ethereumjs-vm/-/ethereumjs-vm-7.0.1.tgz#7d035e0993bcad10716c8b36e61dfb87fa3ca05f" + integrity sha512-rArhyn0jPsS/D+ApFsz3yVJMQ29+pVzNZ0VJgkzAZ+7FqXSRtThl1C1prhmlVr3YNUlfpZ69Ak+RUT4g7VoOuQ== + dependencies: + "@nomicfoundation/ethereumjs-block" "5.0.1" + "@nomicfoundation/ethereumjs-blockchain" "7.0.1" + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-evm" "2.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-statemanager" "2.0.1" + "@nomicfoundation/ethereumjs-trie" "6.0.1" + "@nomicfoundation/ethereumjs-tx" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" debug "^4.3.3" ethereum-cryptography "0.1.3" - functional-red-black-tree "^1.0.1" mcl-wasm "^0.7.1" rustbn.js "~0.2.0" @@ -1346,11 +1384,6 @@ dependencies: ethers "^5.0.2" -"@types/async-eventemitter@^0.2.1": - version "0.2.1" - resolved "https://registry.yarnpkg.com/@types/async-eventemitter/-/async-eventemitter-0.2.1.tgz#f8e6280e87e8c60b2b938624b0a3530fb3e24712" - integrity sha512-M2P4Ng26QbAeITiH7w1d7OxtldgfAe0wobpyJzVK/XOb0cUGKU2R4pfAhqcJBXAe2ife5ZOhSv4wk7p+ffURtg== - "@types/bn.js@*", "@types/bn.js@^5.1.0": version "5.1.1" resolved "https://registry.yarnpkg.com/@types/bn.js/-/bn.js-5.1.1.tgz#b51e1b55920a4ca26e9285ff79936bbdec910682" @@ -1467,6 +1500,14 @@ resolved "https://registry.yarnpkg.com/@types/qs/-/qs-6.9.7.tgz#63bb7d067db107cc1e457c303bc25d511febf6cb" integrity sha512-FGa1F62FT09qcrueBA6qYTrJPVDzah9a+493+o2PCXsesWHIn27G98TsSMs3WPNbZIEj4+VJf6saSFpvD+3Zsw== +"@types/readable-stream@^2.3.13": + version "2.3.15" + resolved "https://registry.yarnpkg.com/@types/readable-stream/-/readable-stream-2.3.15.tgz#3d79c9ceb1b6a57d5f6e6976f489b9b5384321ae" + integrity sha512-oM5JSKQCcICF1wvGgmecmHldZ48OZamtMxcGGVICOJA8o8cahXC1zEVAif8iwoc5j8etxFaRFnf095+CDsuoFQ== + dependencies: + "@types/node" "*" + safe-buffer "~5.1.1" + "@types/resolve@^0.0.8": version "0.0.8" resolved "https://registry.yarnpkg.com/@types/resolve/-/resolve-0.0.8.tgz#f26074d238e02659e323ce1a13d041eee280e194" @@ -1975,7 +2016,7 @@ astral-regex@^2.0.0: resolved "https://registry.yarnpkg.com/astral-regex/-/astral-regex-2.0.0.tgz#483143c567aeed4785759c0865786dc77d7d2e31" integrity sha512-Z7tMw1ytTXt5jqMcOP+OQteU1VuNK9Y02uuJtKQ1Sv69jXQKKg5cibLwGJow8yzZP+eAc18EmLGPal0bp36rvQ== -async-eventemitter@^0.2.2, async-eventemitter@^0.2.4: +async-eventemitter@^0.2.2: version "0.2.4" resolved "https://registry.yarnpkg.com/async-eventemitter/-/async-eventemitter-0.2.4.tgz#f5e7c8ca7d3e46aab9ec40a292baf686a0bafaca" integrity sha512-pd20BwL7Yt1zwDFy+8MX8F1+WCT8aQeKj0kQnTrH9WaeRETlRamVhD0JtRPmrV4GfOJ2F9CvdQkZeZhnh2TuHw== @@ -2996,6 +3037,11 @@ caniuse-lite@^1.0.30000844: resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001441.tgz#987437b266260b640a23cd18fbddb509d7f69f3e" integrity sha512-OyxRR4Vof59I3yGWXws6i908EtGbMzVUi3ganaZQHmydk1iwDhRnvaPG2WaR0KcqrDFKrxVZHULT396LEPhXfg== +case@^1.6.3: + version "1.6.3" + resolved "https://registry.yarnpkg.com/case/-/case-1.6.3.tgz#0a4386e3e9825351ca2e6216c60467ff5f1ea1c9" + integrity sha512-mzDSXIPaFwVDvZAHqZ9VlbyF4yyXRuX6IvB06WvPYkqJVO24kX1PPhv9bfpKNFZyxYFmmgo03HUiD8iklmJYRQ== + caseless@~0.12.0: version "0.12.0" resolved "https://registry.yarnpkg.com/caseless/-/caseless-0.12.0.tgz#1b681c21ff84033c826543090689420d187151dc" @@ -4587,7 +4633,7 @@ ethers@^4.0.32: uuid "2.0.1" xmlhttprequest "1.8.0" -ethers@^5.0.1, ethers@^5.0.2, ethers@^5.5.2, ethers@^5.5.3: +ethers@^5.0.1, ethers@^5.0.2, ethers@^5.5.2, ethers@^5.5.3, ethers@^5.7.1: version "5.7.2" resolved "https://registry.yarnpkg.com/ethers/-/ethers-5.7.2.tgz#3a7deeabbb8c030d4126b24f84e525466145872e" integrity sha512-wswUsmWo1aOK8rR7DIKiWSw9DbLWe6x98Jrn8wcTflTVvaXhAMaB5zGAXy0GYQEQp9iO1iSHWVyARQm11zUtyg== @@ -5484,23 +5530,23 @@ hardhat-deploy@0.11.26: qs "^6.9.4" zksync-web3 "^0.14.3" -hardhat@^2.2.1: - version "2.12.4" - resolved "https://registry.yarnpkg.com/hardhat/-/hardhat-2.12.4.tgz#e539ba58bee9ba1a1ced823bfdcec0b3c5a3e70f" - integrity sha512-rc9S2U/4M+77LxW1Kg7oqMMmjl81tzn5rNFARhbXKUA1am/nhfMJEujOjuKvt+ZGMiZ11PYSe8gyIpB/aRNDgw== +hardhat@^2.14.0: + version "2.14.0" + resolved "https://registry.yarnpkg.com/hardhat/-/hardhat-2.14.0.tgz#b60c74861494aeb1b50803cf04cc47865a42b87a" + integrity sha512-73jsInY4zZahMSVFurSK+5TNCJTXMv+vemvGia0Ac34Mm19fYp6vEPVGF3sucbumszsYxiTT2TbS8Ii2dsDSoQ== dependencies: "@ethersproject/abi" "^5.1.2" "@metamask/eth-sig-util" "^4.0.0" - "@nomicfoundation/ethereumjs-block" "^4.0.0" - "@nomicfoundation/ethereumjs-blockchain" "^6.0.0" - "@nomicfoundation/ethereumjs-common" "^3.0.0" - "@nomicfoundation/ethereumjs-evm" "^1.0.0" - "@nomicfoundation/ethereumjs-rlp" "^4.0.0" - "@nomicfoundation/ethereumjs-statemanager" "^1.0.0" - "@nomicfoundation/ethereumjs-trie" "^5.0.0" - "@nomicfoundation/ethereumjs-tx" "^4.0.0" - "@nomicfoundation/ethereumjs-util" "^8.0.0" - "@nomicfoundation/ethereumjs-vm" "^6.0.0" + "@nomicfoundation/ethereumjs-block" "5.0.1" + "@nomicfoundation/ethereumjs-blockchain" "7.0.1" + "@nomicfoundation/ethereumjs-common" "4.0.1" + "@nomicfoundation/ethereumjs-evm" "2.0.1" + "@nomicfoundation/ethereumjs-rlp" "5.0.1" + "@nomicfoundation/ethereumjs-statemanager" "2.0.1" + "@nomicfoundation/ethereumjs-trie" "6.0.1" + "@nomicfoundation/ethereumjs-tx" "5.0.1" + "@nomicfoundation/ethereumjs-util" "9.0.1" + "@nomicfoundation/ethereumjs-vm" "7.0.1" "@nomicfoundation/solidity-analyzer" "^0.1.0" "@sentry/node" "^5.18.1" "@types/bn.js" "^5.1.0" @@ -5536,7 +5582,7 @@ hardhat@^2.2.1: source-map-support "^0.5.13" stacktrace-parser "^0.1.10" tsort "0.0.1" - undici "^5.4.0" + undici "^5.14.0" uuid "^8.3.2" ws "^7.4.6" @@ -7804,12 +7850,7 @@ prettier@^1.14.3: resolved "https://registry.yarnpkg.com/prettier/-/prettier-1.19.1.tgz#f7d7f5ff8a9cd872a7be4ca142095956a60797cb" integrity sha512-s7PoyDv/II1ObgQunCbB9PdLmUcBZcnWOcxDh7O0N/UwDEsHyqkW+Qh28jW+mVuCdx7gLB0BotYI1Y6uI9iyew== -prettier@^2.1.2: - version "2.8.1" - resolved "https://registry.yarnpkg.com/prettier/-/prettier-2.8.1.tgz#4e1fd11c34e2421bc1da9aea9bd8127cd0a35efc" - integrity sha512-lqGoSJBQNJidqCHE80vqZJHWHRFoNYsSpP9AjFhlhi9ODCJA541svILes/+/1GM3VaL/abZi7cpFzOpdR9UPKg== - -prettier@^2.8.4: +prettier@^2.1.2, prettier@^2.8.4: version "2.8.4" resolved "https://registry.yarnpkg.com/prettier/-/prettier-2.8.4.tgz#34dd2595629bfbb79d344ac4a91ff948694463c3" integrity sha512-vIS4Rlc2FNh0BySk3Wkd6xmwxB0FpOndW5fisM5H8hsZSxU2VWVB5CWIkIjWvrHjIhxk2g3bfMKM87zNTrZddw== @@ -9524,20 +9565,13 @@ underscore@1.9.1: resolved "https://registry.yarnpkg.com/underscore/-/underscore-1.9.1.tgz#06dce34a0e68a7babc29b365b8e74b8925203961" integrity sha512-5/4etnCkd9c8gwgowi5/om/mYO5ajCaOgdzj/oW+0eQV9WxKBDZw5+ycmKmeaTXjInS/W0BzpGLo2xR2aBwZdg== -undici@^5.14.0: +undici@^5.14.0, undici@^5.4.0: version "5.21.2" resolved "https://registry.yarnpkg.com/undici/-/undici-5.21.2.tgz#329f628aaea3f1539a28b9325dccc72097d29acd" integrity sha512-f6pTQ9RF4DQtwoWSaC42P/NKlUjvezVvd9r155ohqkwFNRyBKM3f3pcty3ouusefNRyM25XhIQEbeQ46sZDJfQ== dependencies: busboy "^1.6.0" -undici@^5.4.0: - version "5.14.0" - resolved "https://registry.yarnpkg.com/undici/-/undici-5.14.0.tgz#1169d0cdee06a4ffdd30810f6228d57998884d00" - integrity sha512-yJlHYw6yXPPsuOH0x2Ib1Km61vu4hLiRRQoafs+WUgX1vO64vgnxiCEN9dpIrhZyHFsai3F0AEj4P9zy19enEQ== - dependencies: - busboy "^1.6.0" - union-value@^1.0.0: version "1.0.1" resolved "https://registry.yarnpkg.com/union-value/-/union-value-1.0.1.tgz#0b6fe7b835aecda61c6ea4d4f02c14221e109847" From 1a87152113dce0cb8428f68f7b6f8e852720cca2 Mon Sep 17 00:00:00 2001 From: Mikhail Date: Fri, 30 Jun 2023 15:38:35 +0200 Subject: [PATCH 3/8] fix patch for fv --- certora/applyHarness.patch | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/certora/applyHarness.patch b/certora/applyHarness.patch index d8894d6a6..5830ddbaa 100644 --- a/certora/applyHarness.patch +++ b/certora/applyHarness.patch @@ -1,29 +1,29 @@ -diff -druN Safe.sol Safe.sol ---- Safe.sol 2023-05-16 15:08:39 -+++ Safe.sol 2023-05-25 16:23:56 -@@ -76,7 +76,7 @@ - * so we create a Safe with 0 owners and threshold 1. - * This is an unusable Safe, perfect for the singleton - */ -- threshold = 1; -+ // threshold = 1; MUNGED: remove and add to constructor of the harness - } - - /** diff -druN base/Executor.sol base/Executor.sol ---- base/Executor.sol 2023-05-16 15:08:39 -+++ base/Executor.sol 2023-05-25 16:23:31 -@@ -25,11 +25,9 @@ - Enum.Operation operation, +--- base/Executor.sol 2023-06-30 15:32:21.392860349 +0200 ++++ base/Executor.sol 2023-06-30 15:37:58.671801994 +0200 +@@ -26,11 +26,8 @@ uint256 txGas ) internal returns (bool success) { -+ // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true if (operation == Enum.Operation.DelegateCall) { - // solhint-disable-next-line no-inline-assembly +- /// @solidity memory-safe-assembly - assembly { - success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) - } ++ // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true + return true; } else { // solhint-disable-next-line no-inline-assembly - assembly { + /// @solidity memory-safe-assembly +diff -druN Safe.sol Safe.sol +--- Safe.sol 2023-06-30 15:32:21.392860349 +0200 ++++ Safe.sol 2023-06-30 15:37:17.198953773 +0200 +@@ -76,7 +76,7 @@ + * so we create a Safe with 0 owners and threshold 1. + * This is an unusable Safe, perfect for the singleton + */ +- threshold = 1; ++ // threshold = 1; MUNGED: remove and add to constructor of the harness + } + + /** From 04403af866484d1cd24335845bd860e21493f500 Mon Sep 17 00:00:00 2001 From: Mikhail Date: Fri, 30 Jun 2023 15:59:44 +0200 Subject: [PATCH 4/8] tinkering with certora --- .github/workflows/certora.yml | 2 +- certora/scripts/verifySafe.sh | 2 +- certora/specs/Safe.spec | 27 +++++++++++++-------------- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/.github/workflows/certora.yml b/.github/workflows/certora.yml index e11365ee6..fb854dc22 100644 --- a/.github/workflows/certora.yml +++ b/.github/workflows/certora.yml @@ -26,7 +26,7 @@ jobs: with: { java-version: "17", java-package: jre, distribution: semeru } - name: Install certora cli-beta - run: pip install -Iv certora-cli-beta==4.2.0 + run: pip install -Iv certora-cli - name: Install solc run: | diff --git a/certora/scripts/verifySafe.sh b/certora/scripts/verifySafe.sh index c909a4188..7315e1bb5 100755 --- a/certora/scripts/verifySafe.sh +++ b/certora/scripts/verifySafe.sh @@ -10,7 +10,7 @@ certoraRun certora/harnesses/SafeHarness.sol \ --verify SafeHarness:certora/specs/Safe.spec \ --solc solc7.6 \ --optimistic_loop \ - --prover_args "-optimisticFallback true" \ + --prover_args '-optimisticFallback true' \ --loop_iter 3 \ --optimistic_hashing \ --hashing_length_bound 352 \ diff --git a/certora/specs/Safe.spec b/certora/specs/Safe.spec index f09780902..cfb2784f5 100644 --- a/certora/specs/Safe.spec +++ b/certora/specs/Safe.spec @@ -1,5 +1,4 @@ methods { - // function getThreshold() external returns (uint256) envfree; function disableModule(address,address) external; function nonce() external returns (uint256) envfree; @@ -11,15 +10,15 @@ methods { function getNativeTokenBalance() external returns (uint256) envfree; // optional - function execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation) external returns (bool, bytes memory); - function execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation) external returns (bool); - function execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool); + function execTransactionFromModuleReturnData(address,uint256,bytes,Enum.Operation) external returns (bool, bytes memory); + function execTransactionFromModule(address,uint256,bytes,Enum.Operation) external returns (bool); + function execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool); } definition noHavoc(method f) returns bool = - f.selector != sig:execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation).selector - && f.selector != sig:execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation).selector - && f.selector != sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector; + f.selector != sig:execTransactionFromModuleReturnData(address,uint256,bytes,Enum.Operation).selector + && f.selector != sig:execTransactionFromModule(address,uint256,bytes,Enum.Operation).selector + && f.selector != sig:execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes).selector; definition reachableOnly(method f) returns bool = f.selector != sig:setup(address[],uint256,address,bytes,address,address,uint256,address).selector @@ -46,7 +45,7 @@ rule nonceMonotonicity(method f) filtered { uint256 nonceAfter = nonce(); assert nonceAfter != nonceBefore => - to_mathint(nonceAfter) == nonceBefore + 1 && f.selector == sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector; + to_mathint(nonceAfter) == nonceBefore + 1 && f.selector == sig:execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes).selector; } @@ -174,16 +173,16 @@ rule nativeTokenBalanceSpending(method f) filtered { uint256 balanceAfter = getNativeTokenBalance(); assert balanceAfter < balanceBefore => - f.selector == sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector - || f.selector == sig:execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation).selector - || f.selector == sig:execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation).selector; + f.selector == sig:execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes).selector + || f.selector == sig:execTransactionFromModule(address,uint256,bytes,Enum.Operation).selector + || f.selector == sig:execTransactionFromModuleReturnData(address,uint256,bytes,Enum.Operation).selector; } rule nativeTokenBalanceSpendingExecTransaction( address to, uint256 value, bytes data, - SafeHarness.Operation operation, + Enum.Operation operation, uint256 safeTxGas, uint256 baseGas, uint256 gasPrice, @@ -208,7 +207,7 @@ rule nativeTokenBalanceSpendingExecTransactionFromModule( address to, uint256 value, bytes data, - SafeHarness.Operation operation + Enum.Operation operation ) { uint256 balanceBefore = getNativeTokenBalance(); env e; @@ -226,7 +225,7 @@ rule nativeTokenBalanceSpendingExecTransactionFromModuleReturnData( address to, uint256 value, bytes data, - SafeHarness.Operation operation + Enum.Operation operation ) { uint256 balanceBefore = getNativeTokenBalance(); env e; From 5da87146a26f611aa666730ae415af6131943dfc Mon Sep 17 00:00:00 2001 From: Mikhail Date: Thu, 6 Jul 2023 11:28:49 +0200 Subject: [PATCH 5/8] remove public getChainId function, make encodeTransactionData private --- contracts/Safe.sol | 21 +++++++-------------- test/core/Safe.Signatures.spec.ts | 7 ------- test/integration/Safe.0xExploit.spec.ts | 8 ++++---- 3 files changed, 11 insertions(+), 25 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index bff476b15..9f2b6512e 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -350,25 +350,18 @@ contract Safe is } /** - * @notice Returns the ID of the chain the contract is currently deployed on. - * @return The ID of the current chain as a uint256. + * @dev Returns the domain separator for this contract, as defined in the EIP-712 standard. + * @return bytes32 The domain separator hash. */ - function getChainId() public view returns (uint256) { - uint256 id; + function domainSeparator() public view returns (bytes32) { + uint256 chainId; // solhint-disable-next-line no-inline-assembly /// @solidity memory-safe-assembly assembly { - id := chainid() + chainId := chainid() } - return id; - } - /** - * @dev Returns the domain separator for this contract, as defined in the EIP-712 standard. - * @return bytes32 The domain separator hash. - */ - function domainSeparator() public view returns (bytes32) { - return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this)); + return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, chainId, this)); } /** @@ -396,7 +389,7 @@ contract Safe is address gasToken, address refundReceiver, uint256 _nonce - ) public view returns (bytes memory) { + ) private view returns (bytes memory) { bytes32 safeTxHash = keccak256( abi.encode( SAFE_TX_TYPEHASH, diff --git a/test/core/Safe.Signatures.spec.ts b/test/core/Safe.Signatures.spec.ts index acee692f6..010779ba9 100644 --- a/test/core/Safe.Signatures.spec.ts +++ b/test/core/Safe.Signatures.spec.ts @@ -58,13 +58,6 @@ describe("Safe", async () => { }); }); - describe("getChainId", async () => { - it("should return correct id", async () => { - const { safe } = await setupTests(); - expect(await safe.getChainId()).to.be.eq(await chainId()); - }); - }); - describe("approveHash", async () => { it("approving should only be allowed for owners", async () => { const { safe } = await setupTests(); diff --git a/test/integration/Safe.0xExploit.spec.ts b/test/integration/Safe.0xExploit.spec.ts index ea26ab5ed..8f233072e 100644 --- a/test/integration/Safe.0xExploit.spec.ts +++ b/test/integration/Safe.0xExploit.spec.ts @@ -40,8 +40,8 @@ describe("Safe", async () => { const nonce = await safe.nonce(); // Use off-chain Safe signature - const messageData = await safe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce); - const messageHash = await messageHandler.getMessageHash(ethers.utils.keccak256(messageData)); + 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); @@ -111,8 +111,8 @@ describe("Safe", async () => { const nonce = await safe.nonce(); // Use off-chain Safe signature - const messageData = await safe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce); - const messageHash = await messageHandler.getMessageHash(messageData); + 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); From 290cb8850bf3bf69e8ecbf305aebd0aeb065a146 Mon Sep 17 00:00:00 2001 From: Mikhail Date: Wed, 5 Jul 2023 11:50:13 +0200 Subject: [PATCH 6/8] Use .call to transfer native token --- .github/workflows/ci.yml | 2 +- contracts/Safe.sol | 5 ++-- contracts/test/TestNativeTokenReceiver.sol | 18 ++++++++++++ test/core/Safe.Execution.spec.ts | 33 ++++++++++++++++++++++ 4 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 contracts/test/TestNativeTokenReceiver.sol diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1259c611..a7c2ab85c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,4 +78,4 @@ jobs: with: path: "**/node_modules" key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }} - - run: (yarn --frozen-lockfile && yarn build && yarn hardhat codesize --contractname Safe && yarn benchmark) || echo "Benchmark failed" + - run: (yarn --frozen-lockfile && yarn build && yarn hardhat codesize --contractname Safe && yarn benchmark) || echo "Benchmark failed" \ No newline at end of file diff --git a/contracts/Safe.sol b/contracts/Safe.sol index bff476b15..62f7caad4 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -239,9 +239,10 @@ contract Safe is // solhint-disable-next-line avoid-tx-origin address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver; if (gasToken == address(0)) { - // For ETH we will only adjust the gas price to not be higher than the actual used gas price + // For native tokens, we will only adjust the gas price to not be higher than the actually used gas price payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice); - require(receiver.send(payment), "GS011"); + (bool refundSuccess, ) = receiver.call{value: payment}(""); + require(refundSuccess, "GS011"); } else { payment = gasUsed.add(baseGas).mul(gasPrice); require(transferToken(gasToken, receiver, payment), "GS012"); diff --git a/contracts/test/TestNativeTokenReceiver.sol b/contracts/test/TestNativeTokenReceiver.sol new file mode 100644 index 000000000..48536f5b2 --- /dev/null +++ b/contracts/test/TestNativeTokenReceiver.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.7.0 <0.9.0; + +/// @title TestNativeTokenReceiver +/// @dev This contract emits an event with sender, value, and remaining gas details whenever it receives Ether. +contract TestNativeTokenReceiver { + /// @dev Emitted when the contract receives Ether. + /// @param from The address of the sender. + /// @param amount The amount of Ether received, in wei. + /// @param forwardedGas The remaining gas at the time of transaction. + event BreadReceived(address indexed from, uint256 amount, uint256 forwardedGas); + + /// @dev Fallback function that is called when the contract receives Ether. + /// Emits the BreadReceived event with the sender's address, the amount of Ether sent, and the remaining gas. + fallback() external payable { + emit BreadReceived(msg.sender, msg.value, gasleft()); + } +} diff --git a/test/core/Safe.Execution.spec.ts b/test/core/Safe.Execution.spec.ts index b1a33e915..57daf313e 100644 --- a/test/core/Safe.Execution.spec.ts +++ b/test/core/Safe.Execution.spec.ts @@ -30,6 +30,9 @@ describe("Safe", async () => { } }`; const storageSetter = await deployContract(user1, setterSource); + const TestNativeTokenReceiver = await hre.ethers.getContractFactory("TestNativeTokenReceiver"); + const nativeTokenReceiver = await TestNativeTokenReceiver.deploy(); + const reverterSource = ` contract Reverter { function revert() public { @@ -41,6 +44,7 @@ describe("Safe", async () => { safe: await getSafeWithOwners([user1.address]), reverter, storageSetter, + nativeTokenReceiver, }; }); @@ -282,5 +286,34 @@ describe("Safe", async () => { "Safe transaction should fail with gasPrice 1 and high gasLimit", ).to.emit(safe, "ExecutionFailure"); }); + + it("should forward all the gas to the native token refund receiver", async () => { + const { safe, nativeTokenReceiver } = await setupTests(); + + const tx = buildSafeTransaction({ + to: user1.address, + nonce: await safe.nonce(), + operation: 0, + gasPrice: 1, + safeTxGas: 0, + refundReceiver: nativeTokenReceiver.address, + }); + + await user1.sendTransaction({ to: safe.address, value: parseEther("1") }); + await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1")); + + const executedTx = await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 500000 }); + const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx.hash); + const parsedLogs = []; + for (const log of receipt.logs) { + try { + parsedLogs.push(nativeTokenReceiver.interface.decodeEventLog("BreadReceived", log.data, log.topics)); + } catch (e) { + continue; + } + } + + expect(parsedLogs[0].forwardedGas.toNumber()).to.be.gte(400000); + }); }); }); From 426e965d6533e1d1e9a2cc8830cdfa1b868a08da Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 1 Aug 2023 13:08:43 +0200 Subject: [PATCH 7/8] Feature: Module guard (#571) * Alternative implmentation for module guard * Fix lint error * Update interfaceId for Guard * d tests * Update guardmanager interface --------- Co-authored-by: Mikhail --- contracts/Safe.sol | 3 +- contracts/base/GuardManager.sol | 38 ++- contracts/base/ModuleManager.sol | 13 +- .../examples/guards/DebugTransactionGuard.sol | 30 +++ .../guards/DelegateCallTransactionGuard.sol | 19 ++ contracts/examples/guards/OnlyOwnersGuard.sol | 16 ++ .../guards/ReentrancyTransactionGuard.sol | 22 ++ contracts/test/TestImports.sol | 6 + contracts/test/Token.sol | 1 - package.json | 2 +- test/core/Safe.GuardManager.spec.ts | 216 +++++++++++++----- yarn.lock | 12 +- 12 files changed, 314 insertions(+), 64 deletions(-) create mode 100644 contracts/test/TestImports.sol diff --git a/contracts/Safe.sol b/contracts/Safe.sol index b84c2c4d2..5904a8aae 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -39,8 +39,7 @@ contract Safe is SecuredTokenTransfer, ISignatureValidatorConstants, FallbackManager, - StorageAccessible, - GuardManager + StorageAccessible { using SafeMath for uint256; diff --git a/contracts/base/GuardManager.sol b/contracts/base/GuardManager.sol index 0a4f4169f..60a454d9d 100644 --- a/contracts/base/GuardManager.sol +++ b/contracts/base/GuardManager.sol @@ -5,7 +5,21 @@ import "../common/Enum.sol"; import "../common/SelfAuthorized.sol"; import "../interfaces/IERC165.sol"; +/// @title Guard Interface interface Guard is IERC165 { + /// @notice Checks the transaction details. + /// @dev The function needs to implement transaction validation logic. + /// @param to The address to which the transaction is intended. + /// @param value The value of the transaction in Wei. + /// @param data The transaction data. + /// @param operation The type of operation of the transaction. + /// @param safeTxGas Gas used for the transaction. + /// @param baseGas The base gas for the transaction. + /// @param gasPrice The price of gas in Wei for the transaction. + /// @param gasToken The token used to pay for gas. + /// @param refundReceiver The address which should receive the refund. + /// @param signatures The signatures of the transaction. + /// @param msgSender The address of the message sender. function checkTransaction( address to, uint256 value, @@ -20,13 +34,33 @@ interface Guard is IERC165 { address msgSender ) external; - function checkAfterExecution(bytes32 txHash, bool success) external; + /// @notice Checks the module transaction details. + /// @dev The function needs to implement module transaction validation logic. + /// @param to The address to which the transaction is intended. + /// @param value The value of the transaction in Wei. + /// @param data The transaction data. + /// @param operation The type of operation of the transaction. + /// @param module The module involved in the transaction. + /// @return moduleTxHash The hash of the module transaction. + function checkModuleTransaction( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + address module + ) external returns (bytes32 moduleTxHash); + + /// @notice Checks after execution of transaction. + /// @dev The function needs to implement a check after the execution of the transaction. + /// @param hash The hash of the transaction. + /// @param success The status of the transaction execution. + function checkAfterExecution(bytes32 hash, bool success) external; } abstract contract BaseGuard is Guard { function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) { return - interfaceId == type(Guard).interfaceId || // 0xe6d7a83a + interfaceId == type(Guard).interfaceId || // 0x945b8148 interfaceId == type(IERC165).interfaceId; // 0x01ffc9a7 } } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index ec5d6f5ae..18df7a357 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -3,6 +3,7 @@ pragma solidity >=0.7.0 <0.9.0; import "../common/Enum.sol"; import "../common/SelfAuthorized.sol"; import "./Executor.sol"; +import "./GuardManager.sol"; /** * @title Module Manager - A contract managing Safe modules @@ -13,7 +14,7 @@ import "./Executor.sol"; * @author Stefan George - @Georgi87 * @author Richard Meissner - @rmeissner */ -abstract contract ModuleManager is SelfAuthorized, Executor { +abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager { event EnabledModule(address indexed module); event DisabledModule(address indexed module); event ExecutionFromModuleSuccess(address indexed module); @@ -87,7 +88,17 @@ abstract contract ModuleManager is SelfAuthorized, Executor { // Only whitelisted modules are allowed. require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104"); // Execute transaction without further confirmations. + address guard = getGuard(); + + bytes32 guardHash; + if (guard != address(0)) { + guardHash = Guard(guard).checkModuleTransaction(to, value, data, operation, msg.sender); + } success = execute(to, value, data, operation, type(uint256).max); + + if (guard != address(0)) { + Guard(guard).checkAfterExecution(guardHash, success); + } if (success) emit ExecutionFromModuleSuccess(msg.sender); else emit ExecutionFromModuleFailure(msg.sender); } diff --git a/contracts/examples/guards/DebugTransactionGuard.sol b/contracts/examples/guards/DebugTransactionGuard.sol index e36b61221..2bf598a28 100644 --- a/contracts/examples/guards/DebugTransactionGuard.sol +++ b/contracts/examples/guards/DebugTransactionGuard.sol @@ -31,6 +31,15 @@ contract DebugTransactionGuard is BaseGuard { address executor ); + event ModuleTransasctionDetails( + bytes32 indexed txHash, + address to, + uint256 value, + bytes data, + Enum.Operation operation, + address module + ); + event GasUsage(address indexed safe, bytes32 indexed txHash, uint256 indexed nonce, bool success); mapping(bytes32 => uint256) public txNonces; @@ -85,4 +94,25 @@ contract DebugTransactionGuard is BaseGuard { txNonces[txHash] = 0; emit GasUsage(msg.sender, txHash, nonce, success); } + + /** + * @notice Called by the Safe contract before a transaction is executed via a module. + * @param to Destination address of Safe transaction. + * @param value Ether value of Safe transaction. + * @param data Data payload of Safe transaction. + * @param operation Operation type of Safe transaction. + * @param module Account executing the transaction. + * @return moduleTxHash Hash of the module transaction. + */ + function checkModuleTransaction( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + address module + ) external override returns (bytes32 moduleTxHash) { + moduleTxHash = keccak256(abi.encodePacked(to, value, data, operation, module)); + + emit ModuleTransasctionDetails(moduleTxHash, to, value, data, operation, module); + } } diff --git a/contracts/examples/guards/DelegateCallTransactionGuard.sol b/contracts/examples/guards/DelegateCallTransactionGuard.sol index 4fc108099..8d6f637ca 100644 --- a/contracts/examples/guards/DelegateCallTransactionGuard.sol +++ b/contracts/examples/guards/DelegateCallTransactionGuard.sol @@ -46,4 +46,23 @@ contract DelegateCallTransactionGuard is BaseGuard { } function checkAfterExecution(bytes32, bool) external view override {} + + /** + * @notice Called by the Safe contract before a transaction is executed via a module. + * @param to Destination address of Safe transaction. + * @param value Ether value of Safe transaction. + * @param data Data payload of Safe transaction. + * @param operation Operation type of Safe transaction. + * @param module Module executing the transaction. + */ + function checkModuleTransaction( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + address module + ) external view override returns (bytes32 moduleTxHash) { + require(operation != Enum.Operation.DelegateCall || to == allowedTarget, "This call is restricted"); + moduleTxHash = keccak256(abi.encodePacked(to, value, data, operation, module)); + } } diff --git a/contracts/examples/guards/OnlyOwnersGuard.sol b/contracts/examples/guards/OnlyOwnersGuard.sol index 95ad4b623..41e200b79 100644 --- a/contracts/examples/guards/OnlyOwnersGuard.sol +++ b/contracts/examples/guards/OnlyOwnersGuard.sol @@ -56,4 +56,20 @@ contract OnlyOwnersGuard is BaseGuard { } function checkAfterExecution(bytes32, bool) external view override {} + + /** + * @notice Called by the Safe contract before a transaction is executed via a module. + * @param to Destination address of Safe transaction. + * @param value Ether value of Safe transaction. + * @param data Data payload of Safe transaction. + * @param operation Operation type of Safe transaction. + * @param module Module executing the transaction. + */ + function checkModuleTransaction( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + address module + ) external override returns (bytes32) {} } diff --git a/contracts/examples/guards/ReentrancyTransactionGuard.sol b/contracts/examples/guards/ReentrancyTransactionGuard.sol index 587f44671..60d4157e7 100644 --- a/contracts/examples/guards/ReentrancyTransactionGuard.sol +++ b/contracts/examples/guards/ReentrancyTransactionGuard.sol @@ -66,4 +66,26 @@ contract ReentrancyTransactionGuard is BaseGuard { function checkAfterExecution(bytes32, bool) external override { getGuard().active = false; } + + /** + * @notice Called by the Safe contract before a transaction is executed via a module. + * @param to Destination address of Safe transaction. + * @param value Ether value of Safe transaction. + * @param data Data payload of Safe transaction. + * @param operation Operation type of Safe transaction. + * @param module Account executing the transaction. + */ + function checkModuleTransaction( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + address module + ) external override returns (bytes32 moduleTxHash) { + moduleTxHash = keccak256(abi.encodePacked(to, value, data, operation, module)); + + GuardValue storage guard = getGuard(); + require(!guard.active, "Reentrancy detected"); + guard.active = true; + } } diff --git a/contracts/test/TestImports.sol b/contracts/test/TestImports.sol new file mode 100644 index 000000000..4cafb1854 --- /dev/null +++ b/contracts/test/TestImports.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: LGPL-3.0-only + +pragma solidity >=0.7.0 <0.9.0; + +// Import the contract so hardhat compiles it, and we have the ABI available +import {MockContract} from "@safe-global/mock-contract/contracts/MockContract.sol"; diff --git a/contracts/test/Token.sol b/contracts/test/Token.sol index b10595ba6..b1eee15f0 100644 --- a/contracts/test/Token.sol +++ b/contracts/test/Token.sol @@ -1,6 +1,5 @@ // SPDX-License-Identifier: LGPL-3.0-only pragma solidity >=0.6.0 <0.7.0; -import "@gnosis.pm/mock-contract/contracts/MockContract.sol"; interface Token { function transfer(address _to, uint256 value) external returns (bool); diff --git a/package.json b/package.json index b252bd850..e45982b2f 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "url": "https://github.com/safe-global/safe-contracts/issues" }, "devDependencies": { - "@gnosis.pm/mock-contract": "^4.0.0", + "@safe-global/mock-contract": "^4.1.0", "@gnosis.pm/safe-singleton-factory": "^1.0.14", "@nomiclabs/hardhat-ethers": "2.0.2", "@nomiclabs/hardhat-etherscan": "^3.1.7", diff --git a/test/core/Safe.GuardManager.spec.ts b/test/core/Safe.GuardManager.spec.ts index 614e1596a..2f82db7a1 100644 --- a/test/core/Safe.GuardManager.spec.ts +++ b/test/core/Safe.GuardManager.spec.ts @@ -13,67 +13,71 @@ import { executeTx, safeApproveHash, } from "../../src/utils/execution"; +import crypto from "crypto"; import { chainId } from "../utils/encoding"; describe("GuardManager", async () => { const [user1, user2] = waffle.provider.getWallets(); + const GUARD_STORAGE_SLOT = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("guard_manager.guard.address")); const setupWithTemplate = deployments.createFixture(async ({ deployments }) => { await deployments.fixture(); - const mock = await getMock(); + const validGuardMock = await getMock(); const guardContract = await hre.ethers.getContractAt("Guard", AddressZero); - const guardEip165Calldata = guardContract.interface.encodeFunctionData("supportsInterface", ["0xe6d7a83a"]); - await mock.givenCalldataReturnBool(guardEip165Calldata, true); + const guardEip165Calldata = guardContract.interface.encodeFunctionData("supportsInterface", ["0x945b8148"]); + await validGuardMock.givenCalldataReturnBool(guardEip165Calldata, true); const safe = await getSafeWithOwners([user2.address]); - await executeContractCallWithSigners(safe, safe, "setGuard", [mock.address], [user2]); + await executeContractCallWithSigners(safe, safe, "setGuard", [validGuardMock.address], [user2]); + return { safe, - mock, + validGuardMock, guardEip165Calldata, }; }); describe("setGuard", async () => { - it("is not called when setting initially", async () => { - const { safe, mock, guardEip165Calldata } = await setupWithTemplate(); + it("reverts if the guard does not implement the ERC165 Guard Interface", async () => { + const safe = await getSafeWithOwners([user1.address]); - const slot = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("guard_manager.guard.address")); - - await executeContractCallWithSigners(safe, safe, "setGuard", [AddressZero], [user2]); + await expect(executeContractCallWithSigners(safe, safe, "setGuard", [user2.address], [user1])).to.be.revertedWith("GS013"); + }); - // Check guard - await expect(await hre.ethers.provider.getStorageAt(safe.address, slot)).to.be.eq("0x" + "".padStart(64, "0")); + it("emits an event when the guard is changed", async () => { + const { validGuardMock } = await setupWithTemplate(); + const safe = await getSafeWithOwners([user1.address]); - await mock.reset(); + await expect(executeContractCallWithSigners(safe, safe, "setGuard", [validGuardMock.address], [user1])) + .to.emit(safe, "ChangedGuard") + .withArgs(validGuardMock.address); - await expect(await hre.ethers.provider.getStorageAt(safe.address, slot)).to.be.eq("0x" + "".padStart(64, "0")); + await expect(executeContractCallWithSigners(safe, safe, "setGuard", [AddressZero], [user1])) + .to.emit(safe, "ChangedGuard") + .withArgs(AddressZero); + }); - // Reverts if it doesn't implement ERC165 Guard Interface - await expect(executeContractCallWithSigners(safe, safe, "setGuard", [mock.address], [user2])).to.be.revertedWith("GS013"); + it("is not called when setting initially", async () => { + const { validGuardMock } = await setupWithTemplate(); + const safe = await getSafeWithOwners([user1.address]); - await mock.givenCalldataReturnBool(guardEip165Calldata, true); - await expect(executeContractCallWithSigners(safe, safe, "setGuard", [mock.address], [user2])) - .to.emit(safe, "ChangedGuard") - .withArgs(mock.address); + await executeContractCallWithSigners(safe, safe, "setGuard", [validGuardMock.address], [user1]); // Check guard - await expect(await hre.ethers.provider.getStorageAt(safe.address, slot)).to.be.eq( - "0x" + mock.address.toLowerCase().slice(2).padStart(64, "0"), + await expect(await hre.ethers.provider.getStorageAt(safe.address, GUARD_STORAGE_SLOT)).to.be.eq( + "0x" + validGuardMock.address.toLowerCase().slice(2).padStart(64, "0"), ); // Guard should not be called, as it was not set before the transaction execution - expect(await mock.callStatic.invocationCount()).to.be.eq(0); + expect(await validGuardMock.invocationCount()).to.be.eq(0); }); it("is called when removed", async () => { - const { safe, mock } = await setupWithTemplate(); - - const slot = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("guard_manager.guard.address")); + const { safe, validGuardMock } = await setupWithTemplate(); // Check guard - await expect(await hre.ethers.provider.getStorageAt(safe.address, slot)).to.be.eq( - "0x" + mock.address.toLowerCase().slice(2).padStart(64, "0"), + await expect(await hre.ethers.provider.getStorageAt(safe.address, GUARD_STORAGE_SLOT)).to.be.eq( + "0x" + validGuardMock.address.toLowerCase().slice(2).padStart(64, "0"), ); const safeTx = buildContractCall(safe, "setGuard", [AddressZero], await safe.nonce()); @@ -85,10 +89,10 @@ describe("GuardManager", async () => { .withArgs(AddressZero); // Check guard - await expect(await hre.ethers.provider.getStorageAt(safe.address, slot)).to.be.eq("0x" + "".padStart(64, "0")); + await expect(await hre.ethers.provider.getStorageAt(safe.address, GUARD_STORAGE_SLOT)).to.be.eq("0x" + "".padStart(64, "0")); - expect(await mock.callStatic.invocationCount()).to.be.eq(2); - const guardInterface = (await hre.ethers.getContractAt("Guard", mock.address)).interface; + expect(await validGuardMock.callStatic.invocationCount()).to.be.eq(2); + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; const checkTxData = guardInterface.encodeFunctionData("checkTransaction", [ safeTx.to, safeTx.value, @@ -102,24 +106,24 @@ describe("GuardManager", async () => { signatureBytes, user1.address, ]); - expect(await mock.callStatic.invocationCountForCalldata(checkTxData)).to.be.eq(1); + expect(await validGuardMock.callStatic.invocationCountForCalldata(checkTxData)).to.be.eq(1); // Guard should also be called for post exec check, even if it is removed with the Safe tx const checkExecData = guardInterface.encodeFunctionData("checkAfterExecution", [ calculateSafeTransactionHash(safe, safeTx, await chainId()), true, ]); - expect(await mock.callStatic.invocationCountForCalldata(checkExecData)).to.be.eq(1); + expect(await validGuardMock.callStatic.invocationCountForCalldata(checkExecData)).to.be.eq(1); }); }); describe("execTransaction", async () => { it("reverts if the pre hook of the guard reverts", async () => { - const { safe, mock } = await setupWithTemplate(); + const { safe, validGuardMock } = await setupWithTemplate(); - const safeTx = buildSafeTransaction({ to: mock.address, data: "0xbaddad42", nonce: 1 }); + const safeTx = buildSafeTransaction({ to: validGuardMock.address, data: "0xbaddad42", nonce: 1 }); const signature = await safeApproveHash(user2, safe, safeTx); const signatureBytes = buildSignatureBytes([signature]); - const guardInterface = (await hre.ethers.getContractAt("Guard", mock.address)).interface; + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; const checkTxData = guardInterface.encodeFunctionData("checkTransaction", [ safeTx.to, safeTx.value, @@ -133,7 +137,7 @@ describe("GuardManager", async () => { signatureBytes, user1.address, ]); - await mock.givenCalldataRevertWithMessage(checkTxData, "Computer says Nah"); + await validGuardMock.givenCalldataRevertWithMessage(checkTxData, "Computer says Nah"); const checkExecData = guardInterface.encodeFunctionData("checkAfterExecution", [ calculateSafeTransactionHash(safe, safeTx, await chainId()), true, @@ -141,23 +145,23 @@ describe("GuardManager", async () => { await expect(executeTx(safe, safeTx, [signature])).to.be.revertedWith("Computer says Nah"); - await mock.reset(); + await validGuardMock.reset(); await expect(executeTx(safe, safeTx, [signature])).to.emit(safe, "ExecutionSuccess"); - expect(await mock.callStatic.invocationCount()).to.be.deep.equals(BigNumber.from(3)); - expect(await mock.callStatic.invocationCountForCalldata(checkTxData)).to.be.deep.equals(BigNumber.from(1)); - expect(await mock.callStatic.invocationCountForCalldata(checkExecData)).to.be.deep.equals(BigNumber.from(1)); - expect(await mock.callStatic.invocationCountForCalldata("0xbaddad42")).to.be.deep.equals(BigNumber.from(1)); + expect(await validGuardMock.callStatic.invocationCount()).to.be.deep.equals(BigNumber.from(3)); + expect(await validGuardMock.callStatic.invocationCountForCalldata(checkTxData)).to.be.deep.equals(BigNumber.from(1)); + expect(await validGuardMock.callStatic.invocationCountForCalldata(checkExecData)).to.be.deep.equals(BigNumber.from(1)); + expect(await validGuardMock.callStatic.invocationCountForCalldata("0xbaddad42")).to.be.deep.equals(BigNumber.from(1)); }); it("reverts if the post hook of the guard reverts", async () => { - const { safe, mock } = await setupWithTemplate(); + const { safe, validGuardMock } = await setupWithTemplate(); - const safeTx = buildSafeTransaction({ to: mock.address, data: "0xbaddad42", nonce: 1 }); + const safeTx = buildSafeTransaction({ to: validGuardMock.address, data: "0xbaddad42", nonce: 1 }); const signature = await safeApproveHash(user2, safe, safeTx); const signatureBytes = buildSignatureBytes([signature]); - const guardInterface = (await hre.ethers.getContractAt("Guard", mock.address)).interface; + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; const checkTxData = guardInterface.encodeFunctionData("checkTransaction", [ safeTx.to, safeTx.value, @@ -175,18 +179,128 @@ describe("GuardManager", async () => { calculateSafeTransactionHash(safe, safeTx, await chainId()), true, ]); - await mock.givenCalldataRevertWithMessage(checkExecData, "Computer says Nah"); + await validGuardMock.givenCalldataRevertWithMessage(checkExecData, "Computer says Nah"); await expect(executeTx(safe, safeTx, [signature])).to.be.revertedWith("Computer says Nah"); - await mock.reset(); + await validGuardMock.reset(); await expect(executeTx(safe, safeTx, [signature])).to.emit(safe, "ExecutionSuccess"); - expect(await mock.callStatic.invocationCount()).to.be.deep.equals(BigNumber.from(3)); - expect(await mock.callStatic.invocationCountForCalldata(checkTxData)).to.be.deep.equals(BigNumber.from(1)); - expect(await mock.callStatic.invocationCountForCalldata(checkExecData)).to.be.deep.equals(BigNumber.from(1)); - expect(await mock.callStatic.invocationCountForCalldata("0xbaddad42")).to.be.deep.equals(BigNumber.from(1)); + expect(await validGuardMock.callStatic.invocationCount()).to.be.deep.equals(BigNumber.from(3)); + expect(await validGuardMock.callStatic.invocationCountForCalldata(checkTxData)).to.be.deep.equals(BigNumber.from(1)); + expect(await validGuardMock.callStatic.invocationCountForCalldata(checkExecData)).to.be.deep.equals(BigNumber.from(1)); + expect(await validGuardMock.callStatic.invocationCountForCalldata("0xbaddad42")).to.be.deep.equals(BigNumber.from(1)); + }); + }); + + describe("execTransactionFromModule", async () => { + it("reverts if the pre hook of the guard reverts", async () => { + const { safe, validGuardMock } = await setupWithTemplate(); + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; + const checkModuleTxData = guardInterface.encodeFunctionData("checkModuleTransaction", [ + user1.address, + 0, + "0xbeef73", + 1, + user1.address, + ]); + + await validGuardMock.givenCalldataRevertWithMessage(checkModuleTxData, "Computer says Nah"); + + await expect(safe.execTransactionFromModule(user1.address, 0, "0xbeef73", 1)).to.be.reverted; + }); + + it("reverts if the post hook of the guard reverts", async () => { + const { safe, validGuardMock } = await setupWithTemplate(); + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; + const checkAfterExecutionTxData = guardInterface.encodeFunctionData("checkAfterExecution", [`0x${"0".repeat(64)}`, true]); + + await validGuardMock.givenCalldataRevertWithMessage(checkAfterExecutionTxData, "Computer says Nah"); + + await expect(safe.execTransactionFromModule(user1.address, 0, "0xbeef73", 1)).to.be.reverted; + }); + + it("preserves the hash returned by checkModuleTransaction and passes it to checkAfterExecution", async () => { + const { safe, validGuardMock } = await setupWithTemplate(); + const hash = "0x" + crypto.randomBytes(32).toString("hex"); + + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; + const checkModuleTxData = guardInterface.encodeFunctionData("checkModuleTransaction", [ + user1.address, + 0, + "0xbeef73", + 1, + user1.address, + ]); + + const checkAfterExecutionTxData = guardInterface.encodeFunctionData("checkAfterExecution", [hash, true]); + await validGuardMock.givenCalldataReturnBytes32(checkModuleTxData, hash); + + await safe.execTransactionFromModule(user1.address, 0, "0xbeef73", 1); + + expect(await validGuardMock.invocationCountForCalldata(checkAfterExecutionTxData)).to.equal(1); + }); + }); + + describe("execTransactionFromModuleReturnData", async () => { + it("reverts if the pre hook of the guard reverts", async () => { + const { safe, validGuardMock } = await setupWithTemplate(); + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; + const checkModuleTxData = guardInterface.encodeFunctionData("checkModuleTransaction", [ + user1.address, + 0, + "0xbeef73", + 1, + user1.address, + ]); + + await validGuardMock.givenCalldataRevertWithMessage(checkModuleTxData, "Computer says Nah"); + + await expect(safe.execTransactionFromModuleReturnData(user1.address, 0, "0xbeef73", 1)).to.be.reverted; + }); + + it("reverts if the post hook of the guard reverts", async () => { + const { safe, validGuardMock } = await setupWithTemplate(); + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; + const checkAfterExecutionTxData = guardInterface.encodeFunctionData("checkAfterExecution", [`0x${"0".repeat(64)}`, true]); + + await validGuardMock.givenCalldataRevertWithMessage(checkAfterExecutionTxData, "Computer says Nah"); + + await expect(safe.execTransactionFromModuleReturnData(user1.address, 0, "0xbeef73", 1)).to.be.reverted; + }); + + it("preserves the hash returned by checkModuleTransaction and passes it to checkAfterExecution", async () => { + const { safe, validGuardMock } = await setupWithTemplate(); + const hash = "0x" + crypto.randomBytes(32).toString("hex"); + + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMock.address)).interface; + const checkModuleTxData = guardInterface.encodeFunctionData("checkModuleTransaction", [ + user1.address, + 0, + "0xbeef73", + 1, + user1.address, + ]); + + const checkAfterExecutionTxData = guardInterface.encodeFunctionData("checkAfterExecution", [hash, true]); + await validGuardMock.givenCalldataReturnBytes32(checkModuleTxData, hash); + + await safe.execTransactionFromModuleReturnData(user1.address, 0, "0xbeef73", 1); + + expect(await validGuardMock.invocationCountForCalldata(checkAfterExecutionTxData)).to.equal(1); }); }); }); diff --git a/yarn.lock b/yarn.lock index 76849e83b..2d0fe4149 100644 --- a/yarn.lock +++ b/yarn.lock @@ -871,11 +871,6 @@ "@ethersproject/properties" "^5.7.0" "@ethersproject/strings" "^5.7.0" -"@gnosis.pm/mock-contract@^4.0.0": - version "4.0.0" - resolved "https://registry.yarnpkg.com/@gnosis.pm/mock-contract/-/mock-contract-4.0.0.tgz#eaf500fddcab81b5f6c22280a7b22ff891dd6f87" - integrity sha512-SkRq2KwPx6vo0LAjSc8JhgQstrQFXRyn2yqquIfub7r2WHi5nUbF8beeSSXsd36hvBcQxQfmOIYNYRpj9JOhrQ== - "@gnosis.pm/safe-singleton-factory@^1.0.14": version "1.0.14" resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-singleton-factory/-/safe-singleton-factory-1.0.14.tgz#42dae9a91fda21b605f94bfe310a7fccc6a4d738" @@ -1218,6 +1213,11 @@ path-browserify "^1.0.0" url "^0.11.0" +"@safe-global/mock-contract@^4.1.0": + version "4.1.0" + resolved "https://registry.yarnpkg.com/@safe-global/mock-contract/-/mock-contract-4.1.0.tgz#6a8ee3afbe094c3a90a21af8e6a88b058acb327f" + integrity sha512-EGLpdNDdA1nx/DDjPmHSDZS2Zrlu3v4EsNw1zbM1Netk8Vf7oMwQetyqtfb8+ZYHhufkumVves3ED/jRlJPwLw== + "@scure/base@~1.1.0": version "1.1.1" resolved "https://registry.yarnpkg.com/@scure/base/-/base-1.1.1.tgz#ebb651ee52ff84f420097055f4bf46cfba403938" @@ -9565,7 +9565,7 @@ underscore@1.9.1: resolved "https://registry.yarnpkg.com/underscore/-/underscore-1.9.1.tgz#06dce34a0e68a7babc29b365b8e74b8925203961" integrity sha512-5/4etnCkd9c8gwgowi5/om/mYO5ajCaOgdzj/oW+0eQV9WxKBDZw5+ycmKmeaTXjInS/W0BzpGLo2xR2aBwZdg== -undici@^5.14.0, undici@^5.4.0: +undici@^5.14.0: version "5.21.2" resolved "https://registry.yarnpkg.com/undici/-/undici-5.21.2.tgz#329f628aaea3f1539a28b9325dccc72097d29acd" integrity sha512-f6pTQ9RF4DQtwoWSaC42P/NKlUjvezVvd9r155ohqkwFNRyBKM3f3pcty3ouusefNRyM25XhIQEbeQ46sZDJfQ== From f9a316bd7bfc192e242188ef23c85a1677758945 Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 1 Aug 2023 13:09:12 +0200 Subject: [PATCH 8/8] Add overloaded `checkNSignatures` (#589) * Add overloaded checkNSignatures * add legacy checknsignatures tests --------- Co-authored-by: Mikhail --- contracts/Safe.sol | 26 +- .../handler/CompatibilityFallbackHandler.sol | 18 +- test/core/Safe.Signatures.spec.ts | 250 ++++++++++++++++-- 3 files changed, 255 insertions(+), 39 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index 5904a8aae..12dae1ad5 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -150,8 +150,7 @@ contract Safe is bytes32 txHash; // Use scope here to limit variable lifetime and prevent `stack too deep` errors { - bytes memory txHashData = encodeTransactionData( - // Transaction info + txHash = getTransactionHash( // Transaction info to, value, data, @@ -163,12 +162,10 @@ contract Safe is gasToken, refundReceiver, // Signature info - nonce + // We use the post-increment here, so the current nonce value is used and incremented afterwards. + nonce++ ); - // Increase nonce and execute transaction. - nonce++; - txHash = keccak256(txHashData); - checkSignatures(txHash, txHashData, signatures); + checkSignatures(txHash, "", signatures); } address guard = getGuard(); { @@ -260,7 +257,7 @@ contract Safe is uint256 _threshold = threshold; // Check that a threshold is set require(_threshold > 0, "GS001"); - checkNSignatures(dataHash, data, signatures, _threshold); + checkNSignatures(msg.sender, dataHash, data, signatures, _threshold); } /** @@ -269,12 +266,21 @@ contract Safe is * The data parameter (bytes) is not used since v1.5.0 as it is not required anymore. Prior to v1.5.0, * data parameter was used in contract signature validation flow using legacy EIP-1271. * Version v1.5.0, uses dataHash parameter instead of data with updated EIP-1271 implementation. + * @param executor Address that executing the transaction. + * ⚠️⚠️⚠️ Make sure that the executor address is a legitmate executor. + * Incorrectly passed the executor might reduce the threshold by 1 signature. ⚠️⚠️⚠️ * @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. * @param requiredSignatures Amount of required valid signatures. */ - function checkNSignatures(bytes32 dataHash, bytes memory /* data */, bytes memory signatures, uint256 requiredSignatures) public view { + function checkNSignatures( + address executor, + bytes32 dataHash, + bytes memory /* data */, + bytes memory signatures, + uint256 requiredSignatures + ) public view { // Check that the provided signature data is not too short require(signatures.length >= requiredSignatures.mul(65), "GS020"); // There cannot be an owner with address 0. @@ -322,7 +328,7 @@ contract Safe is // When handling approved hashes the address of the approver is encoded into r currentOwner = address(uint160(uint256(r))); // Hashes are automatically approved by the sender of the message or when they have been pre-approved via a separate transaction - require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025"); + require(executor == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025"); } else if (v > 30) { // If v > 30 then default va (27,28) has been adjusted for eth_sign flow // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol index fdc515887..56c905292 100644 --- a/contracts/handler/CompatibilityFallbackHandler.sol +++ b/contracts/handler/CompatibilityFallbackHandler.sol @@ -4,12 +4,13 @@ pragma solidity >=0.7.0 <0.9.0; import "./TokenCallbackHandler.sol"; import "../interfaces/ISignatureValidator.sol"; import "../Safe.sol"; +import "./HandlerContext.sol"; /** * @title Compatibility Fallback Handler - Provides compatibility between pre 1.3.0 and 1.3.0+ Safe contracts. * @author Richard Meissner - @rmeissner */ -contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidator { +contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidator, HandlerContext { // keccak256("SafeMessage(bytes message)"); bytes32 private constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca; @@ -154,4 +155,19 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat } } } + + /** + * @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 accepts the executor as a 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. + * @param requiredSignatures Amount of required valid signatures. + */ + function checkNSignatures(bytes32 dataHash, bytes memory, bytes memory signatures, uint256 requiredSignatures) public view { + Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, "", signatures, requiredSignatures); + } } diff --git a/test/core/Safe.Signatures.spec.ts b/test/core/Safe.Signatures.spec.ts index 010779ba9..7b4794095 100644 --- a/test/core/Safe.Signatures.spec.ts +++ b/test/core/Safe.Signatures.spec.ts @@ -24,10 +24,17 @@ describe("Safe", async () => { const setupTests = deployments.createFixture(async ({ deployments }) => { await deployments.fixture(); + const compatFallbackHandler = await getCompatFallbackHandler(); + const safe = await getSafeWithOwners([user1.address], 1, compatFallbackHandler.address); + const safeWithCompatFbHandlerIface = compatFallbackHandler.attach(safe.address); + return { - safe: await getSafeWithOwners([user1.address]), + safe, + compatFallbackHandler, + safeWithCompatFbHandlerIface, }; }); + describe("domainSeparator", async () => { it("should be correct according to EIP-712", async () => { const { safe } = await setupTests(); @@ -244,7 +251,7 @@ describe("Safe", async () => { it("should fail if signature points into static part", async () => { const { safe } = await setupTests(); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = "0x" + @@ -365,11 +372,12 @@ describe("Safe", async () => { }); }); - describe("checkSignatures", async () => { + describe("checkNSignatures", async () => { it("should fail if signature points into static part", async () => { const { safe } = await setupTests(); + + const sender = await safe.signer.getAddress(); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = "0x" + @@ -378,13 +386,13 @@ describe("Safe", async () => { "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v "0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read - await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS021"); + await expect(safe.checkNSignatures(sender, txHash, "0x", signatures, 1)).to.be.revertedWith("GS021"); }); it("should fail if signatures data is not present", async () => { const { safe } = await setupTests(); + const sender = await safe.signer.getAddress(); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = @@ -394,11 +402,13 @@ describe("Safe", async () => { "0000000000000000000000000000000000000000000000000000000000000041" + "00"; // r, s, v - await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS022"); + await expect(safe.checkNSignatures(sender, txHash, "0x", signatures, 1)).to.be.revertedWith("GS022"); }); it("should fail if signatures data is too short", async () => { const { safe } = await setupTests(); + + const sender = await safe.signer.getAddress(); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); @@ -411,50 +421,46 @@ describe("Safe", async () => { "00" + // r, s, v "0000000000000000000000000000000000000000000000000000000000000020"; // length - await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS023"); + await expect(safe.checkNSignatures(sender, txHash, txHashData, signatures, 1)).to.be.revertedWith("GS023"); }); it("should not be able to use different chainId for signing", async () => { - await setupTests(); - const safe = await getSafeWithOwners([user1.address]); + const { safe } = await setupTests(); + const sender = await safe.signer.getAddress(); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = buildSignatureBytes([await safeSignTypedData(user1, safe, tx, 1)]); - await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS026"); + await expect(safe.checkNSignatures(sender, txHash, "0x", signatures, 1)).to.be.revertedWith("GS026"); }); it("if not msg.sender on-chain approval is required", async () => { const { safe } = await setupTests(); const user2Safe = safe.connect(user2); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]); - await expect(user2Safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS025"); + await expect(user2Safe.checkNSignatures(AddressZero, txHash, "0x", signatures, 1)).to.be.revertedWith("GS025"); }); it("should revert if not the required amount of signature data is provided", async () => { await setupTests(); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address]); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); - await expect(safe.checkNSignatures(txHash, txHashData, "0x", 1)).to.be.revertedWith("GS020"); + await expect(safe.checkNSignatures(AddressZero, txHash, "0x", "0x", 1)).to.be.revertedWith("GS020"); }); it("should not be able to use different signature type of same owner", async () => { await setupTests(); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address]); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = buildSignatureBytes([ await safeApproveHash(user1, safe, tx), await safeSignTypedData(user1, safe, tx), await safeSignTypedData(user3, safe, tx), ]); - await expect(safe.checkNSignatures(txHash, txHashData, signatures, 3)).to.be.revertedWith("GS026"); + await expect(safe.checkNSignatures(AddressZero, txHash, "0x", signatures, 3)).to.be.revertedWith("GS026"); }); it("should be able to mix all signature types", async () => { @@ -477,45 +483,233 @@ describe("Safe", async () => { signerSafeSig, ]); - await safe.checkNSignatures(txHash, "0x", signatures, 5); + await safe.checkNSignatures(user1.address, txHash, "0x", signatures, 5); }); it("should be able to require no signatures", async () => { - await setupTests(); - const safe = await getSafeTemplate(); + const { safe } = await setupTests(); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); - await safe.checkNSignatures(txHash, txHashData, "0x", 0); + await safe.checkNSignatures(AddressZero, txHash, "0x", "0x", 0); }); it("should be able to require less signatures than the threshold", async () => { await setupTests(); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address]); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = buildSignatureBytes([await safeSignTypedData(user3, safe, tx)]); - await safe.checkNSignatures(txHash, txHashData, signatures, 1); + await safe.checkNSignatures(AddressZero, txHash, "0x", signatures, 1); }); it("should be able to require more signatures than the threshold", async () => { await setupTests(); const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address], 2); const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); - const txHashData = preimageSafeTransactionHash(safe, tx, await chainId()); const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); const signatures = buildSignatureBytes([ await safeApproveHash(user1, safe, tx, true), await safeApproveHash(user4, safe, tx), await safeSignTypedData(user2, safe, tx), ]); + const sender = await safe.signer.getAddress(); + // Should fail as only 3 signatures are provided + await expect(safe.checkNSignatures(sender, txHash, "0x", signatures, 4)).to.be.revertedWith("GS020"); + + await safe.checkNSignatures(sender, txHash, "0x", signatures, 3); + }); + + it("Should accept an arbitrary msg.sender", async () => { + await setupTests(); + + const safe = await getSafeWithOwners([user1.address]); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + + const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]); + const safeConnectUser2 = safe.connect(user2); + + await safeConnectUser2.checkNSignatures(user1.address, txHash, "0x", signatures, 1); + }); + }); + + describe("checkNSignatures (legacy)", async () => { + it("should use msg.sender executing the check", async () => { + // We attach the safe to user2 but the only owner of the safe is user1 + // If it fails to preserve the msg.sender, it will fail because user2 is not an owner + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + + const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]); + const safeConnectedUser2 = safeWithCompatFbHandlerIface.connect(user2); + + await expect(safeConnectedUser2.checkNSignatures(txHash, "0x", signatures, 1)).to.be.revertedWith("GS025"); + }); + + it("should fail if signature points into static part", async () => { + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + const signatures = + "0x" + + "000000000000000000000000" + + user1.address.slice(2) + + "0000000000000000000000000000000000000000000000000000000000000020" + + "00" + // r, s, v + "0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 1)).to.be.revertedWith("GS021"); + }); + + it("should fail if signatures data is not present", async () => { + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + + const signatures = + "0x" + + "000000000000000000000000" + + user1.address.slice(2) + + "0000000000000000000000000000000000000000000000000000000000000041" + + "00"; // r, s, v + + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 1)).to.be.revertedWith("GS022"); + }); + + it("should fail if signatures data is too short", async () => { + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + + const signatures = + "0x" + + "000000000000000000000000" + + user1.address.slice(2) + + "0000000000000000000000000000000000000000000000000000000000000041" + + "00" + // r, s, v + "0000000000000000000000000000000000000000000000000000000000000020"; // length + + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 1)).to.be.revertedWith("GS023"); + }); + + it("should not be able to use different chainId for signing", async () => { + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + const signatures = buildSignatureBytes([await safeSignTypedData(user1, safe, tx, 1)]); + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 1)).to.be.revertedWith("GS026"); + }); + + it("if not msg.sender on-chain approval is required", async () => { + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + const user2Safe = safeWithCompatFbHandlerIface.connect(user2); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]); + await expect(user2Safe.checkNSignatures(txHash, "0x", signatures, 1)).to.be.revertedWith("GS025"); + }); + + it("should revert if not the required amount of signature data is provided", async () => { + const { compatFallbackHandler } = await setupTests(); + const safe = await getSafeWithOwners([user1.address, user2.address, user3.address], 3, compatFallbackHandler.address); + const safeWithCompatFbHandlerIface = compatFallbackHandler.attach(safe.address); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", "0x", 1)).to.be.revertedWith("GS020"); + }); + + it("should not be able to use different signature type of same owner", async () => { + const { compatFallbackHandler } = await setupTests(); + const safe = await getSafeWithOwners([user1.address, user2.address, user3.address], 3, compatFallbackHandler.address); + const safeWithCompatFbHandlerIface = compatFallbackHandler.attach(safe.address); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + const signatures = buildSignatureBytes([ + await safeApproveHash(user1, safe, tx), + await safeSignTypedData(user1, safe, tx), + await safeSignTypedData(user3, safe, tx), + ]); + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 3)).to.be.revertedWith("GS026"); + }); + + it("should be able to mix all signature types", async () => { + await setupTests(); + const compatFallbackHandler = await getCompatFallbackHandler(); + const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address); + const safe = await getSafeWithOwners( + [user1.address, user2.address, user3.address, user4.address, signerSafe.address], + 5, + compatFallbackHandler.address, + ); + const safeWithCompatFbHandlerIface = compatFallbackHandler.attach(safe.address); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + + const safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId()); + const signerSafeOwnerSignature = await signHash(user5, safeMessageHash); + const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data); + + const signatures = buildSignatureBytes([ + await safeApproveHash(user1, safe, tx, true), + await safeApproveHash(user4, safe, tx), + await safeSignTypedData(user2, safe, tx), + await safeSignTypedData(user3, safe, tx), + signerSafeSig, + ]); + + await safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 5); + }); + + it("should be able to require no signatures", async () => { + const { safe, safeWithCompatFbHandlerIface } = await setupTests(); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + + await safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", "0x", 0); + }); + + it("should be able to require less signatures than the threshold", async () => { + const { compatFallbackHandler } = await setupTests(); + const safe = await getSafeWithOwners( + [user1.address, user2.address, user3.address, user4.address], + 4, + compatFallbackHandler.address, + ); + const safeWithCompatFbHandlerIface = compatFallbackHandler.attach(safe.address); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + const signatures = buildSignatureBytes([await safeSignTypedData(user3, safe, tx)]); + + await safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 1); + }); + + it("should be able to require more signatures than the threshold", async () => { + const { compatFallbackHandler } = await setupTests(); + const safe = await getSafeWithOwners( + [user1.address, user2.address, user3.address, user4.address], + 2, + compatFallbackHandler.address, + ); + const safeWithCompatFbHandlerIface = compatFallbackHandler.attach(safe.address); + const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() }); + const txHash = calculateSafeTransactionHash(safe, tx, await chainId()); + const signatures = buildSignatureBytes([ + await safeApproveHash(user1, safe, tx, true), + await safeApproveHash(user4, safe, tx), + await safeSignTypedData(user2, safe, tx), + ]); + // Should fail as only 3 signatures are provided - await expect(safe.checkNSignatures(txHash, txHashData, signatures, 4)).to.be.revertedWith("GS020"); + await expect(safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 4)).to.be.revertedWith("GS020"); - await safe.checkNSignatures(txHash, txHashData, signatures, 3); + await safeWithCompatFbHandlerIface.checkNSignatures(txHash, "0x", signatures, 3); }); }); });