Skip to content

Commit

Permalink
Add a warning comment about potential dirty bits in `getTransactionHa…
Browse files Browse the repository at this point in the history
…sh` (#872)

This commit adds a detailed warning in the `Safe.sol` contract regarding
potential dirty bits in assembly code for types smaller than 256 bits.
It emphasizes the importance of considering this for future changes
while explaining the rationale behind using assembly for memory
efficiency.

No functional changes were made to the contract logic. This update aims
to improve code clarity and maintainability.

Originally reported by @jhoenicke
  • Loading branch information
mmv08 authored Dec 18, 2024
1 parent 489b2bc commit 4309776
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions contracts/Safe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,11 @@ contract Safe is

// We opted for using assembly code here, because the way Solidity compiler we use (0.7.6) allocates memory is
// inefficient. We do not need to allocate memory for temporary variables to be used in the keccak256 call.
//
// WARNING: We do not clean potential dirty bits in types that are less than 256 bits (addresses and Enum.Operation)
// The solidity assembly types that are smaller than 256 bit can have dirty high bits according to the spec (see the Warning in https://docs.soliditylang.org/en/latest/assembly.html#access-to-external-variables-functions-and-libraries).
// However, we read most of the data from calldata, where the variables are not packed, and the only variable we read from storage is uint256 nonce.
// This is not a problem, however, we must consider this for potential future changes.
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
Expand Down

0 comments on commit 4309776

Please sign in to comment.