Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Nov 29, 2024
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
8 changes: 8 additions & 0 deletions contracts/interfaces/draft-IERC4337.sol
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,12 @@ interface IEntryPoint is IEntryPointNonces, IEntryPointStake {
interface IAccount {
/**
* @dev Validates a user operation.
*
* 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 @@ -207,6 +213,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 {IERC4337-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 unexpected behavior.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
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 unexpected behavior.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
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 unexpected behavior.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
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