fix: use dataGas when calculating message hash of <1.0.0

[iamacook]

What it solves

Resolves incorrect message hash for <1.0.0 Safes

How this PR fixes it

For Safe versions >=1.0.0, the SafeTx typehash uses baseGas:

//keccak256(
//    "SafeTx(address to,uint256 value,bytes data,uint8 operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)"
//);
bytes32 public constant SAFE_TX_TYPEHASH = 0xbb8310d486368db6bd6f849402fdd73ad53d316b5a4b2644ad6efe0f941286d8;

For Safe versions <1.0.0, the SafeTx typehas uses dataGas:

//keccak256(
//    "SafeTx(address to,uint256 value,bytes data,uint8 operation,uint256 safeTxGas,uint256 dataGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)"
//);
bytes32 public constant SAFE_TX_TYPEHASH = 0x14d461bc7412367e924637b363c7bf29b8f47e2f84869f4426e5633d8af47b20;

When calculating the message hash, we do not take the contract version into account, calculating based on the newer SafeTx typehash.

This conditionally adjusted baseGas to dataGas when calculating the message hash for the current Safe, therfore correcting it for <1.0.0 versions.

How to test it

Cross-reference message hash match with the following script for <1.0.0 Safes.

Note: an example 0.1.0 Safe is eth:0xecd11858a4bcc35A51084Ebe672beaCe01142fcA

Checklist

• I've tested the branch on mobile 📱
• I've documented how it affects the analytics (if at all) 📊
• I've written a unit/e2e test for it (if applicable) 🧑‍💻

[github-actions]

Branch preview

✅ Deploy successful!

Website:
https://old_safetx_hash--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://old_safetx_hash--walletweb.review.5afe.dev/storybook/

[github-actions]

Code review by ChatGPT

[github-actions]

1. Consistency in TypeHash Constants: Consider organizing the constants (OLD_SAFE_TX_TYPEHASH and NEW_SAFE_TX_TYPEHASH) similarly to improve readability and maintainability. Grouping them together with their respective contexts (like versions) can enhance the logical flow.

2. Comment Clarification: The inline comment // <1.0.0 is dataGas in the test case might lead to confusion. Consider rephrasing it for clarity, such as // For versions <1.0.0, it's dataGas to clearly relate it to handling within the test case.

3. DRY Principle: The encoding parameter list in getMessageHash test has repetition. Consider moving the parameter arrays ('address', 'uint256', etc., and SafeTx.to, SafeTx.value, etc.) to separate reusable constants or functions to reduce redundancy and improve clarity.

4. Type Safety: Instead of using as const, defining a type or enum for versions could improve type safety and clarity, making the code more robust to potential version mismatches.

5. Functionality Clarification: The test setup for SafeTx includes a version switch between dataGas and baseGas. Consider clarifying the handling in getMessageHash, potentially through comments or structuring the code to highlight this distinction, to avoid confusion during future modifications.

Overall, the changes generally follow good practices, but addressing these points could improve code maintenance and readability.

[github-actions]

1. Function Naming and Consistency:

   • In the getMessageHash function, consider using consistent naming for variables and functions. For example, tx is used as a shorthand which could be improved by renaming to transaction for clarity.

2. Type Safety:

   • The tx variable is typed as any. If possible, provide a more specific TypeScript type instead of using any to enhance type safety and catch potential errors during compilation.

3. Direct Modification Avoidance:

   • In getMessageHash, there is direct mutation of the SafeTx array element: SafeTx[5].name = 'dataGas'. This could lead to potential side effects if SafeTx is used elsewhere. Consider creating a modified copy instead, adhering to immutability principles.

4. Potential Logic Concerns:

   • Ensure that SafeTx[5] exists before attempting to access or modify it to avoid potential runtime errors.

5. Semantic Version Utility:

   • The line const usesBaseGas = semverSatisfies(safeVersion, NEW_SAFE_TX_TYPE_HASH_VERSION) assumes that semverSatisfies is a valid utility function. Verify the correctness of this approach and ensure it is imported or defined as expected.

6. Conditional Logic Simplification:

   • Review the condition if (!usesBaseGas) logic. If more such conditions are expected, consider refactoring by using helper functions or extracting condition checks to be more descriptive.

7. React Component Logic Separation:

   • Within SafeTxHashDataRow, try to minimize logic by further separating complex logic outside of the main render function.

8. Use of Dependencies:

   • Ensure all necessary imports for utilities like TypedDataEncoder and any other dependencies are correctly imported and up-to-date.

No significant issues that require more detailed refactoring suggestions were found.

[github-actions]

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
global	1016.58 KB (🟡 +98 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	73.92% (+0.02% 🔼)	14410/19493
🔴	Branches	51.47% (+0.02% 🔼)	3439/6682
🔴	Functions	56.87% (+0.01% 🔼)	2045/3596
🟡	Lines	75.46% (+0.02% 🔼)	13070/17321

Test suite run success

1718 tests passing in 232 suites.

Report generated by 🧪jest coverage report action from 10d9e24