Various changes to code clarity (Fix N-07) (OpenZeppelin#5317)

Co-authored-by: Ernesto García <[email protected]> Co-authored-by: Arr00 <[email protected]> Signed-off-by: Hadrien Croubois <[email protected]>
Amxx · Nov 30, 2024 · a2dfd87 · a2dfd87
1 parent dd5eb7f
commit a2dfd87
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 22 deletions.
diff --git a/contracts/account/utils/draft-ERC4337Utils.sol b/contracts/account/utils/draft-ERC4337Utils.sol
@@ -24,9 +24,9 @@ library ERC4337Utils {
     function parseValidationData(
         uint256 validationData
     ) internal pure returns (address aggregator, uint48 validAfter, uint48 validUntil) {
-        validAfter = uint48(bytes32(validationData).extract_32_6(0x00));
-        validUntil = uint48(bytes32(validationData).extract_32_6(0x06));
-        aggregator = address(bytes32(validationData).extract_32_20(0x0c));
+        validAfter = uint48(bytes32(validationData).extract_32_6(0));
+        validUntil = uint48(bytes32(validationData).extract_32_6(6));
+        aggregator = address(bytes32(validationData).extract_32_20(12));
         if (validUntil == 0) validUntil = type(uint48).max;
     }
 
@@ -59,7 +59,8 @@ library ERC4337Utils {
         (address aggregator1, uint48 validAfter1, uint48 validUntil1) = parseValidationData(validationData1);
         (address aggregator2, uint48 validAfter2, uint48 validUntil2) = parseValidationData(validationData2);
 
-        bool success = aggregator1 == address(0) && aggregator2 == address(0);
+        bool success = aggregator1 == address(uint160(SIG_VALIDATION_SUCCESS)) &&
+            aggregator2 == address(uint160(SIG_VALIDATION_SUCCESS));
         uint48 validAfter = uint48(Math.max(validAfter1, validAfter2));
         uint48 validUntil = uint48(Math.min(validUntil1, validUntil2));
         return packValidationData(success, validAfter, validUntil);
@@ -110,22 +111,22 @@ library ERC4337Utils {
 
     /// @dev Returns `verificationGasLimit` from the {PackedUserOperation}.
     function verificationGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.accountGasLimits.extract_32_16(0x00));
+        return uint128(self.accountGasLimits.extract_32_16(0));
     }
 
     /// @dev Returns `accountGasLimits` from the {PackedUserOperation}.
     function callGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.accountGasLimits.extract_32_16(0x10));
+        return uint128(self.accountGasLimits.extract_32_16(16));
     }
 
     /// @dev Returns the first section of `gasFees` from the {PackedUserOperation}.
     function maxPriorityFeePerGas(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.gasFees.extract_32_16(0x00));
+        return uint128(self.gasFees.extract_32_16(0));
     }
 
     /// @dev Returns the second section of `gasFees` from the {PackedUserOperation}.
     function maxFeePerGas(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.gasFees.extract_32_16(0x10));
+        return uint128(self.gasFees.extract_32_16(16));
     }
 
     /// @dev Returns the total gas price for the {PackedUserOperation} (ie. `maxFeePerGas` or `maxPriorityFeePerGas + basefee`).

diff --git a/contracts/governance/extensions/GovernorCountingOverridable.sol b/contracts/governance/extensions/GovernorCountingOverridable.sol
@@ -144,9 +144,9 @@ abstract contract GovernorCountingOverridable is GovernorVotes {
             revert GovernorAlreadyOverridenVote(account);
         }
 
-        uint256 proposalSnapshot = proposalSnapshot(proposalId);
-        uint256 overridenWeight = VotesExtended(address(token())).getPastBalanceOf(account, proposalSnapshot);
-        address delegate = VotesExtended(address(token())).getPastDelegate(account, proposalSnapshot);
+        uint256 snapshot = proposalSnapshot(proposalId);
+        uint256 overridenWeight = VotesExtended(address(token())).getPastBalanceOf(account, snapshot);
+        address delegate = VotesExtended(address(token())).getPastDelegate(account, snapshot);
         uint8 delegateCasted = proposalVote.voteReceipt[delegate].casted;
 
         proposalVote.voteReceipt[account].hasOverriden = true;

diff --git a/contracts/governance/utils/VotesExtended.sol b/contracts/governance/utils/VotesExtended.sol
@@ -34,8 +34,8 @@ abstract contract VotesExtended is Votes {
     using Checkpoints for Checkpoints.Trace160;
     using Checkpoints for Checkpoints.Trace208;
 
-    mapping(address delegator => Checkpoints.Trace160) private _delegateCheckpoints;
-    mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints;
+    mapping(address delegator => Checkpoints.Trace160) private _userDelegationCheckpoints;
+    mapping(address account => Checkpoints.Trace208) private _userVotingUnitsCheckpoints;
 
     /**
      * @dev Returns the delegate of an `account` at a specific moment in the past. If the `clock()` is
@@ -46,7 +46,7 @@ abstract contract VotesExtended is Votes {
      * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined.
      */
     function getPastDelegate(address account, uint256 timepoint) public view virtual returns (address) {
-        return address(_delegateCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint)));
+        return address(_userDelegationCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint)));
     }
 
     /**
@@ -58,25 +58,25 @@ abstract contract VotesExtended is Votes {
      * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined.
      */
     function getPastBalanceOf(address account, uint256 timepoint) public view virtual returns (uint256) {
-        return _balanceOfCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint));
+        return _userVotingUnitsCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint));
     }
 
     /// @inheritdoc Votes
     function _delegate(address account, address delegatee) internal virtual override {
         super._delegate(account, delegatee);
 
-        _delegateCheckpoints[account].push(clock(), uint160(delegatee));
+        _userDelegationCheckpoints[account].push(clock(), uint160(delegatee));
     }
 
     /// @inheritdoc Votes
     function _transferVotingUnits(address from, address to, uint256 amount) internal virtual override {
         super._transferVotingUnits(from, to, amount);
         if (from != to) {
             if (from != address(0)) {
-                _balanceOfCheckpoints[from].push(clock(), SafeCast.toUint208(_getVotingUnits(from)));
+                _userVotingUnitsCheckpoints[from].push(clock(), SafeCast.toUint208(_getVotingUnits(from)));
             }
             if (to != address(0)) {
-                _balanceOfCheckpoints[to].push(clock(), SafeCast.toUint208(_getVotingUnits(to)));
+                _userVotingUnitsCheckpoints[to].push(clock(), SafeCast.toUint208(_getVotingUnits(to)));
             }
         }
     }

diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol
@@ -227,9 +227,9 @@ library Clones {
      *   function should only be used to check addresses that are known to be clones.
      */
     function fetchCloneArgs(address instance) internal view returns (bytes memory) {
-        bytes memory result = new bytes(instance.code.length - 0x2d); // revert if length is too short
+        bytes memory result = new bytes(instance.code.length - 45); // revert if length is too short
         assembly ("memory-safe") {
-            extcodecopy(instance, add(result, 0x20), 0x2d, mload(result))
+            extcodecopy(instance, add(result, 32), 45, mload(result))
         }
         return result;
     }
@@ -248,11 +248,11 @@ library Clones {
         address implementation,
         bytes memory args
     ) private pure returns (bytes memory) {
-        if (args.length > 0x5fd3) revert CloneArgumentsTooLong();
+        if (args.length > 24531) revert CloneArgumentsTooLong();
         return
             abi.encodePacked(
                 hex"61",
-                uint16(args.length + 0x2d),
+                uint16(args.length + 45),
                 hex"3d81600a3d39f3363d3d373d3d3d363d73",
                 implementation,
                 hex"5af43d82803e903d91602b57fd5bf3",

diff --git a/contracts/utils/NoncesKeyed.sol b/contracts/utils/NoncesKeyed.sol
@@ -7,6 +7,10 @@ import {Nonces} from "./Nonces.sol";
  * @dev Alternative to {Nonces}, that supports key-ed nonces.
  *
  * Follows the https://eips.ethereum.org/EIPS/eip-4337#semi-abstracted-nonce-support[ERC-4337's semi-abstracted nonce system].
+ *
+ * NOTE: This contract inherits from {Nonces} and reuses its storage for the first nonce key (i.e. `0`). This
+ * makes upgrading from {Nonces} to {NoncesKeyed} safe when using their upgradeable versions (e.g. `NoncesKeyedUpgradeable`).
+ * Doing so will NOT reset the current state of nonces, avoiding replay attacks where a nonce is reused after the upgrade.
  */
 abstract contract NoncesKeyed is Nonces {
     mapping(address owner => mapping(uint192 key => uint64)) private _nonces;