From e5e9ff72f0193d4b84bc8759595de3599b7aa3d9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Dec 2024 17:37:13 +0100 Subject: [PATCH] Release v5.2 audit fixes (#5330) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Hadrien Croubois Co-authored-by: Sam Bugs <101145325+0xsambugs@users.noreply.github.com> Co-authored-by: Ernesto García Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: wizard <112275929+famouswizard@users.noreply.github.com> Co-authored-by: leopardracer <136604165+leopardracer@users.noreply.github.com> Co-authored-by: cairo --- .changeset/proud-planes-arrive.md | 2 +- GUIDELINES.md | 2 +- .../account/utils/draft-ERC4337Utils.sol | 57 ++++++---- .../account/utils/draft-ERC7579Utils.sol | 20 ++-- contracts/finance/VestingWallet.sol | 5 + .../GovernorCountingOverridable.sol | 32 ++++-- contracts/governance/utils/Votes.sol | 21 ++-- contracts/governance/utils/VotesExtended.sol | 47 +++++--- contracts/interfaces/draft-IERC4337.sol | 51 +++++++-- contracts/interfaces/draft-IERC7579.sol | 29 +++++ ...sol => AccessControlNonRevokableAdmin.sol} | 0 contracts/proxy/Clones.sol | 14 +-- contracts/token/ERC20/extensions/ERC1363.sol | 7 +- contracts/token/ERC20/utils/SafeERC20.sol | 1 - contracts/utils/Bytes.sol | 12 +-- contracts/utils/CAIP10.sol | 10 +- contracts/utils/CAIP2.sol | 7 +- contracts/utils/NoncesKeyed.sol | 43 +++++--- contracts/utils/Strings.sol | 57 ++++++++-- test/account/utils/draft-ERC4337Utils.test.js | 101 +++++++++++++++--- test/account/utils/draft-ERC7579Utils.test.js | 5 +- ...ckpoints.test.js => VotesExtended.test.js} | 0 test/helpers/erc4337.js | 32 ++++-- test/utils/Nonces.behavior.js | 49 +++++++-- test/utils/Strings.t.sol | 23 ++++ test/utils/Strings.test.js | 15 +++ 26 files changed, 490 insertions(+), 152 deletions(-) rename contracts/mocks/docs/access-control/{AccessControlUnrevokableAdmin.sol => AccessControlNonRevokableAdmin.sol} (100%) rename test/governance/utils/{VotesAdditionalCheckpoints.test.js => VotesExtended.test.js} (100%) diff --git a/.changeset/proud-planes-arrive.md b/.changeset/proud-planes-arrive.md index 6408976414d..60c831bd690 100644 --- a/.changeset/proud-planes-arrive.md +++ b/.changeset/proud-planes-arrive.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Bytes`: Add a library of common operation that operate on `bytes` objects. +`Bytes`: Add a library of common operations that operate on `bytes` objects. diff --git a/GUIDELINES.md b/GUIDELINES.md index a1418d6528a..2c21e956b1e 100644 --- a/GUIDELINES.md +++ b/GUIDELINES.md @@ -55,7 +55,7 @@ External contributions must be reviewed separately by multiple maintainers. Automation should be used as much as possible to reduce the possibility of human error and forgetfulness. -Automations that make use of sensitive credentials must use secure secret management, and must be strengthened against attacks such as [those on GitHub Actions worklows](https://github.com/nikitastupin/pwnhub). +Automations that make use of sensitive credentials must use secure secret management, and must be strengthened against attacks such as [those on GitHub Actions workflows](https://github.com/nikitastupin/pwnhub). Some other examples of automation are: diff --git a/contracts/account/utils/draft-ERC4337Utils.sol b/contracts/account/utils/draft-ERC4337Utils.sol index bf559b86d6e..a5eec71b4d8 100644 --- a/contracts/account/utils/draft-ERC4337Utils.sol +++ b/contracts/account/utils/draft-ERC4337Utils.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; -import {IEntryPoint, PackedUserOperation} from "../../interfaces/draft-IERC4337.sol"; +import {PackedUserOperation} from "../../interfaces/draft-IERC4337.sol"; import {Math} from "../../utils/math/Math.sol"; import {Packing} from "../../utils/Packing.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); @@ -71,12 +72,7 @@ library ERC4337Utils { return (aggregator_, block.timestamp < validAfter || validUntil < block.timestamp); } - /// @dev Computes the hash of a user operation with the current entrypoint and chainid. - function hash(PackedUserOperation calldata self) internal view returns (bytes32) { - return hash(self, address(this), block.chainid); - } - - /// @dev Sames as {hash}, but with a custom entrypoint and chainid. + /// @dev Computes the hash of a user operation for a given entrypoint and chainid. function hash( PackedUserOperation calldata self, address entrypoint, @@ -103,24 +99,34 @@ library ERC4337Utils { return result; } + /// @dev Returns `factory` from the {PackedUserOperation}, or address(0) if the initCode is empty or not properly formatted. + function factory(PackedUserOperation calldata self) internal pure returns (address) { + return self.initCode.length < 20 ? address(0) : address(bytes20(self.initCode[0:20])); + } + + /// @dev Returns `factoryData` from the {PackedUserOperation}, or empty bytes if the initCode is empty or not properly formatted. + function factoryData(PackedUserOperation calldata self) internal pure returns (bytes calldata) { + return self.initCode.length < 20 ? _emptyCalldataBytes() : self.initCode[20:]; + } + /// @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`). @@ -129,22 +135,35 @@ library ERC4337Utils { // Following values are "per gas" uint256 maxPriorityFee = maxPriorityFeePerGas(self); uint256 maxFee = maxFeePerGas(self); - return Math.ternary(maxFee == maxPriorityFee, maxFee, Math.min(maxFee, maxPriorityFee + block.basefee)); + return Math.min(maxFee, maxPriorityFee + block.basefee); } } /// @dev Returns the first section of `paymasterAndData` from the {PackedUserOperation}. function paymaster(PackedUserOperation calldata self) internal pure returns (address) { - return address(bytes20(self.paymasterAndData[0:20])); + return self.paymasterAndData.length < 52 ? address(0) : address(bytes20(self.paymasterAndData[0:20])); } /// @dev Returns the second section of `paymasterAndData` from the {PackedUserOperation}. function paymasterVerificationGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) { - return uint128(bytes16(self.paymasterAndData[20:36])); + return self.paymasterAndData.length < 52 ? 0 : uint128(bytes16(self.paymasterAndData[20:36])); } /// @dev Returns the third section of `paymasterAndData` from the {PackedUserOperation}. function paymasterPostOpGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) { - return uint128(bytes16(self.paymasterAndData[36:52])); + return self.paymasterAndData.length < 52 ? 0 : uint128(bytes16(self.paymasterAndData[36:52])); + } + + /// @dev Returns the forth section of `paymasterAndData` from the {PackedUserOperation}. + function paymasterData(PackedUserOperation calldata self) internal pure returns (bytes calldata) { + return self.paymasterAndData.length < 52 ? _emptyCalldataBytes() : self.paymasterAndData[52:]; + } + + // slither-disable-next-line write-after-write + function _emptyCalldataBytes() private pure returns (bytes calldata result) { + assembly ("memory-safe") { + result.offset := 0 + result.length := 0 + } } } diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index de6bb6509ec..dab18f26bf0 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -22,22 +22,26 @@ library ERC7579Utils { using Packing for *; /// @dev A single `call` execution. - CallType constant CALLTYPE_SINGLE = CallType.wrap(0x00); + CallType internal constant CALLTYPE_SINGLE = CallType.wrap(0x00); /// @dev A batch of `call` executions. - CallType constant CALLTYPE_BATCH = CallType.wrap(0x01); + CallType internal constant CALLTYPE_BATCH = CallType.wrap(0x01); /// @dev A `delegatecall` execution. - CallType constant CALLTYPE_DELEGATECALL = CallType.wrap(0xFF); + CallType internal constant CALLTYPE_DELEGATECALL = CallType.wrap(0xFF); /// @dev Default execution type that reverts on failure. - ExecType constant EXECTYPE_DEFAULT = ExecType.wrap(0x00); + ExecType internal constant EXECTYPE_DEFAULT = ExecType.wrap(0x00); /// @dev Execution type that does not revert on failure. - ExecType constant EXECTYPE_TRY = ExecType.wrap(0x01); - - /// @dev Emits when an {EXECTYPE_TRY} execution fails. - event ERC7579TryExecuteFail(uint256 batchExecutionIndex, bytes result); + ExecType internal constant EXECTYPE_TRY = ExecType.wrap(0x01); + + /** + * @dev Emits when an {EXECTYPE_TRY} execution fails. + * @param batchExecutionIndex The index of the failed call in the execution batch. + * @param returndata The returned data from the failed call. + */ + event ERC7579TryExecuteFail(uint256 batchExecutionIndex, bytes returndata); /// @dev The provided {CallType} is not supported. error ERC7579UnsupportedCallType(CallType callType); diff --git a/contracts/finance/VestingWallet.sol b/contracts/finance/VestingWallet.sol index 0e0321d0da2..b03c4968430 100644 --- a/contracts/finance/VestingWallet.sol +++ b/contracts/finance/VestingWallet.sol @@ -26,6 +26,11 @@ import {Ownable} from "../access/Ownable.sol"; * * NOTE: When using this contract with any token whose balance is adjusted automatically (i.e. a rebase token), make * sure to account the supply/balance adjustment in the vesting schedule to ensure the vested amount is as intended. + * + * NOTE: Chains with support for native ERC20s may allow the vesting wallet to withdraw the underlying asset as both an + * ERC20 and as native currency. For example, if chain C supports token A and the wallet gets deposited 100 A, then + * at 50% of the vesting period, the beneficiary can withdraw 50 A as ERC20 and 25 A as native currency (totaling 75 A). + * Consider disabling one of the withdrawal methods. */ contract VestingWallet is Context, Ownable { event EtherReleased(uint256 amount); diff --git a/contracts/governance/extensions/GovernorCountingOverridable.sol b/contracts/governance/extensions/GovernorCountingOverridable.sol index 9b46903e35d..db375a93f47 100644 --- a/contracts/governance/extensions/GovernorCountingOverridable.sol +++ b/contracts/governance/extensions/GovernorCountingOverridable.sol @@ -8,8 +8,8 @@ import {VotesExtended} from "../utils/VotesExtended.sol"; import {GovernorVotes} from "./GovernorVotes.sol"; /** - * @dev Extension of {Governor} which enables delegatees to override the vote of their delegates. This module requires a - * token token that inherits `VotesExtended`. + * @dev Extension of {Governor} which enables delegators to override the vote of their delegates. This module requires a + * token that inherits {VotesExtended}. */ abstract contract GovernorCountingOverridable is GovernorVotes { bytes32 public constant OVERRIDE_BALLOT_TYPEHASH = @@ -35,7 +35,10 @@ abstract contract GovernorCountingOverridable is GovernorVotes { mapping(address voter => VoteReceipt) voteReceipt; } - event VoteReduced(address indexed voter, uint256 proposalId, uint8 support, uint256 weight); + /// @dev The votes casted by `delegate` were reduced by `weight` after an override vote was casted by the original token holder + event VoteReduced(address indexed delegate, uint256 proposalId, uint8 support, uint256 weight); + + /// @dev A delegated vote on `proposalId` was overridden by `weight` event OverrideVoteCast(address indexed voter, uint256 proposalId, uint8 support, uint256 weight, string reason); error GovernorAlreadyOverridenVote(address account); @@ -52,6 +55,11 @@ abstract contract GovernorCountingOverridable is GovernorVotes { /** * @dev See {IGovernor-hasVoted}. + * + * NOTE: Calling {castVote} (or similar) casts a vote using the voting power that is delegated to the voter. + * Conversely, calling {castOverrideVote} (or similar) uses the voting power of the account itself, from its asset + * balances. Casting an "override vote" does not count as voting and won't be reflected by this getter. Consider + * using {hasVotedOverride} to check if an account has casted an "override vote" for a given proposal id. */ function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { return _proposalVotes[proposalId].voteReceipt[account].casted != 0; @@ -120,7 +128,11 @@ abstract contract GovernorCountingOverridable is GovernorVotes { return totalWeight; } - /// @dev Variant of {Governor-_countVote} that deals with vote overrides. + /** + * @dev Variant of {Governor-_countVote} that deals with vote overrides. + * + * NOTE: See {hasVoted} for more details about the difference between {castVote} and {castOverrideVote}. + */ function _countOverride(uint256 proposalId, address account, uint8 support) internal virtual returns (uint256) { ProposalVote storage proposalVote = _proposalVotes[proposalId]; @@ -132,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; @@ -150,7 +162,7 @@ abstract contract GovernorCountingOverridable is GovernorVotes { return overridenWeight; } - /// @dev variant of {Governor-_castVote} that deals with vote overrides. + /// @dev Variant of {Governor-_castVote} that deals with vote overrides. Returns the overridden weight. function _castOverride( uint256 proposalId, address account, @@ -168,7 +180,7 @@ abstract contract GovernorCountingOverridable is GovernorVotes { return overridenWeight; } - /// @dev Public function for casting an override vote + /// @dev Public function for casting an override vote. Returns the overridden weight. function castOverrideVote( uint256 proposalId, uint8 support, @@ -178,7 +190,7 @@ abstract contract GovernorCountingOverridable is GovernorVotes { return _castOverride(proposalId, voter, support, reason); } - /// @dev Public function for casting an override vote using a voter's signature + /// @dev Public function for casting an override vote using a voter's signature. Returns the overridden weight. function castOverrideVoteBySig( uint256 proposalId, uint8 support, diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index bbbc2264ff9..976579719de 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -71,6 +71,15 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { return "mode=blocknumber&from=default"; } + /** + * @dev Validate that a timepoint is in the past, and return it as a uint48. + */ + function _validateTimepoint(uint256 timepoint) internal view returns (uint48) { + uint48 currentTimepoint = clock(); + if (timepoint >= currentTimepoint) revert ERC5805FutureLookup(timepoint, currentTimepoint); + return SafeCast.toUint48(timepoint); + } + /** * @dev Returns the current amount of votes that `account` has. */ @@ -87,11 +96,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined. */ function getPastVotes(address account, uint256 timepoint) public view virtual returns (uint256) { - uint48 currentTimepoint = clock(); - if (timepoint >= currentTimepoint) { - revert ERC5805FutureLookup(timepoint, currentTimepoint); - } - return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); + return _delegateCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint)); } /** @@ -107,11 +112,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined. */ function getPastTotalSupply(uint256 timepoint) public view virtual returns (uint256) { - uint48 currentTimepoint = clock(); - if (timepoint >= currentTimepoint) { - revert ERC5805FutureLookup(timepoint, currentTimepoint); - } - return _totalCheckpoints.upperLookupRecent(SafeCast.toUint48(timepoint)); + return _totalCheckpoints.upperLookupRecent(_validateTimepoint(timepoint)); } /** diff --git a/contracts/governance/utils/VotesExtended.sol b/contracts/governance/utils/VotesExtended.sol index 62ddd5f7a2e..70b0d92fb75 100644 --- a/contracts/governance/utils/VotesExtended.sol +++ b/contracts/governance/utils/VotesExtended.sol @@ -6,15 +6,36 @@ import {Votes} from "./Votes.sol"; import {SafeCast} from "../../utils/math/SafeCast.sol"; /** - * @dev Extension of {Votes} that adds exposes checkpoints for delegations and balances. + * @dev Extension of {Votes} that adds checkpoints for delegations and balances. + * + * WARNING: While this contract extends {Votes}, valid uses of {Votes} may not be compatible with + * {VotesExtended} without additional considerations. This implementation of {_transferVotingUnits} must + * run AFTER the voting weight movement is registered, such that it is reflected on {_getVotingUnits}. + * + * Said differently, {VotesExtended} MUST be integrated in a way that calls {_transferVotingUnits} AFTER the + * asset transfer is registered and balances are updated: + * + * ```solidity + * contract VotingToken is Token, VotesExtended { + * function transfer(address from, address to, uint256 tokenId) public override { + * super.transfer(from, to, tokenId); // <- Perform the transfer first ... + * _transferVotingUnits(from, to, 1); // <- ... then call _transferVotingUnits. + * } + * + * function _getVotingUnits(address account) internal view override returns (uint256) { + * return balanceOf(account); + * } + * } + * ``` + * + * {ERC20Votes} and {ERC721Votes} follow this pattern and are thus safe to use with {VotesExtended}. */ abstract contract VotesExtended is Votes { - using SafeCast for uint256; using Checkpoints for Checkpoints.Trace160; using Checkpoints for Checkpoints.Trace208; - mapping(address delegatee => 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 @@ -25,11 +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) { - uint48 currentTimepoint = clock(); - if (timepoint >= currentTimepoint) { - revert ERC5805FutureLookup(timepoint, currentTimepoint); - } - return address(_delegateCheckpoints[account].upperLookupRecent(timepoint.toUint48())); + return address(_userDelegationCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint))); } /** @@ -41,18 +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) { - uint48 currentTimepoint = clock(); - if (timepoint >= currentTimepoint) { - revert ERC5805FutureLookup(timepoint, currentTimepoint); - } - return _balanceOfCheckpoints[account].upperLookupRecent(timepoint.toUint48()); + 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 @@ -60,10 +73,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()); + _userVotingUnitsCheckpoints[from].push(clock(), SafeCast.toUint208(_getVotingUnits(from))); } if (to != address(0)) { - _balanceOfCheckpoints[to].push(clock(), _getVotingUnits(to).toUint208()); + _userVotingUnitsCheckpoints[to].push(clock(), SafeCast.toUint208(_getVotingUnits(to))); } } } diff --git a/contracts/interfaces/draft-IERC4337.sol b/contracts/interfaces/draft-IERC4337.sol index 9b1af56b6e5..67ab146c053 100644 --- a/contracts/interfaces/draft-IERC4337.sol +++ b/contracts/interfaces/draft-IERC4337.sol @@ -11,7 +11,7 @@ pragma solidity ^0.8.20; * - `callData` (`bytes`): The data to pass to the sender during the main execution call * - `callGasLimit` (`uint256`): The amount of gas to allocate the main execution call * - `verificationGasLimit` (`uint256`): The amount of gas to allocate for the verification step - * - `preVerificationGas` (`uint256`): Extra gas to pay the bunder + * - `preVerificationGas` (`uint256`): Extra gas to pay the bundler * - `maxFeePerGas` (`uint256`): Maximum fee per gas (similar to EIP-1559 max_fee_per_gas) * - `maxPriorityFeePerGas` (`uint256`): Maximum priority fee per gas (similar to EIP-1559 max_priority_fee_per_gas) * - `paymaster` (`address`): Address of paymaster contract, (or empty, if account pays for itself) @@ -27,7 +27,7 @@ pragma solidity ^0.8.20; * - `callData` (`bytes`) * - `accountGasLimits` (`bytes32`): concatenation of verificationGas (16 bytes) and callGas (16 bytes) * - `preVerificationGas` (`uint256`) - * - `gasFees` (`bytes32`): concatenation of maxPriorityFee (16 bytes) and maxFeePerGas (16 bytes) + * - `gasFees` (`bytes32`): concatenation of maxPriorityFeePerGas (16 bytes) and maxFeePerGas (16 bytes) * - `paymasterAndData` (`bytes`): concatenation of paymaster fields (or empty) * - `signature` (`bytes`) */ @@ -38,17 +38,25 @@ struct PackedUserOperation { bytes callData; bytes32 accountGasLimits; // `abi.encodePacked(verificationGasLimit, callGasLimit)` 16 bytes each uint256 preVerificationGas; - bytes32 gasFees; // `abi.encodePacked(maxPriorityFee, maxFeePerGas)` 16 bytes each - bytes paymasterAndData; // `abi.encodePacked(paymaster, paymasterVerificationGasLimit, paymasterPostOpGasLimit, paymasterData)` + bytes32 gasFees; // `abi.encodePacked(maxPriorityFeePerGas, maxFeePerGas)` 16 bytes each + bytes paymasterAndData; // `abi.encodePacked(paymaster, paymasterVerificationGasLimit, paymasterPostOpGasLimit, paymasterData)` (20 bytes, 16 bytes, 16 bytes, dynamic) bytes signature; } /** * @dev Aggregates and validates multiple signatures for a batch of user operations. + * + * A contract could implement this interface with custom validation schemes that allow signature aggregation, + * enabling significant optimizations and gas savings for execution and transaction data cost. + * + * Bundlers and clients whitelist supported aggregators. + * + * See https://eips.ethereum.org/EIPS/eip-7766[ERC-7766] */ interface IAggregator { /** * @dev Validates the signature for a user operation. + * Returns an alternative signature that should be used during bundling. */ function validateUserOpSignature( PackedUserOperation calldata userOp @@ -73,6 +81,12 @@ interface IAggregator { /** * @dev Handle nonce management for accounts. + * + * Nonces are used in accounts as a replay protection mechanism and to ensure the order of user operations. + * To avoid limiting the number of operations an account can perform, the interface allows using parallel + * nonces by using a `key` parameter. + * + * See https://eips.ethereum.org/EIPS/eip-4337#semi-abstracted-nonce-support[ERC-4337 semi-abstracted nonce support]. */ interface IEntryPointNonces { /** @@ -84,7 +98,11 @@ interface IEntryPointNonces { } /** - * @dev Handle stake management for accounts. + * @dev Handle stake management for entities (i.e. accounts, paymasters, factories). + * + * The EntryPoint must implement the following API to let entities like paymasters have a stake, + * and thus have more flexibility in their storage access + * (see https://eips.ethereum.org/EIPS/eip-4337#reputation-scoring-and-throttlingbanning-for-global-entities[reputation, throttling and banning.]) */ interface IEntryPointStake { /** @@ -120,6 +138,8 @@ interface IEntryPointStake { /** * @dev Entry point for user operations. + * + * User operations are validated and executed by this contract. */ interface IEntryPoint is IEntryPointNonces, IEntryPointStake { /** @@ -143,11 +163,13 @@ interface IEntryPoint is IEntryPointNonces, IEntryPointStake { /** * @dev Executes a batch of user operations. + * @param beneficiary Address to which gas is refunded up completing the execution. */ function handleOps(PackedUserOperation[] calldata ops, address payable beneficiary) external; /** * @dev Executes a batch of aggregated user operations per aggregator. + * @param beneficiary Address to which gas is refunded up completing the execution. */ function handleAggregatedOps( UserOpsPerAggregator[] calldata opsPerAggregator, @@ -156,11 +178,23 @@ interface IEntryPoint is IEntryPointNonces, IEntryPointStake { } /** - * @dev Base interface for an account. + * @dev Base interface for an ERC-4337 account. */ interface IAccount { /** * @dev Validates a user operation. + * + * * MUST validate the caller is a trusted EntryPoint + * * MUST validate that the signature is a valid signature of the userOpHash, and SHOULD + * return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error MUST revert. + * * MUST pay the entryPoint (caller) at least the “missingAccountFunds” (which might + * be zero, in case the current account’s deposit is high enough) + * + * Returns an encoded packed validation data that is composed of the following elements: + * + * - `authorizer` (`address`): 0 for success, 1 for failure, otherwise the address of an authorizer contract + * - `validUntil` (`uint48`): The UserOp is valid only up to this time. Zero for “infinite”. + * - `validAfter` (`uint48`): The UserOp is valid only after this time. */ function validateUserOp( PackedUserOperation calldata userOp, @@ -193,7 +227,8 @@ interface IPaymaster { } /** - * @dev Validates whether the paymaster is willing to pay for the user operation. + * @dev Validates whether the paymaster is willing to pay for the user operation. See + * {IAccount-validateUserOp} for additional information on the return value. * * NOTE: Bundlers will reject this method if it modifies the state, unless it's whitelisted. */ @@ -205,6 +240,8 @@ interface IPaymaster { /** * @dev Verifies the sender is the entrypoint. + * @param actualGasCost the actual amount paid (by account or paymaster) for this UserOperation + * @param actualUserOpFeePerGas total gas used by this UserOperation (including preVerification, creation, validation and execution) */ function postOp( PostOpMode mode, diff --git a/contracts/interfaces/draft-IERC7579.sol b/contracts/interfaces/draft-IERC7579.sol index 47f1627f682..f97e33dc045 100644 --- a/contracts/interfaces/draft-IERC7579.sol +++ b/contracts/interfaces/draft-IERC7579.sol @@ -10,6 +10,7 @@ uint256 constant MODULE_TYPE_EXECUTOR = 2; uint256 constant MODULE_TYPE_FALLBACK = 3; uint256 constant MODULE_TYPE_HOOK = 4; +/// @dev Minimal configuration interface for ERC-7579 modules interface IERC7579Module { /** * @dev This function is called by the smart account during installation of the module @@ -36,6 +37,11 @@ interface IERC7579Module { function isModuleType(uint256 moduleTypeId) external view returns (bool); } +/** + * @dev ERC-7579 Validation module (type 1). + * + * A module that implements logic to validate user operations and signatures. + */ interface IERC7579Validator is IERC7579Module { /** * @dev Validates a UserOperation @@ -44,6 +50,7 @@ interface IERC7579Validator is IERC7579Module { * * MUST validate that the signature is a valid signature of the userOpHash * SHOULD return ERC-4337's SIG_VALIDATION_FAILED (and not revert) on signature mismatch + * See {IAccount-validateUserOp} for additional information on the return value */ function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); @@ -63,6 +70,12 @@ interface IERC7579Validator is IERC7579Module { ) external view returns (bytes4); } +/** + * @dev ERC-7579 Hooks module (type 4). + * + * A module that implements logic to execute before and after the account executes a user operation, + * either individually or batched. + */ interface IERC7579Hook is IERC7579Module { /** * @dev Called by the smart account before execution @@ -93,6 +106,11 @@ struct Execution { bytes callData; } +/** + * @dev ERC-7579 Execution. + * + * Accounts should implement this interface so that the Entrypoint and ERC-7579 modules can execute operations. + */ interface IERC7579Execution { /** * @dev Executes a transaction on behalf of the account. @@ -109,6 +127,7 @@ interface IERC7579Execution { * This function is intended to be called by Executor Modules * @param mode The encoded execution mode of the transaction. See ModeLib.sol for details * @param executionCalldata The encoded execution call data + * @return returnData An array with the returned data of each executed subcall * * MUST ensure adequate authorization control: i.e. onlyExecutorModule * If a mode is requested that is not supported by the Account, it MUST revert @@ -119,6 +138,11 @@ interface IERC7579Execution { ) external returns (bytes[] memory returnData); } +/** + * @dev ERC-7579 Account Config. + * + * Accounts should implement this interface to expose information that identifies the account, supported modules and capabilities. + */ interface IERC7579AccountConfig { /** * @dev Returns the account id of the smart account @@ -148,6 +172,11 @@ interface IERC7579AccountConfig { function supportsModule(uint256 moduleTypeId) external view returns (bool); } +/** + * @dev ERC-7579 Module Config. + * + * Accounts should implement this interface to allow installing and uninstalling modules. + */ interface IERC7579ModuleConfig { event ModuleInstalled(uint256 moduleTypeId, address module); event ModuleUninstalled(uint256 moduleTypeId, address module); diff --git a/contracts/mocks/docs/access-control/AccessControlUnrevokableAdmin.sol b/contracts/mocks/docs/access-control/AccessControlNonRevokableAdmin.sol similarity index 100% rename from contracts/mocks/docs/access-control/AccessControlUnrevokableAdmin.sol rename to contracts/mocks/docs/access-control/AccessControlNonRevokableAdmin.sol diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 99e25ab46db..5bd45a0864b 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -57,7 +57,7 @@ library Clones { * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. * * This function uses the create2 opcode and a `salt` to deterministically deploy - * the clone. Using the same `implementation` and `salt` multiple time will revert, since + * the clone. Using the same `implementation` and `salt` multiple times will revert, since * the clones cannot be deployed twice at the same address. */ function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) { @@ -163,8 +163,8 @@ library Clones { * access the arguments within the implementation, use {fetchCloneArgs}. * * This function uses the create2 opcode and a `salt` to deterministically deploy the clone. Using the same - * `implementation` and `salt` multiple time will revert, since the clones cannot be deployed twice at the same - * address. + * `implementation`, `args` and `salt` multiple times will revert, since the clones cannot be deployed twice + * at the same address. */ function cloneDeterministicWithImmutableArgs( address implementation, @@ -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/token/ERC20/extensions/ERC1363.sol b/contracts/token/ERC20/extensions/ERC1363.sol index acc841d78fc..952582a8404 100644 --- a/contracts/token/ERC20/extensions/ERC1363.sol +++ b/contracts/token/ERC20/extensions/ERC1363.sol @@ -48,7 +48,8 @@ abstract contract ERC1363 is ERC20, ERC165, IERC1363 { /** * @dev Moves a `value` amount of tokens from the caller's account to `to` - * and then calls {IERC1363Receiver-onTransferReceived} on `to`. + * and then calls {IERC1363Receiver-onTransferReceived} on `to`. Returns a flag that indicates + * if the call succeeded. * * Requirements: * @@ -75,7 +76,8 @@ abstract contract ERC1363 is ERC20, ERC165, IERC1363 { /** * @dev Moves a `value` amount of tokens from `from` to `to` using the allowance mechanism - * and then calls {IERC1363Receiver-onTransferReceived} on `to`. + * and then calls {IERC1363Receiver-onTransferReceived} on `to`. Returns a flag that indicates + * if the call succeeded. * * Requirements: * @@ -108,6 +110,7 @@ abstract contract ERC1363 is ERC20, ERC165, IERC1363 { /** * @dev Sets a `value` amount of tokens as the allowance of `spender` over the * caller's tokens and then calls {IERC1363Spender-onApprovalReceived} on `spender`. + * Returns a flag that indicates if the call succeeded. * * Requirements: * diff --git a/contracts/token/ERC20/utils/SafeERC20.sol b/contracts/token/ERC20/utils/SafeERC20.sol index eb2f903fbed..a77907b4c33 100644 --- a/contracts/token/ERC20/utils/SafeERC20.sol +++ b/contracts/token/ERC20/utils/SafeERC20.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.20; import {IERC20} from "../IERC20.sol"; import {IERC1363} from "../../../interfaces/IERC1363.sol"; -import {Address} from "../../../utils/Address.sol"; /** * @title SafeERC20 diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index 6fe49fd8d14..cf0cb8fcd7f 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -27,15 +27,13 @@ library Bytes { * NOTE: replicates the behavior of https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf[Javascript's `Array.indexOf`] */ function indexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) { - unchecked { - uint256 length = buffer.length; - for (uint256 i = pos; i < length; ++i) { - if (bytes1(_unsafeReadBytesOffset(buffer, i)) == s) { - return i; - } + uint256 length = buffer.length; + for (uint256 i = pos; i < length; ++i) { + if (bytes1(_unsafeReadBytesOffset(buffer, i)) == s) { + return i; } - return type(uint256).max; } + return type(uint256).max; } /** diff --git a/contracts/utils/CAIP10.sol b/contracts/utils/CAIP10.sol index 95aa2a97737..f0424601735 100644 --- a/contracts/utils/CAIP10.sol +++ b/contracts/utils/CAIP10.sol @@ -2,10 +2,9 @@ pragma solidity ^0.8.24; -import {SafeCast} from "./math/SafeCast.sol"; import {Bytes} from "./Bytes.sol"; -import {CAIP2} from "./CAIP2.sol"; import {Strings} from "./Strings.sol"; +import {CAIP2} from "./CAIP2.sol"; /** * @dev Helper library to format and parse CAIP-10 identifiers @@ -14,9 +13,14 @@ import {Strings} from "./Strings.sol"; * account_id: chain_id + ":" + account_address * chain_id: [-a-z0-9]{3,8}:[-_a-zA-Z0-9]{1,32} (See {CAIP2}) * account_address: [-.%a-zA-Z0-9]{1,128} + * + * WARNING: According to [CAIP-10's canonicalization section](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-10.md#canonicalization), + * the implementation remains at the developer's discretion. Please note that case variations may introduce ambiguity. + * For example, when building hashes to identify accounts or data associated to them, multiple representations of the + * same account would derive to different hashes. For EVM chains, we recommend using checksummed addresses for the + * "account_address" part. They can be generated onchain using {Strings-toChecksumHexString}. */ library CAIP10 { - using SafeCast for uint256; using Strings for address; using Bytes for bytes; diff --git a/contracts/utils/CAIP2.sol b/contracts/utils/CAIP2.sol index cfad67e00d0..a7a69e6a873 100644 --- a/contracts/utils/CAIP2.sol +++ b/contracts/utils/CAIP2.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.24; -import {SafeCast} from "./math/SafeCast.sol"; import {Bytes} from "./Bytes.sol"; import {Strings} from "./Strings.sol"; @@ -13,9 +12,13 @@ import {Strings} from "./Strings.sol"; * chain_id: namespace + ":" + reference * namespace: [-a-z0-9]{3,8} * reference: [-_a-zA-Z0-9]{1,32} + * + * WARNING: In some cases, multiple CAIP-2 identifiers may all be valid representation of a single chain. + * For EVM chains, it is recommended to use `eip155:xxx` as the canonical representation (where `xxx` is + * the EIP-155 chain id). Consider the possible ambiguity when processing CAIP-2 identifiers or when using them + * in the context of hashes. */ library CAIP2 { - using SafeCast for uint256; using Strings for uint256; using Bytes for bytes; diff --git a/contracts/utils/NoncesKeyed.sol b/contracts/utils/NoncesKeyed.sol index 133b0c3fe4a..31cd0704e15 100644 --- a/contracts/utils/NoncesKeyed.sol +++ b/contracts/utils/NoncesKeyed.sol @@ -4,22 +4,26 @@ pragma solidity ^0.8.20; import {Nonces} from "./Nonces.sol"; /** - * @dev Alternative to {Nonces}, that support key-ed nonces. + * @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; /// @dev Returns the next unused nonce for an address and key. Result contains the key prefix. function nonces(address owner, uint192 key) public view virtual returns (uint256) { - return key == 0 ? nonces(owner) : ((uint256(key) << 64) | _nonces[owner][key]); + return key == 0 ? nonces(owner) : _pack(key, _nonces[owner][key]); } /** * @dev Consumes the next unused nonce for an address and key. * - * Returns the current value without the key prefix. Consumed nonce is increased, so calling this functions twice + * Returns the current value without the key prefix. Consumed nonce is increased, so calling this function twice * with the same arguments will return different (sequential) results. */ function _useNonce(address owner, uint192 key) internal virtual returns (uint256) { @@ -27,7 +31,7 @@ abstract contract NoncesKeyed is Nonces { // decremented or reset. This guarantees that the nonce never overflows. unchecked { // It is important to do x++ and not ++x here. - return key == 0 ? _useNonce(owner) : _nonces[owner][key]++; + return key == 0 ? _useNonce(owner) : _pack(key, _nonces[owner][key]++); } } @@ -35,11 +39,17 @@ abstract contract NoncesKeyed is Nonces { * @dev Same as {_useNonce} but checking that `nonce` is the next valid for `owner`. * * This version takes the key and the nonce in a single uint256 parameter: - * - use the first 8 bytes for the key - * - use the last 24 bytes for the nonce + * - use the first 24 bytes for the key + * - use the last 8 bytes for the nonce */ function _useCheckedNonce(address owner, uint256 keyNonce) internal virtual override { - _useCheckedNonce(owner, uint192(keyNonce >> 64), uint64(keyNonce)); + (uint192 key, ) = _unpack(keyNonce); + if (key == 0) { + super._useCheckedNonce(owner, keyNonce); + } else { + uint256 current = _useNonce(owner, key); + if (keyNonce != current) revert InvalidAccountNonce(owner, current); + } } /** @@ -48,13 +58,16 @@ abstract contract NoncesKeyed is Nonces { * This version takes the key and the nonce as two different parameters. */ function _useCheckedNonce(address owner, uint192 key, uint64 nonce) internal virtual { - if (key == 0) { - super._useCheckedNonce(owner, nonce); - } else { - uint256 current = _useNonce(owner, key); - if (nonce != current) { - revert InvalidAccountNonce(owner, current); - } - } + _useCheckedNonce(owner, _pack(key, nonce)); + } + + /// @dev Pack key and nonce into a keyNonce + function _pack(uint192 key, uint64 nonce) private pure returns (uint256) { + return (uint256(key) << 64) | nonce; + } + + /// @dev Unpack a keyNonce into its key and nonce components + function _unpack(uint256 keyNonce) private pure returns (uint192 key, uint64 nonce) { + return (uint192(keyNonce >> 64), uint64(keyNonce)); } } diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index c353212927e..f9465eaf04d 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -158,7 +158,7 @@ library Strings { * NOTE: This function will revert if the result does not fit in a `uint256`. */ function tryParseUint(string memory input) internal pure returns (bool success, uint256 value) { - return tryParseUint(input, 0, bytes(input).length); + return _tryParseUintUncheckedBounds(input, 0, bytes(input).length); } /** @@ -172,6 +172,19 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, uint256 value) { + if (end > bytes(input).length || begin > end) return (false, 0); + return _tryParseUintUncheckedBounds(input, begin, end); + } + + /** + * @dev Implementation of {tryParseUint} that does not check bounds. Caller should make sure that + * `begin <= end <= input.length`. Other inputs would result in undefined behavior. + */ + function _tryParseUintUncheckedBounds( + string memory input, + uint256 begin, + uint256 end + ) private pure returns (bool success, uint256 value) { bytes memory buffer = bytes(input); uint256 result = 0; @@ -216,7 +229,7 @@ library Strings { * NOTE: This function will revert if the absolute value of the result does not fit in a `uint256`. */ function tryParseInt(string memory input) internal pure returns (bool success, int256 value) { - return tryParseInt(input, 0, bytes(input).length); + return _tryParseIntUncheckedBounds(input, 0, bytes(input).length); } uint256 private constant ABS_MIN_INT256 = 2 ** 255; @@ -232,10 +245,23 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, int256 value) { + if (end > bytes(input).length || begin > end) return (false, 0); + return _tryParseIntUncheckedBounds(input, begin, end); + } + + /** + * @dev Implementation of {tryParseInt} that does not check bounds. Caller should make sure that + * `begin <= end <= input.length`. Other inputs would result in undefined behavior. + */ + function _tryParseIntUncheckedBounds( + string memory input, + uint256 begin, + uint256 end + ) private pure returns (bool success, int256 value) { bytes memory buffer = bytes(input); // Check presence of a negative sign. - bytes1 sign = bytes1(_unsafeReadBytesOffset(buffer, begin)); + bytes1 sign = begin == end ? bytes1(0) : bytes1(_unsafeReadBytesOffset(buffer, begin)); // don't do out-of-bound (possibly unsafe) read if sub-string is empty bool positiveSign = sign == bytes1("+"); bool negativeSign = sign == bytes1("-"); uint256 offset = (positiveSign || negativeSign).toUint(); @@ -280,7 +306,7 @@ library Strings { * NOTE: This function will revert if the result does not fit in a `uint256`. */ function tryParseHexUint(string memory input) internal pure returns (bool success, uint256 value) { - return tryParseHexUint(input, 0, bytes(input).length); + return _tryParseHexUintUncheckedBounds(input, 0, bytes(input).length); } /** @@ -294,10 +320,23 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, uint256 value) { + if (end > bytes(input).length || begin > end) return (false, 0); + return _tryParseHexUintUncheckedBounds(input, begin, end); + } + + /** + * @dev Implementation of {tryParseHexUint} that does not check bounds. Caller should make sure that + * `begin <= end <= input.length`. Other inputs would result in undefined behavior. + */ + function _tryParseHexUintUncheckedBounds( + string memory input, + uint256 begin, + uint256 end + ) private pure returns (bool success, uint256 value) { bytes memory buffer = bytes(input); // skip 0x prefix if present - bool hasPrefix = bytes2(_unsafeReadBytesOffset(buffer, begin)) == bytes2("0x"); + bool hasPrefix = (end > begin + 1) && bytes2(_unsafeReadBytesOffset(buffer, begin)) == bytes2("0x"); // don't do out-of-bound (possibly unsafe) read if sub-string is empty uint256 offset = hasPrefix.toUint() * 2; uint256 result = 0; @@ -354,13 +393,15 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, address value) { - // check that input is the correct length - bool hasPrefix = bytes2(_unsafeReadBytesOffset(bytes(input), begin)) == bytes2("0x"); + if (end > bytes(input).length || begin > end) return (false, address(0)); + + bool hasPrefix = (end > begin + 1) && bytes2(_unsafeReadBytesOffset(bytes(input), begin)) == bytes2("0x"); // don't do out-of-bound (possibly unsafe) read if sub-string is empty uint256 expectedLength = 40 + hasPrefix.toUint() * 2; + // check that input is the correct length if (end - begin == expectedLength) { // length guarantees that this does not overflow, and value is at most type(uint160).max - (bool s, uint256 v) = tryParseHexUint(input, begin, end); + (bool s, uint256 v) = _tryParseHexUintUncheckedBounds(input, begin, end); return (s, address(uint160(v))); } else { return (false, address(0)); diff --git a/test/account/utils/draft-ERC4337Utils.test.js b/test/account/utils/draft-ERC4337Utils.test.js index 8374bc745c9..ab2500b5382 100644 --- a/test/account/utils/draft-ERC4337Utils.test.js +++ b/test/account/utils/draft-ERC4337Utils.test.js @@ -2,13 +2,16 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); -const { packValidationData, packPaymasterData, UserOperation } = require('../../helpers/erc4337'); +const { packValidationData, UserOperation } = require('../../helpers/erc4337'); const { MAX_UINT48 } = require('../../helpers/constants'); +const ADDRESS_ONE = '0x0000000000000000000000000000000000000001'; const fixture = async () => { - const [authorizer, sender, entrypoint, paymaster] = await ethers.getSigners(); + const [authorizer, sender, entrypoint, factory, paymaster] = await ethers.getSigners(); const utils = await ethers.deployContract('$ERC4337Utils'); - return { utils, authorizer, sender, entrypoint, paymaster }; + const SIG_VALIDATION_SUCCESS = await utils.$SIG_VALIDATION_SUCCESS(); + const SIG_VALIDATION_FAILED = await utils.$SIG_VALIDATION_FAILED(); + return { utils, authorizer, sender, entrypoint, factory, paymaster, SIG_VALIDATION_SUCCESS, SIG_VALIDATION_FAILED }; }; describe('ERC4337Utils', function () { @@ -41,6 +44,20 @@ describe('ERC4337Utils', function () { MAX_UINT48, ]); }); + + it('parse canonical values', async function () { + expect(this.utils.$parseValidationData(this.SIG_VALIDATION_SUCCESS)).to.eventually.deep.equal([ + ethers.ZeroAddress, + 0n, + MAX_UINT48, + ]); + + expect(this.utils.$parseValidationData(this.SIG_VALIDATION_FAILED)).to.eventually.deep.equal([ + ADDRESS_ONE, + 0n, + MAX_UINT48, + ]); + }); }); describe('packValidationData', function () { @@ -65,6 +82,21 @@ describe('ERC4337Utils', function () { validationData, ); }); + + it('packing reproduced canonical values', async function () { + expect(this.utils.$packValidationData(ethers.Typed.address(ethers.ZeroAddress), 0n, 0n)).to.eventually.equal( + this.SIG_VALIDATION_SUCCESS, + ); + expect(this.utils.$packValidationData(ethers.Typed.bool(true), 0n, 0n)).to.eventually.equal( + this.SIG_VALIDATION_SUCCESS, + ); + expect(this.utils.$packValidationData(ethers.Typed.address(ADDRESS_ONE), 0n, 0n)).to.eventually.equal( + this.SIG_VALIDATION_FAILED, + ); + expect(this.utils.$packValidationData(ethers.Typed.bool(false), 0n, 0n)).to.eventually.equal( + this.SIG_VALIDATION_FAILED, + ); + }); }); describe('combineValidationData', function () { @@ -133,13 +165,6 @@ describe('ERC4337Utils', function () { }); describe('hash', function () { - it('returns the user operation hash', async function () { - const userOp = new UserOperation({ sender: this.sender, nonce: 1 }); - const chainId = await ethers.provider.getNetwork().then(({ chainId }) => chainId); - - expect(this.utils.$hash(userOp.packed)).to.eventually.equal(userOp.hash(this.utils.target, chainId)); - }); - it('returns the operation hash with specified entrypoint and chainId', async function () { const userOp = new UserOperation({ sender: this.sender, nonce: 1 }); const chainId = 0xdeadbeef; @@ -151,6 +176,33 @@ describe('ERC4337Utils', function () { }); describe('userOp values', function () { + describe('intiCode', function () { + beforeEach(async function () { + this.userOp = new UserOperation({ + sender: this.sender, + nonce: 1, + verificationGas: 0x12345678n, + factory: this.factory, + factoryData: '0x123456', + }); + + this.emptyUserOp = new UserOperation({ + sender: this.sender, + nonce: 1, + }); + }); + + it('returns factory', async function () { + expect(this.utils.$factory(this.userOp.packed)).to.eventually.equal(this.factory); + expect(this.utils.$factory(this.emptyUserOp.packed)).to.eventually.equal(ethers.ZeroAddress); + }); + + it('returns factoryData', async function () { + expect(this.utils.$factoryData(this.userOp.packed)).to.eventually.equal('0x123456'); + expect(this.utils.$factoryData(this.emptyUserOp.packed)).to.eventually.equal('0x'); + }); + }); + it('returns verificationGasLimit', async function () { const userOp = new UserOperation({ sender: this.sender, nonce: 1, verificationGas: 0x12345678n }); expect(this.utils.$verificationGasLimit(userOp.packed)).to.eventually.equal(userOp.verificationGas); @@ -183,28 +235,43 @@ describe('ERC4337Utils', function () { describe('paymasterAndData', function () { beforeEach(async function () { - this.verificationGasLimit = 0x12345678n; - this.postOpGasLimit = 0x87654321n; - this.paymasterAndData = packPaymasterData(this.paymaster, this.verificationGasLimit, this.postOpGasLimit); this.userOp = new UserOperation({ sender: this.sender, nonce: 1, - paymasterAndData: this.paymasterAndData, + paymaster: this.paymaster, + paymasterVerificationGasLimit: 0x12345678n, + paymasterPostOpGasLimit: 0x87654321n, + paymasterData: '0xbeefcafe', + }); + + this.emptyUserOp = new UserOperation({ + sender: this.sender, + nonce: 1, }); }); it('returns paymaster', async function () { - expect(this.utils.$paymaster(this.userOp.packed)).to.eventually.equal(this.paymaster); + expect(this.utils.$paymaster(this.userOp.packed)).to.eventually.equal(this.userOp.paymaster); + expect(this.utils.$paymaster(this.emptyUserOp.packed)).to.eventually.equal(ethers.ZeroAddress); }); it('returns verificationGasLimit', async function () { expect(this.utils.$paymasterVerificationGasLimit(this.userOp.packed)).to.eventually.equal( - this.verificationGasLimit, + this.userOp.paymasterVerificationGasLimit, ); + expect(this.utils.$paymasterVerificationGasLimit(this.emptyUserOp.packed)).to.eventually.equal(0n); }); it('returns postOpGasLimit', async function () { - expect(this.utils.$paymasterPostOpGasLimit(this.userOp.packed)).to.eventually.equal(this.postOpGasLimit); + expect(this.utils.$paymasterPostOpGasLimit(this.userOp.packed)).to.eventually.equal( + this.userOp.paymasterPostOpGasLimit, + ); + expect(this.utils.$paymasterPostOpGasLimit(this.emptyUserOp.packed)).to.eventually.equal(0n); + }); + + it('returns data', async function () { + expect(this.utils.$paymasterData(this.userOp.packed)).to.eventually.equal(this.userOp.paymasterData); + expect(this.utils.$paymasterData(this.emptyUserOp.packed)).to.eventually.equal('0x'); }); }); }); diff --git a/test/account/utils/draft-ERC7579Utils.test.js b/test/account/utils/draft-ERC7579Utils.test.js index cc3b70425dd..e72b6698d60 100644 --- a/test/account/utils/draft-ERC7579Utils.test.js +++ b/test/account/utils/draft-ERC7579Utils.test.js @@ -1,6 +1,6 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); -const { loadFixture, setBalance } = require('@nomicfoundation/hardhat-network-helpers'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { EXEC_TYPE_DEFAULT, EXEC_TYPE_TRY, @@ -17,11 +17,10 @@ const coder = ethers.AbiCoder.defaultAbiCoder(); const fixture = async () => { const [sender] = await ethers.getSigners(); - const utils = await ethers.deployContract('$ERC7579Utils'); + const utils = await ethers.deployContract('$ERC7579Utils', { value: ethers.parseEther('1') }); const utilsGlobal = await ethers.deployContract('$ERC7579UtilsGlobalMock'); const target = await ethers.deployContract('CallReceiverMock'); const anotherTarget = await ethers.deployContract('CallReceiverMock'); - await setBalance(utils.target, ethers.parseEther('1')); return { utils, utilsGlobal, target, anotherTarget, sender }; }; diff --git a/test/governance/utils/VotesAdditionalCheckpoints.test.js b/test/governance/utils/VotesExtended.test.js similarity index 100% rename from test/governance/utils/VotesAdditionalCheckpoints.test.js rename to test/governance/utils/VotesExtended.test.js diff --git a/test/helpers/erc4337.js b/test/helpers/erc4337.js index 5901375b174..50a3b4a043b 100644 --- a/test/helpers/erc4337.js +++ b/test/helpers/erc4337.js @@ -26,10 +26,14 @@ function packValidationData(validAfter, validUntil, authorizer) { ); } -function packPaymasterData(paymaster, verificationGasLimit, postOpGasLimit) { +function packInitCode(factory, factoryData) { + return ethers.solidityPacked(['address', 'bytes'], [getAddress(factory), factoryData]); +} + +function packPaymasterAndData(paymaster, paymasterVerificationGasLimit, paymasterPostOpGasLimit, paymasterData) { return ethers.solidityPacked( - ['address', 'uint128', 'uint128'], - [getAddress(paymaster), verificationGasLimit, postOpGasLimit], + ['address', 'uint128', 'uint128', 'bytes'], + [getAddress(paymaster), paymasterVerificationGasLimit, paymasterPostOpGasLimit, paymasterData], ); } @@ -38,14 +42,18 @@ class UserOperation { constructor(params) { this.sender = getAddress(params.sender); this.nonce = params.nonce; - this.initCode = params.initCode ?? '0x'; + this.factory = params.factory ?? undefined; + this.factoryData = params.factoryData ?? '0x'; this.callData = params.callData ?? '0x'; this.verificationGas = params.verificationGas ?? 10_000_000n; this.callGas = params.callGas ?? 100_000n; this.preVerificationGas = params.preVerificationGas ?? 100_000n; this.maxPriorityFee = params.maxPriorityFee ?? 100_000n; this.maxFeePerGas = params.maxFeePerGas ?? 100_000n; - this.paymasterAndData = params.paymasterAndData ?? '0x'; + this.paymaster = params.paymaster ?? undefined; + this.paymasterVerificationGasLimit = params.paymasterVerificationGasLimit ?? 0n; + this.paymasterPostOpGasLimit = params.paymasterPostOpGasLimit ?? 0n; + this.paymasterData = params.paymasterData ?? '0x'; this.signature = params.signature ?? '0x'; } @@ -53,12 +61,19 @@ class UserOperation { return { sender: this.sender, nonce: this.nonce, - initCode: this.initCode, + initCode: this.factory ? packInitCode(this.factory, this.factoryData) : '0x', callData: this.callData, accountGasLimits: pack(this.verificationGas, this.callGas), preVerificationGas: this.preVerificationGas, gasFees: pack(this.maxPriorityFee, this.maxFeePerGas), - paymasterAndData: this.paymasterAndData, + paymasterAndData: this.paymaster + ? packPaymasterAndData( + this.paymaster, + this.paymasterVerificationGasLimit, + this.paymasterPostOpGasLimit, + this.paymasterData, + ) + : '0x', signature: this.signature, }; } @@ -90,6 +105,7 @@ module.exports = { SIG_VALIDATION_SUCCESS, SIG_VALIDATION_FAILURE, packValidationData, - packPaymasterData, + packInitCode, + packPaymasterAndData, UserOperation, }; diff --git a/test/utils/Nonces.behavior.js b/test/utils/Nonces.behavior.js index 17073966427..32201f690d6 100644 --- a/test/utils/Nonces.behavior.js +++ b/test/utils/Nonces.behavior.js @@ -86,7 +86,7 @@ function shouldBehaveLikeNoncesKeyed() { await expect(this.mock.$_useNonce(sender, ethers.Typed.uint192(0n))) .to.emit(this.mock, 'return$_useNonce_address_uint192') - .withArgs(1n); + .withArgs(keyOffset(0n) + 1n); expect(this.mock.nonces(sender, ethers.Typed.uint192(0n))).to.eventually.equal(keyOffset(0n) + 2n); expect(this.mock.nonces(sender, ethers.Typed.uint192(17n))).to.eventually.equal(keyOffset(17n) + 0n); @@ -98,18 +98,18 @@ function shouldBehaveLikeNoncesKeyed() { await expect(this.mock.$_useNonce(sender, ethers.Typed.uint192(17n))) .to.emit(this.mock, 'return$_useNonce_address_uint192') - .withArgs(0n); + .withArgs(keyOffset(17n) + 0n); await expect(this.mock.$_useNonce(sender, ethers.Typed.uint192(17n))) .to.emit(this.mock, 'return$_useNonce_address_uint192') - .withArgs(1n); + .withArgs(keyOffset(17n) + 1n); expect(this.mock.nonces(sender, ethers.Typed.uint192(0n))).to.eventually.equal(keyOffset(0n) + 0n); expect(this.mock.nonces(sender, ethers.Typed.uint192(17n))).to.eventually.equal(keyOffset(17n) + 2n); }); }); - describe('_useCheckedNonce', function () { + describe('_useCheckedNonce(address, uint256)', function () { it('default variant uses key 0', async function () { const currentNonce = await this.mock.nonces(sender, ethers.Typed.uint192(0n)); @@ -135,12 +135,49 @@ function shouldBehaveLikeNoncesKeyed() { // reuse same nonce await expect(this.mock.$_useCheckedNonce(sender, currentNonce)) .to.be.revertedWithCustomError(this.mock, 'InvalidAccountNonce') - .withArgs(sender, 1); + .withArgs(sender, currentNonce + 1n); // use "future" nonce too early await expect(this.mock.$_useCheckedNonce(sender, currentNonce + 10n)) .to.be.revertedWithCustomError(this.mock, 'InvalidAccountNonce') - .withArgs(sender, 1); + .withArgs(sender, currentNonce + 1n); + }); + }); + + describe('_useCheckedNonce(address, uint192, uint64)', function () { + const MASK = 0xffffffffffffffffn; + + it('default variant uses key 0', async function () { + const currentNonce = await this.mock.nonces(sender, ethers.Typed.uint192(0n)); + + await this.mock.$_useCheckedNonce(sender, ethers.Typed.uint192(0n), currentNonce); + + expect(this.mock.nonces(sender, ethers.Typed.uint192(0n))).to.eventually.equal(currentNonce + 1n); + }); + + it('use nonce at another key', async function () { + const currentNonce = await this.mock.nonces(sender, ethers.Typed.uint192(17n)); + + await this.mock.$_useCheckedNonce(sender, ethers.Typed.uint192(17n), currentNonce & MASK); + + expect(this.mock.nonces(sender, ethers.Typed.uint192(17n))).to.eventually.equal(currentNonce + 1n); + }); + + it('reverts when nonce is not the expected', async function () { + const currentNonce = await this.mock.nonces(sender, ethers.Typed.uint192(42n)); + + // use and increment + await this.mock.$_useCheckedNonce(sender, ethers.Typed.uint192(42n), currentNonce & MASK); + + // reuse same nonce + await expect(this.mock.$_useCheckedNonce(sender, ethers.Typed.uint192(42n), currentNonce & MASK)) + .to.be.revertedWithCustomError(this.mock, 'InvalidAccountNonce') + .withArgs(sender, currentNonce + 1n); + + // use "future" nonce too early + await expect(this.mock.$_useCheckedNonce(sender, ethers.Typed.uint192(42n), (currentNonce & MASK) + 10n)) + .to.be.revertedWithCustomError(this.mock, 'InvalidAccountNonce') + .withArgs(sender, currentNonce + 1n); }); }); }); diff --git a/test/utils/Strings.t.sol b/test/utils/Strings.t.sol index b3eb67a5cd2..f59e675cd32 100644 --- a/test/utils/Strings.t.sol +++ b/test/utils/Strings.t.sol @@ -24,4 +24,27 @@ contract StringsTest is Test { function testParseChecksumHex(address value) external { assertEq(value, value.toChecksumHexString().parseAddress()); } + + function testTryParseHexUintExtendedEnd(string memory random) external pure { + uint256 length = bytes(random).length; + assembly ("memory-safe") { + mstore(add(add(random, 0x20), length), 0x3030303030303030303030303030303030303030303030303030303030303030) + } + + (bool success, ) = random.tryParseHexUint(1, length + 1); + assertFalse(success); + } + + function testTryParseAddressExtendedEnd(address random, uint256 begin) external pure { + begin = bound(begin, 3, 43); + string memory input = random.toHexString(); + uint256 length = bytes(input).length; + + assembly ("memory-safe") { + mstore(add(add(input, 0x20), length), 0x3030303030303030303030303030303030303030303030303030303030303030) + } + + (bool success, ) = input.tryParseAddress(begin, begin + 40); + assertFalse(success); + } } diff --git a/test/utils/Strings.test.js b/test/utils/Strings.test.js index 5a47d4d10de..0b2d87190d3 100644 --- a/test/utils/Strings.test.js +++ b/test/utils/Strings.test.js @@ -240,6 +240,11 @@ describe('Strings', function () { expect(await this.mock.$tryParseUint('1 000')).deep.equal([false, 0n]); }); + it('parseUint invalid range', async function () { + expect(this.mock.$parseUint('12', 3, 2)).to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar'); + expect(await this.mock.$tryParseUint('12', 3, 2)).to.deep.equal([false, 0n]); + }); + it('parseInt overflow', async function () { await expect(this.mock.$parseInt((ethers.MaxUint256 + 1n).toString(10))).to.be.revertedWithPanic( PANIC_CODES.ARITHMETIC_OVERFLOW, @@ -276,6 +281,11 @@ describe('Strings', function () { expect(await this.mock.$tryParseInt('1 000')).to.deep.equal([false, 0n]); }); + it('parseInt invalid range', async function () { + expect(this.mock.$parseInt('-12', 3, 2)).to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar'); + expect(await this.mock.$tryParseInt('-12', 3, 2)).to.deep.equal([false, 0n]); + }); + it('parseHexUint overflow', async function () { await expect(this.mock.$parseHexUint((ethers.MaxUint256 + 1n).toString(16))).to.be.revertedWithPanic( PANIC_CODES.ARITHMETIC_OVERFLOW, @@ -303,6 +313,11 @@ describe('Strings', function () { expect(await this.mock.$tryParseHexUint('1 000')).to.deep.equal([false, 0n]); }); + it('parseHexUint invalid begin and end', async function () { + expect(this.mock.$parseHexUint('0x', 3, 2)).to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar'); + expect(await this.mock.$tryParseHexUint('0x', 3, 2)).to.deep.equal([false, 0n]); + }); + it('parseAddress invalid format', async function () { for (const addr of [ '0x736a507fB2881d6bB62dcA54673CF5295dC07833', // valid