Skip to content

Commit

Permalink
Implement feedback for M-01, L-08, L-09 (OpenZeppelin#5324)
Browse files Browse the repository at this point in the history
Co-authored-by: Hadrien Croubois <[email protected]>
Signed-off-by: Hadrien Croubois <[email protected]>
  • Loading branch information
ernestognw and Amxx committed Nov 30, 2024
1 parent a2dfd87 commit 4299784
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 23 deletions.
5 changes: 3 additions & 2 deletions contracts/account/utils/draft-ERC7579Utils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
41 changes: 38 additions & 3 deletions contracts/interfaces/draft-IERC4337.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
/**
Expand All @@ -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 {
/**
Expand Down Expand Up @@ -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 {
/**
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
*/
Expand All @@ -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,
Expand Down
7 changes: 4 additions & 3 deletions contracts/interfaces/draft-IERC7579.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand All @@ -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 {
/**
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion contracts/proxy/Clones.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
7 changes: 5 additions & 2 deletions contracts/token/ERC20/extensions/ERC1363.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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:
*
Expand All @@ -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:
*
Expand Down Expand Up @@ -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:
*
Expand Down
18 changes: 11 additions & 7 deletions contracts/utils/Strings.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down Expand Up @@ -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)));
Expand Down
34 changes: 33 additions & 1 deletion test/account/utils/draft-ERC4337Utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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 () {
Expand All @@ -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 () {
Expand Down

0 comments on commit 4299784

Please sign in to comment.