diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index 652f55545..dab18f26b 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -38,9 +38,10 @@ library ERC7579Utils { /** * @dev Emits when an {EXECTYPE_TRY} execution fails. - * @param batchExecutionIndex The index of the failed transaction in the execution batch. + * @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 result); + event ERC7579TryExecuteFail(uint256 batchExecutionIndex, bytes returndata); /// @dev The provided {CallType} is not supported. error ERC7579UnsupportedCallType(CallType callType); diff --git a/contracts/governance/extensions/GovernorCountingOverridable.sol b/contracts/governance/extensions/GovernorCountingOverridable.sol index 5b89c6ddf..db375a93f 100644 --- a/contracts/governance/extensions/GovernorCountingOverridable.sol +++ b/contracts/governance/extensions/GovernorCountingOverridable.sol @@ -8,7 +8,7 @@ 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 + * @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 { @@ -162,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, @@ -180,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, @@ -190,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/interfaces/draft-IERC4337.sol b/contracts/interfaces/draft-IERC4337.sol index 5d765dbcd..67ab146c0 100644 --- a/contracts/interfaces/draft-IERC4337.sol +++ b/contracts/interfaces/draft-IERC4337.sol @@ -45,10 +45,18 @@ struct PackedUserOperation { /** * @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 { /** @@ -158,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, @@ -195,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. */ @@ -207,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 61a862377..f97e33dc0 100644 --- a/contracts/interfaces/draft-IERC7579.sol +++ b/contracts/interfaces/draft-IERC7579.sol @@ -50,7 +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 ERC-4337 for additional information on the return value + * See {IAccount-validateUserOp} for additional information on the return value */ function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); @@ -127,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 @@ -140,7 +141,7 @@ interface IERC7579Execution { /** * @dev ERC-7579 Account Config. * - * Accounts should implement this interface to exposes information that identifies the account, supported modules and capabilities. + * Accounts should implement this interface to expose information that identifies the account, supported modules and capabilities. */ interface IERC7579AccountConfig { /** @@ -174,7 +175,7 @@ interface IERC7579AccountConfig { /** * @dev ERC-7579 Module Config. * - * Accounts should implement this interface to allows installing and uninstalling modules. + * Accounts should implement this interface to allow installing and uninstalling modules. */ interface IERC7579ModuleConfig { event ModuleInstalled(uint256 moduleTypeId, address module); diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index ed8fd6fab..5bd45a086 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -163,7 +163,7 @@ 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`, `args` and `salt` multiple time will revert, since the clones cannot be deployed twice + * `implementation`, `args` and `salt` multiple times will revert, since the clones cannot be deployed twice * at the same address. */ function cloneDeterministicWithImmutableArgs( diff --git a/contracts/token/ERC20/extensions/ERC1363.sol b/contracts/token/ERC20/extensions/ERC1363.sol index acc841d78..952582a84 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/utils/Strings.sol b/contracts/utils/Strings.sol index 161a2b576..f9465eaf0 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -177,7 +177,8 @@ library Strings { } /** - * @dev Variant of {tryParseUint} that does not check bounds and returns (true, 0) if they are invalid. + * @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, @@ -249,7 +250,8 @@ library Strings { } /** - * @dev Variant of {tryParseInt} that does not check bounds and returns (true, 0) if they are invalid. + * @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, @@ -323,7 +325,8 @@ library Strings { } /** - * @dev Variant of {tryParseHexUint} that does not check bounds and returns (true, 0) if they are invalid. + * @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, @@ -333,7 +336,7 @@ library Strings { bytes memory buffer = bytes(input); // skip 0x prefix if present - bool hasPrefix = (begin < end + 1) && bytes2(_unsafeReadBytesOffset(buffer, begin)) == bytes2("0x"); // don't do out-of-bound (possibly unsafe) read if sub-string is empty + 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; @@ -390,12 +393,13 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, address value) { - // check that input is the correct length - bool hasPrefix = (begin < end + 1) && bytes2(_unsafeReadBytesOffset(bytes(input), begin)) == bytes2("0x"); // don't do out-of-bound (possibly unsafe) read if sub-string is empty + 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; - if (end - begin == expectedLength && end <= bytes(input).length) { + // 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) = _tryParseHexUintUncheckedBounds(input, begin, end); return (s, address(uint160(v))); diff --git a/test/account/utils/draft-ERC4337Utils.test.js b/test/account/utils/draft-ERC4337Utils.test.js index 96310ed83..ab2500b53 100644 --- a/test/account/utils/draft-ERC4337Utils.test.js +++ b/test/account/utils/draft-ERC4337Utils.test.js @@ -4,11 +4,14 @@ const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { packValidationData, UserOperation } = require('../../helpers/erc4337'); const { MAX_UINT48 } = require('../../helpers/constants'); +const ADDRESS_ONE = '0x0000000000000000000000000000000000000001'; const fixture = async () => { const [authorizer, sender, entrypoint, factory, paymaster] = await ethers.getSigners(); const utils = await ethers.deployContract('$ERC4337Utils'); - return { utils, authorizer, sender, entrypoint, factory, 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 () {