From cd0299aff929fe0e6d9a9b92de37f01849fc856f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 25 Nov 2024 11:28:43 +0100 Subject: [PATCH 01/10] Various changes to code clarity (Fix N-07) --- contracts/account/utils/draft-ERC4337Utils.sol | 14 +++++++------- contracts/governance/utils/VotesExtended.sol | 14 +++++++------- contracts/proxy/Clones.sol | 8 ++++---- contracts/utils/NoncesKeyed.sol | 4 ++++ 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/contracts/account/utils/draft-ERC4337Utils.sol b/contracts/account/utils/draft-ERC4337Utils.sol index 7f72d3404e..e55d784397 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; } @@ -100,22 +100,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/utils/VotesExtended.sol b/contracts/governance/utils/VotesExtended.sol index 82e4624fb0..2c4fd438fd 100644 --- a/contracts/governance/utils/VotesExtended.sol +++ b/contracts/governance/utils/VotesExtended.sol @@ -13,8 +13,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 _userVotingWeightCheckpoints; /** * @dev Returns the delegate of an `account` at a specific moment in the past. If the `clock()` is @@ -29,7 +29,7 @@ abstract contract VotesExtended is Votes { if (timepoint >= currentTimepoint) { revert ERC5805FutureLookup(timepoint, currentTimepoint); } - return address(_delegateCheckpoints[account].upperLookupRecent(timepoint.toUint48())); + return address(_userDelegationCheckpoints[account].upperLookupRecent(timepoint.toUint48())); } /** @@ -45,14 +45,14 @@ abstract contract VotesExtended is Votes { if (timepoint >= currentTimepoint) { revert ERC5805FutureLookup(timepoint, currentTimepoint); } - return _balanceOfCheckpoints[account].upperLookupRecent(timepoint.toUint48()); + return _userVotingWeightCheckpoints[account].upperLookupRecent(timepoint.toUint48()); } /// @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 @@ -60,10 +60,10 @@ abstract contract VotesExtended is Votes { super._transferVotingUnits(from, to, amount); if (from != to) { if (from != address(0)) { - _balanceOfCheckpoints[from].push(clock(), _getVotingUnits(from).toUint208()); + _userVotingWeightCheckpoints[from].push(clock(), _getVotingUnits(from).toUint208()); } if (to != address(0)) { - _balanceOfCheckpoints[to].push(clock(), _getVotingUnits(to).toUint208()); + _userVotingWeightCheckpoints[to].push(clock(), _getVotingUnits(to).toUint208()); } } } diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 6bef4b4845..0c6852f980 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, 0x20), 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 27e0685b3a..d7bfcc27af 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 extends {Nonces}, and reuses {Nonces}'s storage for key #0. This makes upgrading from {Nonces} + * to {NoncesKeyed} safe, as such an upgrade will NOT reset the current state of nonces. Doing otherwise would enable + * replay attacks where operation that used nonces before the upgrade would be replayable after the upgrade. */ abstract contract NoncesKeyed is Nonces { mapping(address owner => mapping(uint192 key => uint64)) private _nonces; From 00a4489a36842dfc1347f9cf7f781f2cf1f34136 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Mon, 25 Nov 2024 15:49:55 -0600 Subject: [PATCH 02/10] Avoid shadowing proposalSnapshot --- .../governance/extensions/GovernorCountingOverridable.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/governance/extensions/GovernorCountingOverridable.sol b/contracts/governance/extensions/GovernorCountingOverridable.sol index 2ab669fa00..fa25e3f53e 100644 --- a/contracts/governance/extensions/GovernorCountingOverridable.sol +++ b/contracts/governance/extensions/GovernorCountingOverridable.sol @@ -141,9 +141,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; From bf2d786700cb857d114dc635f10dde6851dc7fb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 26 Nov 2024 11:48:53 -0600 Subject: [PATCH 03/10] Update contracts/utils/NoncesKeyed.sol --- contracts/utils/NoncesKeyed.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/utils/NoncesKeyed.sol b/contracts/utils/NoncesKeyed.sol index d7bfcc27af..4d030f72df 100644 --- a/contracts/utils/NoncesKeyed.sol +++ b/contracts/utils/NoncesKeyed.sol @@ -8,9 +8,9 @@ import {Nonces} from "./Nonces.sol"; * * Follows the https://eips.ethereum.org/EIPS/eip-4337#semi-abstracted-nonce-support[ERC-4337's semi-abstracted nonce system]. * - * Note: This contract extends {Nonces}, and reuses {Nonces}'s storage for key #0. This makes upgrading from {Nonces} - * to {NoncesKeyed} safe, as such an upgrade will NOT reset the current state of nonces. Doing otherwise would enable - * replay attacks where operation that used nonces before the upgrade would be replayable after the upgrade. + * 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. Doing so will NOT reset the current state of nonces to + * avoid replay attacks where a nonce is reused after the upgrade. */ abstract contract NoncesKeyed is Nonces { mapping(address owner => mapping(uint192 key => uint64)) private _nonces; From 01c38d2ac49c15db48322e3d7c8c1a6fd967e474 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 26 Nov 2024 17:28:42 -0600 Subject: [PATCH 04/10] Implement audit feedback --- contracts/proxy/Clones.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 0c6852f980..ed8fd6fab9 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -229,7 +229,7 @@ library Clones { function fetchCloneArgs(address instance) internal view returns (bytes memory) { bytes memory result = new bytes(instance.code.length - 45); // revert if length is too short assembly ("memory-safe") { - extcodecopy(instance, add(result, 0x20), 45, mload(result)) + extcodecopy(instance, add(result, 32), 45, mload(result)) } return result; } From 20adb7239839a579ba7f8a5a15b7dbd629759629 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 26 Nov 2024 17:38:13 -0600 Subject: [PATCH 05/10] Mention upgradeable version in Natspec note --- contracts/utils/NoncesKeyed.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/NoncesKeyed.sol b/contracts/utils/NoncesKeyed.sol index 4d030f72df..d2f29b98cc 100644 --- a/contracts/utils/NoncesKeyed.sol +++ b/contracts/utils/NoncesKeyed.sol @@ -9,8 +9,8 @@ import {Nonces} from "./Nonces.sol"; * 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. Doing so will NOT reset the current state of nonces to - * avoid replay attacks where a nonce is reused after the upgrade. + * 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 to avoid replay attacks where a nonce is reused after the upgrade. */ abstract contract NoncesKeyed is Nonces { mapping(address owner => mapping(uint192 key => uint64)) private _nonces; From 9f70de36d0721f0c77662ddeac841755f40b021e Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 26 Nov 2024 17:38:49 -0600 Subject: [PATCH 06/10] lint --- contracts/utils/NoncesKeyed.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/NoncesKeyed.sol b/contracts/utils/NoncesKeyed.sol index d2f29b98cc..1c351faee6 100644 --- a/contracts/utils/NoncesKeyed.sol +++ b/contracts/utils/NoncesKeyed.sol @@ -9,7 +9,7 @@ import {Nonces} from "./Nonces.sol"; * 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`). + * 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 to avoid replay attacks where a nonce is reused after the upgrade. */ abstract contract NoncesKeyed is Nonces { From 905467d965d9f937933eb25c6f569fbaf6797584 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Wed, 27 Nov 2024 11:53:12 -0500 Subject: [PATCH 07/10] Rename `_userVotingWeightCheckpoints` to `_userVotingUnitsCheckpoints` --- contracts/governance/utils/VotesExtended.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/utils/VotesExtended.sol b/contracts/governance/utils/VotesExtended.sol index b989a8fce5..e013ceda01 100644 --- a/contracts/governance/utils/VotesExtended.sol +++ b/contracts/governance/utils/VotesExtended.sol @@ -35,7 +35,7 @@ abstract contract VotesExtended is Votes { using Checkpoints for Checkpoints.Trace208; mapping(address delegator => Checkpoints.Trace160) private _userDelegationCheckpoints; - mapping(address account => Checkpoints.Trace208) private _userVotingWeightCheckpoints; + 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 From c075155ea00e71a463f938c5b14e23d8a0937131 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Wed, 27 Nov 2024 11:59:01 -0500 Subject: [PATCH 08/10] finish rename of `_userVotingWeightCheckpoints` --- contracts/governance/utils/VotesExtended.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/governance/utils/VotesExtended.sol b/contracts/governance/utils/VotesExtended.sol index e013ceda01..70b0d92fb7 100644 --- a/contracts/governance/utils/VotesExtended.sol +++ b/contracts/governance/utils/VotesExtended.sol @@ -58,7 +58,7 @@ 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 _userVotingWeightCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint)); + return _userVotingUnitsCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint)); } /// @inheritdoc Votes @@ -73,10 +73,10 @@ abstract contract VotesExtended is Votes { super._transferVotingUnits(from, to, amount); if (from != to) { if (from != address(0)) { - _userVotingWeightCheckpoints[from].push(clock(), SafeCast.toUint208(_getVotingUnits(from))); + _userVotingUnitsCheckpoints[from].push(clock(), SafeCast.toUint208(_getVotingUnits(from))); } if (to != address(0)) { - _userVotingWeightCheckpoints[to].push(clock(), SafeCast.toUint208(_getVotingUnits(to))); + _userVotingUnitsCheckpoints[to].push(clock(), SafeCast.toUint208(_getVotingUnits(to))); } } } From a65ee06f1885026c59e1fa59b9c2dfafb152f8f3 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:59:56 -0500 Subject: [PATCH 09/10] update sentence structure --- contracts/utils/NoncesKeyed.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/NoncesKeyed.sol b/contracts/utils/NoncesKeyed.sol index 1c351faee6..31cd0704e1 100644 --- a/contracts/utils/NoncesKeyed.sol +++ b/contracts/utils/NoncesKeyed.sol @@ -10,7 +10,7 @@ import {Nonces} from "./Nonces.sol"; * * 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 to avoid replay attacks where a nonce is reused after the upgrade. + * 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; From 7a3707f0f1cc7dbf4b1ed0d9411642739cd0638f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 27 Nov 2024 14:24:48 -0600 Subject: [PATCH 10/10] Use SIG_VALIDATION_SUCCESS in combineValidationData --- contracts/account/utils/draft-ERC4337Utils.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/account/utils/draft-ERC4337Utils.sol b/contracts/account/utils/draft-ERC4337Utils.sol index d1bf3c9b05..a5eec71b4d 100644 --- a/contracts/account/utils/draft-ERC4337Utils.sol +++ b/contracts/account/utils/draft-ERC4337Utils.sol @@ -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);