From ccb5f2d8ca9d194623cf3cff7f010ab92715826b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 20 Nov 2024 09:21:49 +0100 Subject: [PATCH] Fix 5.2 audit L-05, N-03, N-04, N-05 and N-06 issues (#5308) --- contracts/account/utils/draft-ERC4337Utils.sol | 11 +++-------- contracts/account/utils/draft-ERC7579Utils.sol | 10 +++++----- .../extensions/GovernorCountingOverridable.sol | 2 +- contracts/governance/utils/VotesExtended.sol | 2 +- contracts/interfaces/draft-IERC4337.sol | 2 +- contracts/proxy/Clones.sol | 4 ++-- contracts/utils/Bytes.sol | 12 +++++------- contracts/utils/CAIP10.sol | 4 +--- contracts/utils/CAIP2.sol | 2 -- contracts/utils/NoncesKeyed.sol | 4 ++-- test/account/utils/draft-ERC4337Utils.test.js | 7 ------- 11 files changed, 21 insertions(+), 39 deletions(-) diff --git a/contracts/account/utils/draft-ERC4337Utils.sol b/contracts/account/utils/draft-ERC4337Utils.sol index bf559b86d..7f72d3404 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"; @@ -71,12 +71,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, @@ -129,7 +124,7 @@ 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); } } diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index de6bb6509..55fb7a09a 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -22,19 +22,19 @@ 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); + ExecType internal constant EXECTYPE_TRY = ExecType.wrap(0x01); /// @dev Emits when an {EXECTYPE_TRY} execution fails. event ERC7579TryExecuteFail(uint256 batchExecutionIndex, bytes result); diff --git a/contracts/governance/extensions/GovernorCountingOverridable.sol b/contracts/governance/extensions/GovernorCountingOverridable.sol index 9b46903e3..6e20d9fbf 100644 --- a/contracts/governance/extensions/GovernorCountingOverridable.sol +++ b/contracts/governance/extensions/GovernorCountingOverridable.sol @@ -9,7 +9,7 @@ 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`. + * token that inherits {VotesExtended}. */ abstract contract GovernorCountingOverridable is GovernorVotes { bytes32 public constant OVERRIDE_BALLOT_TYPEHASH = diff --git a/contracts/governance/utils/VotesExtended.sol b/contracts/governance/utils/VotesExtended.sol index 62ddd5f7a..1850d1b98 100644 --- a/contracts/governance/utils/VotesExtended.sol +++ b/contracts/governance/utils/VotesExtended.sol @@ -6,7 +6,7 @@ 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. */ abstract contract VotesExtended is Votes { using SafeCast for uint256; diff --git a/contracts/interfaces/draft-IERC4337.sol b/contracts/interfaces/draft-IERC4337.sol index 9b1af56b6..603bce15b 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) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 99e25ab46..19fe6c9f8 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,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` and `salt` multiple time will revert, since the clones cannot be deployed twice at the same + * `implementation` and `salt` multiple times will revert, since the clones cannot be deployed twice at the same * address. */ function cloneDeterministicWithImmutableArgs( diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index 6fe49fd8d..cf0cb8fcd 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 95aa2a977..bd9ddbc9f 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 @@ -16,7 +15,6 @@ import {Strings} from "./Strings.sol"; * account_address: [-.%a-zA-Z0-9]{1,128} */ 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 cfad67e00..c6789ce43 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"; @@ -15,7 +14,6 @@ import {Strings} from "./Strings.sol"; * reference: [-_a-zA-Z0-9]{1,32} */ 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 133b0c3fe..c775700d2 100644 --- a/contracts/utils/NoncesKeyed.sol +++ b/contracts/utils/NoncesKeyed.sol @@ -4,7 +4,7 @@ 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]. */ @@ -19,7 +19,7 @@ abstract contract NoncesKeyed is Nonces { /** * @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) { diff --git a/test/account/utils/draft-ERC4337Utils.test.js b/test/account/utils/draft-ERC4337Utils.test.js index 8374bc745..5138a5d8e 100644 --- a/test/account/utils/draft-ERC4337Utils.test.js +++ b/test/account/utils/draft-ERC4337Utils.test.js @@ -133,13 +133,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;