diff --git a/certora/applyHarness.patch b/certora/applyHarness.patch index 3d88a5ccf..afb171687 100644 --- a/certora/applyHarness.patch +++ b/certora/applyHarness.patch @@ -1,54 +1,7 @@ -diff -druN base/Executor.sol base/Executor.sol ---- base/Executor.sol 2024-04-17 14:32:25.133704630 +0200 -+++ base/Executor.sol 2024-04-18 12:13:12.682392677 +0200 -@@ -26,12 +26,8 @@ - uint256 txGas - ) internal returns (bool success) { - if (operation == Enum.Operation.DelegateCall) { -- /* solhint-disable no-inline-assembly */ -- /// @solidity memory-safe-assembly -- assembly { -- success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) -- } -- /* solhint-enable no-inline-assembly */ -+ // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true -+ return true; - } else { - /* solhint-disable no-inline-assembly */ - /// @solidity memory-safe-assembly -diff -druN interfaces/ISafe.sol interfaces/ISafe.sol ---- interfaces/ISafe.sol 2024-04-18 13:33:47.817387950 +0200 -+++ interfaces/ISafe.sol 2024-04-24 12:56:22.448349149 +0200 -@@ -109,7 +109,7 @@ - */ - function domainSeparator() external view returns (bytes32); - -- /** -+ /* - * @notice Returns transaction hash to be signed by owners. - * @param to Destination address. - * @param value Ether value. -@@ -122,7 +122,6 @@ - * @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin). - * @param _nonce Transaction nonce. - * @return Transaction hash. -- */ - function getTransactionHash( - address to, - uint256 value, -@@ -135,6 +134,8 @@ - address refundReceiver, - uint256 _nonce - ) external view returns (bytes32); -+ */ -+ // MUNGED: The function was made internal to enable CVL summaries. - - /** - * External getter function for state variables. diff -druN Safe.sol Safe.sol ---- Safe.sol 2024-04-19 12:20:27.643013980 +0200 -+++ Safe.sol 2024-04-24 12:57:47.960375971 +0200 -@@ -72,22 +72,24 @@ +--- Safe.sol 2024-10-23 15:00:06 ++++ Safe.sol 2024-10-23 15:04:05 +@@ -75,22 +75,24 @@ * so we create a Safe with 0 owners and threshold 1. * This is an unusable Safe, perfect for the singleton */ @@ -77,8 +30,8 @@ diff -druN Safe.sol Safe.sol // setupOwners checks if the Threshold is already set, therefore preventing that this method is called twice setupOwners(_owners, _threshold); if (fallbackHandler != address(0)) internalSetFallbackHandler(fallbackHandler); -@@ -417,9 +419,6 @@ - return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeTxHash); +@@ -386,9 +388,6 @@ + return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, chainId, this)); } - /** @@ -87,13 +40,61 @@ diff -druN Safe.sol Safe.sol function getTransactionHash( address to, uint256 value, -@@ -431,7 +430,8 @@ +@@ -400,7 +399,9 @@ address gasToken, address refundReceiver, uint256 _nonce -- ) public view override returns (bytes32) { -+ ) internal view returns (bytes32) { +- ) public view override returns (bytes32 txHash) { ++ ) internal view returns (bytes32 txHash) { + // MUNGED: The function was made internal to enable CVL summaries. - return keccak256(encodeTransactionData(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce)); - } - } ++ + bytes32 domainHash = domainSeparator(); + + // We opted out for using assembly code here, because the way Solidity compiler we use (0.7.6) +diff -druN base/Executor.sol base/Executor.sol +--- base/Executor.sol 2024-10-18 15:20:48 ++++ base/Executor.sol 2024-10-23 15:03:22 +@@ -26,12 +26,8 @@ + uint256 txGas + ) internal returns (bool success) { + if (operation == Enum.Operation.DelegateCall) { +- /* solhint-disable no-inline-assembly */ +- /// @solidity memory-safe-assembly +- assembly { +- success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) +- } +- /* solhint-enable no-inline-assembly */ ++ // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true ++ return true; + } else { + /* solhint-disable no-inline-assembly */ + /// @solidity memory-safe-assembly +diff -druN interfaces/ISafe.sol interfaces/ISafe.sol +--- interfaces/ISafe.sol 2024-10-18 15:20:48 ++++ interfaces/ISafe.sol 2024-10-23 15:03:22 +@@ -110,7 +110,7 @@ + */ + function domainSeparator() external view returns (bytes32); + +- /** ++ /* + * @notice Returns transaction hash to be signed by owners. + * @param to Destination address. + * @param value Ether value. +@@ -123,7 +123,6 @@ + * @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin). + * @param _nonce Transaction nonce. + * @return Transaction hash. +- */ + function getTransactionHash( + address to, + uint256 value, +@@ -136,6 +135,8 @@ + address refundReceiver, + uint256 _nonce + ) external view returns (bytes32); ++ */ ++ // MUNGED: The function was made internal to enable CVL summaries. + + /** + * External getter function for state variables. diff --git a/certora/specs/Safe.spec b/certora/specs/Safe.spec index 4b15d43ea..5149a637d 100644 --- a/certora/specs/Safe.spec +++ b/certora/specs/Safe.spec @@ -4,6 +4,7 @@ methods { function nonce() external returns (uint256) envfree; function signedMessages(bytes32) external returns (uint256) envfree; function isOwner(address) external returns (bool) envfree; + function getTransactionHashPublic(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,uint256) external returns (bytes32) envfree; // harnessed function getModule(address) external returns (address) envfree; @@ -271,3 +272,25 @@ rule onlyModuleCanExecuteModuleThransactionsWithReturnData( execTransactionFromModuleReturnData@withrevert(e, to, value, data, operation); assert !lastReverted => getModule(e.msg.sender) != 0, "Only modules can execute module transactions"; } + +rule transactionHashCantCollide() { + env e; + + address to1; address to2; + uint256 value1; uint256 value2; + bytes data1; bytes data2; + Enum.Operation operation1; Enum.Operation operation2; + uint256 safeTxGas1; uint256 safeTxGas2; + uint256 baseGas1; uint256 baseGas2; + uint256 gasPrice1; uint256 gasPrice2; + address gasToken1; address gasToken2; + address refundReceiver1; address refundReceiver2; + uint256 nonce1; uint256 nonce2; + + bytes32 hash1 = getTransactionHashPublic(to1, value1, data1, operation1, safeTxGas1, baseGas1, gasPrice1, gasToken1, refundReceiver1, nonce1); + bytes32 hash2 = getTransactionHashPublic(to2, value2, data2, operation2, safeTxGas2, baseGas2, gasPrice2, gasToken2, refundReceiver2, nonce2); + + assert hash1 == hash2 => + to1 == to2 && value1 == value2 && data1 == data2 && operation1 == operation2 && safeTxGas1 == safeTxGas2 + && baseGas1 == baseGas2 && gasPrice1 == gasPrice2 && gasToken1 == gasToken2 && refundReceiver1 == refundReceiver2 && nonce1 == nonce2; +} diff --git a/contracts/Safe.sol b/contracts/Safe.sol index cdbea741b..b09630504 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -386,50 +386,6 @@ contract Safe is return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, chainId, this)); } - /** - * @notice Returns the pre-image of the transaction hash (see getTransactionHash). - * @param to Destination address. - * @param value Ether value. - * @param data Data payload. - * @param operation Operation type. - * @param safeTxGas Gas that should be used for the safe transaction. - * @param baseGas Gas costs for that are independent of the transaction execution(e.g. base transaction fee, signature check, payment of the refund) - * @param gasPrice Maximum gas price that should be used for this transaction. - * @param gasToken Token address (or 0 if ETH) that is used for the payment. - * @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin). - * @param _nonce Transaction nonce. - * @return Transaction hash bytes. - */ - function encodeTransactionData( - address to, - uint256 value, - bytes calldata data, - Enum.Operation operation, - uint256 safeTxGas, - uint256 baseGas, - uint256 gasPrice, - address gasToken, - address refundReceiver, - uint256 _nonce - ) private view returns (bytes memory) { - bytes32 safeTxStructHash = keccak256( - abi.encode( - SAFE_TX_TYPEHASH, - to, - value, - keccak256(data), - operation, - safeTxGas, - baseGas, - gasPrice, - gasToken, - refundReceiver, - _nonce - ) - ); - return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeTxStructHash); - } - /** * @inheritdoc ISafe */ @@ -444,8 +400,58 @@ contract Safe is address gasToken, address refundReceiver, uint256 _nonce - ) public view override returns (bytes32) { - return keccak256(encodeTransactionData(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce)); + ) public view override returns (bytes32 txHash) { + bytes32 domainHash = domainSeparator(); + + // We opted out for using assembly code here, because the way Solidity compiler we use (0.7.6) + // allocates memory is inefficient. We only need to allocate memory for temporary variables to be used in the keccak256 call. + /* solhint-disable no-inline-assembly */ + assembly { + // Get the free memory pointer + let ptr := mload(0x40) + + // Step 1: Hash the transaction data + // Copy transaction data to memory and hash it + calldatacopy(ptr, data.offset, data.length) + let calldataHash := keccak256(ptr, data.length) + + // Step 2: Prepare the SafeTX struct for hashing + // Layout in memory: + // ptr + 0: SAFE_TX_TYPEHASH (constant defining the struct hash) + // ptr + 32: to address + // ptr + 64: value + // ptr + 96: calldataHash + // ptr + 128: operation + // ptr + 160: safeTxGas + // ptr + 192: baseGas + // ptr + 224: gasPrice + // ptr + 256: gasToken + // ptr + 288: refundReceiver + // ptr + 320: nonce + mstore(ptr, SAFE_TX_TYPEHASH) + mstore(add(ptr, 32), to) + mstore(add(ptr, 64), value) + mstore(add(ptr, 96), calldataHash) + mstore(add(ptr, 128), operation) + mstore(add(ptr, 160), safeTxGas) + mstore(add(ptr, 192), baseGas) + mstore(add(ptr, 224), gasPrice) + mstore(add(ptr, 256), gasToken) + mstore(add(ptr, 288), refundReceiver) + mstore(add(ptr, 320), _nonce) + + // Step 3: Calculate the final EIP-712 hash + // First, hash the SafeTX struct (352 bytes total length) + mstore(add(ptr, 64), keccak256(ptr, 352)) + // Store the EIP-712 prefix (0x1901), note that integers are left-padded + // so the EIP-712 encoded data starts at add(ptr, 30) + mstore(ptr, 0x1901) + // Store the domain separator + mstore(add(ptr, 32), domainHash) + // Calculate the hash + txHash := keccak256(add(ptr, 30), 66) + } + /* solhint-enable no-inline-assembly */ } /** diff --git a/test/core/Safe.Signatures.spec.ts b/test/core/Safe.Signatures.spec.ts index c6d2d93ca..2941a3f41 100644 --- a/test/core/Safe.Signatures.spec.ts +++ b/test/core/Safe.Signatures.spec.ts @@ -2,6 +2,7 @@ import { getCompatFallbackHandler } from "./../utils/setup"; import { calculateSafeMessageHash, signHash, buildContractSignature } from "./../../src/utils/execution"; import { expect } from "chai"; import hre from "hardhat"; +import crypto from "crypto"; import { AddressZero } from "@ethersproject/constants"; import { getSafeTemplate, getSafe } from "../utils/setup"; import { @@ -46,22 +47,29 @@ describe("Safe", () => { it("should correctly calculate EIP-712 hash", async () => { const { safe } = await setupTests(); const safeAddress = await safe.getAddress(); - const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() }); - const typedDataHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); - await expect( - await safe.getTransactionHash( - tx.to, - tx.value, - tx.data, - tx.operation, - tx.safeTxGas, - tx.baseGas, - tx.gasPrice, - tx.gasToken, - tx.refundReceiver, - tx.nonce, - ), - ).to.be.eq(typedDataHash); + + for (let i = 0; i < 100; i++) { + const randomAddress = "0x" + crypto.randomBytes(20).toString("hex"); + const randomValue = "0x" + crypto.randomBytes(32).toString("hex"); + const randomData = "0x" + crypto.randomBytes(128).toString("hex"); + + const tx = buildSafeTransaction({ to: randomAddress, nonce: await safe.nonce(), value: randomValue, data: randomData }); + const typedDataHash = calculateSafeTransactionHash(safeAddress, tx, await chainId()); + await expect( + await safe.getTransactionHash( + tx.to, + tx.value, + tx.data, + tx.operation, + tx.safeTxGas, + tx.baseGas, + tx.gasPrice, + tx.gasToken, + tx.refundReceiver, + tx.nonce, + ), + ).to.be.eq(typedDataHash); + } }); });