Implement feedback for M-01, L-08, L-09 (OpenZeppelin#5324)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

Author: ernestognw

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);

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,

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,

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);

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(

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:
      *

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)));

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 () {