optimize getTransactionHash by implementing it in assembly (#847)

mmv08 · web-flow · commit b55fd8f3bd78 · 2024-10-28T15:05:00.000+01:00
This pull request includes significant changes to the `Safe` contract
and its associated test suite. The changes focus on optimizing the
encoding of transaction data and enhancing the test coverage for
transaction hash calculations.

### Optimizations in `Safe` contract:

*
[`contracts/Safe.sol`](diffhunk://#diff-587b494ea631bb6b7adf4fc3e1a2e6a277a385ff16e1163b26e39de24e9483deL414-R467):
Rewrote the transaction data encoding logic in assembly to avoid
multiple memory allocations, improving gas efficiency.

### Enhancements in test suite:

*
[`test/core/Safe.Signatures.spec.ts`](diffhunk://#diff-d7bc3771858069f85022d38344b6cb5302146da4bafde1ac18910e3d7bfac43bL49-R56):
Enhanced the test case for calculating EIP-712 hash by introducing a
loop to generate and test multiple random transactions.
[[1]](diffhunk://#diff-d7bc3771858069f85022d38344b6cb5302146da4bafde1ac18910e3d7bfac43bL49-R56)
[[2]](diffhunk://#diff-d7bc3771858069f85022d38344b6cb5302146da4bafde1ac18910e3d7bfac43bR72).
The previous test case was inefficient as it contained empty safe
transaction data. The test would still pass if you forgot to include it
in hashing.
* I also added a FV rule to verify hash computation correctness.

### Benchmarks

#### Before
```
  ERC20 - transfer
           Used 51800n gas for &gt;transfer&lt;
    ✔ with an EOA (137ms)
           Used 82980n gas for &gt;transfer&lt;
    ✔ with a single owner Safe
           Used 88874n gas for &gt;transfer&lt;
    ✔ with a single owner and guard Safe
           Used 90024n gas for &gt;transfer&lt;
    ✔ with a 2 out of 2 Safe
           Used 97094n gas for &gt;transfer&lt;
    ✔ with a 3 out of 3 Safe
           Used 97094n gas for &gt;transfer&lt;
    ✔ with a 3 out of 5 Safe
```    

#### After

```
ERC20 - transfer
           Used 51800n gas for &gt;transfer&lt;
    ✔ with an EOA (71ms)
           Used 82494n gas for &gt;transfer&lt;
    ✔ with a single owner Safe
           Used 88375n gas for &gt;transfer&lt;
    ✔ with a single owner and guard Safe
           Used 89547n gas for &gt;transfer&lt;
    ✔ with a 2 out of 2 Safe
           Used 96577n gas for &gt;transfer&lt;
    ✔ with a 3 out of 3 Safe
           Used 96589n gas for &gt;transfer&lt;
    ✔ with a 3 out of 5 Safe
```

On average, it saves ~485 gas, not much, but considering this is the
hottest path, it should result in significant accumulated savings.
(After 44 Safe transactions, a user would save 21k gas - enough for
broadcasting a native token transfer)

### Codesize

It saves 273 bytes in code size.

#### Before
SafeL2 22582 bytes (limit is 24576)

#### After

SafeL2 22309 bytes (limit is 24576)
diff --git 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
@@ -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
@@ -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
@@ -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);
+            }
         });
     });
 
