From a2dfd87e51b4065e10bb58bbf6e3e86ab3e9f03c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 27 Nov 2024 21:25:30 +0100 Subject: [PATCH] Various changes to code clarity (Fix N-07) (#5317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Signed-off-by: Hadrien Croubois --- contracts/account/utils/draft-ERC4337Utils.sol | 17 +++++++++-------- .../extensions/GovernorCountingOverridable.sol | 6 +++--- contracts/governance/utils/VotesExtended.sol | 14 +++++++------- contracts/proxy/Clones.sol | 8 ++++---- contracts/utils/NoncesKeyed.sol | 4 ++++ 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/contracts/account/utils/draft-ERC4337Utils.sol b/contracts/account/utils/draft-ERC4337Utils.sol index 2caa3bd2112..a5eec71b4d8 100644 --- 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 index e89fb49d5af..5b89c6ddfab 100644 --- 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 index 27baaab84cf..70b0d92fb75 100644 --- 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,14 +58,14 @@ 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 @@ -73,10 +73,10 @@ abstract contract VotesExtended is Votes { 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 index 6bef4b4845e..ed8fd6fab96 100644 --- 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 index 27e0685b3a3..31cd0704e15 100644 --- 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;