Release v5.2 audit fixes (#5330)

Signed-off-by: Hadrien Croubois <[email protected]>
Co-authored-by: Sam Bugs <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Arr00 <[email protected]>
Co-authored-by: wizard <[email protected]>
Co-authored-by: leopardracer <[email protected]>
Co-authored-by: cairo <[email protected]>

Author: 7 people

.changeset/proud-planes-arrive.md

@@ -2,4 +2,4 @@
 'openzeppelin-solidity': minor
 ---
 
-`Bytes`: Add a library of common operation that operate on `bytes` objects.
+`Bytes`: Add a library of common operations that operate on `bytes` objects.

GUIDELINES.md

@@ -55,7 +55,7 @@ External contributions must be reviewed separately by multiple maintainers.
 
 Automation should be used as much as possible to reduce the possibility of human error and forgetfulness.
 
-Automations that make use of sensitive credentials must use secure secret management, and must be strengthened against attacks such as [those on GitHub Actions worklows](https://github.com/nikitastupin/pwnhub).
+Automations that make use of sensitive credentials must use secure secret management, and must be strengthened against attacks such as [those on GitHub Actions workflows](https://github.com/nikitastupin/pwnhub).
 
 Some other examples of automation are:
 

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";
 
@@ -24,9 +24,9 @@ library ERC4337Utils {
     function parseValidationData(
         uint256 validationData
     ) internal pure returns (address aggregator, uint48 validAfter, uint48 validUntil) {
-        validAfter = uint48(bytes32(validationData).extract_32_6(0x00));
-        validUntil = uint48(bytes32(validationData).extract_32_6(0x06));
-        aggregator = address(bytes32(validationData).extract_32_20(0x0c));
+        validAfter = uint48(bytes32(validationData).extract_32_6(0));
+        validUntil = uint48(bytes32(validationData).extract_32_6(6));
+        aggregator = address(bytes32(validationData).extract_32_20(12));
         if (validUntil == 0) validUntil = type(uint48).max;
     }
 
@@ -59,7 +59,8 @@ library ERC4337Utils {
         (address aggregator1, uint48 validAfter1, uint48 validUntil1) = parseValidationData(validationData1);
         (address aggregator2, uint48 validAfter2, uint48 validUntil2) = parseValidationData(validationData2);
 
-        bool success = aggregator1 == address(0) && aggregator2 == address(0);
+        bool success = aggregator1 == address(uint160(SIG_VALIDATION_SUCCESS)) &&
+            aggregator2 == address(uint160(SIG_VALIDATION_SUCCESS));
         uint48 validAfter = uint48(Math.max(validAfter1, validAfter2));
         uint48 validUntil = uint48(Math.min(validUntil1, validUntil2));
         return packValidationData(success, validAfter, validUntil);
@@ -71,12 +72,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,
@@ -103,24 +99,34 @@ library ERC4337Utils {
         return result;
     }
 
+    /// @dev Returns `factory` from the {PackedUserOperation}, or address(0) if the initCode is empty or not properly formatted.
+    function factory(PackedUserOperation calldata self) internal pure returns (address) {
+        return self.initCode.length < 20 ? address(0) : address(bytes20(self.initCode[0:20]));
+    }
+
+    /// @dev Returns `factoryData` from the {PackedUserOperation}, or empty bytes if the initCode is empty or not properly formatted.
+    function factoryData(PackedUserOperation calldata self) internal pure returns (bytes calldata) {
+        return self.initCode.length < 20 ? _emptyCalldataBytes() : self.initCode[20:];
+    }
+
     /// @dev Returns `verificationGasLimit` from the {PackedUserOperation}.
     function verificationGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.accountGasLimits.extract_32_16(0x00));
+        return uint128(self.accountGasLimits.extract_32_16(0));
     }
 
     /// @dev Returns `accountGasLimits` from the {PackedUserOperation}.
     function callGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.accountGasLimits.extract_32_16(0x10));
+        return uint128(self.accountGasLimits.extract_32_16(16));
     }
 
     /// @dev Returns the first section of `gasFees` from the {PackedUserOperation}.
     function maxPriorityFeePerGas(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.gasFees.extract_32_16(0x00));
+        return uint128(self.gasFees.extract_32_16(0));
     }
 
     /// @dev Returns the second section of `gasFees` from the {PackedUserOperation}.
     function maxFeePerGas(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(self.gasFees.extract_32_16(0x10));
+        return uint128(self.gasFees.extract_32_16(16));
     }
 
     /// @dev Returns the total gas price for the {PackedUserOperation} (ie. `maxFeePerGas` or `maxPriorityFeePerGas + basefee`).
@@ -129,22 +135,35 @@ 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);
         }
     }
 
     /// @dev Returns the first section of `paymasterAndData` from the {PackedUserOperation}.
     function paymaster(PackedUserOperation calldata self) internal pure returns (address) {
-        return address(bytes20(self.paymasterAndData[0:20]));
+        return self.paymasterAndData.length < 52 ? address(0) : address(bytes20(self.paymasterAndData[0:20]));
     }
 
     /// @dev Returns the second section of `paymasterAndData` from the {PackedUserOperation}.
     function paymasterVerificationGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(bytes16(self.paymasterAndData[20:36]));
+        return self.paymasterAndData.length < 52 ? 0 : uint128(bytes16(self.paymasterAndData[20:36]));
     }
 
     /// @dev Returns the third section of `paymasterAndData` from the {PackedUserOperation}.
     function paymasterPostOpGasLimit(PackedUserOperation calldata self) internal pure returns (uint256) {
-        return uint128(bytes16(self.paymasterAndData[36:52]));
+        return self.paymasterAndData.length < 52 ? 0 : uint128(bytes16(self.paymasterAndData[36:52]));
+    }
+
+    /// @dev Returns the forth section of `paymasterAndData` from the {PackedUserOperation}.
+    function paymasterData(PackedUserOperation calldata self) internal pure returns (bytes calldata) {
+        return self.paymasterAndData.length < 52 ? _emptyCalldataBytes() : self.paymasterAndData[52:];
+    }
+
+    // slither-disable-next-line write-after-write
+    function _emptyCalldataBytes() private pure returns (bytes calldata result) {
+        assembly ("memory-safe") {
+            result.offset := 0
+            result.length := 0
+        }
     }
 }

contracts/account/utils/draft-ERC7579Utils.sol

@@ -22,22 +22,26 @@ 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);
-
-    /// @dev Emits when an {EXECTYPE_TRY} execution fails.
-    event ERC7579TryExecuteFail(uint256 batchExecutionIndex, bytes result);
+    ExecType internal constant EXECTYPE_TRY = ExecType.wrap(0x01);
+
+    /**
+     * @dev Emits when an {EXECTYPE_TRY} execution fails.
+     * @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 returndata);
 
     /// @dev The provided {CallType} is not supported.
     error ERC7579UnsupportedCallType(CallType callType);

contracts/finance/VestingWallet.sol

@@ -26,6 +26,11 @@ import {Ownable} from "../access/Ownable.sol";
  *
  * NOTE: When using this contract with any token whose balance is adjusted automatically (i.e. a rebase token), make
  * sure to account the supply/balance adjustment in the vesting schedule to ensure the vested amount is as intended.
+ *
+ * NOTE: Chains with support for native ERC20s may allow the vesting wallet to withdraw the underlying asset as both an
+ * ERC20 and as native currency. For example, if chain C supports token A and the wallet gets deposited 100 A, then
+ * at 50% of the vesting period, the beneficiary can withdraw 50 A as ERC20 and 25 A as native currency (totaling 75 A).
+ * Consider disabling one of the withdrawal methods.
  */
 contract VestingWallet is Context, Ownable {
     event EtherReleased(uint256 amount);

contracts/governance/extensions/GovernorCountingOverridable.sol

@@ -8,8 +8,8 @@ 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
- * token token that inherits `VotesExtended`.
+ * @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 {
     bytes32 public constant OVERRIDE_BALLOT_TYPEHASH =
@@ -35,7 +35,10 @@ abstract contract GovernorCountingOverridable is GovernorVotes {
         mapping(address voter => VoteReceipt) voteReceipt;
     }
 
-    event VoteReduced(address indexed voter, uint256 proposalId, uint8 support, uint256 weight);
+    /// @dev The votes casted by `delegate` were reduced by `weight` after an override vote was casted by the original token holder
+    event VoteReduced(address indexed delegate, uint256 proposalId, uint8 support, uint256 weight);
+
+    /// @dev A delegated vote on `proposalId` was overridden by `weight`
     event OverrideVoteCast(address indexed voter, uint256 proposalId, uint8 support, uint256 weight, string reason);
 
     error GovernorAlreadyOverridenVote(address account);
@@ -52,6 +55,11 @@ abstract contract GovernorCountingOverridable is GovernorVotes {
 
     /**
      * @dev See {IGovernor-hasVoted}.
+     *
+     * NOTE: Calling {castVote} (or similar) casts a vote using the voting power that is delegated to the voter.
+     * Conversely, calling {castOverrideVote} (or similar) uses the voting power of the account itself, from its asset
+     * balances. Casting an "override vote" does not count as voting and won't be reflected by this getter. Consider
+     * using {hasVotedOverride} to check if an account has casted an "override vote" for a given proposal id.
      */
     function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) {
         return _proposalVotes[proposalId].voteReceipt[account].casted != 0;
@@ -120,7 +128,11 @@ abstract contract GovernorCountingOverridable is GovernorVotes {
         return totalWeight;
     }
 
-    /// @dev Variant of {Governor-_countVote} that deals with vote overrides.
+    /**
+     * @dev Variant of {Governor-_countVote} that deals with vote overrides.
+     *
+     * NOTE: See {hasVoted} for more details about the difference between {castVote} and {castOverrideVote}.
+     */
     function _countOverride(uint256 proposalId, address account, uint8 support) internal virtual returns (uint256) {
         ProposalVote storage proposalVote = _proposalVotes[proposalId];
 
@@ -132,9 +144,9 @@ abstract contract GovernorCountingOverridable is GovernorVotes {
             revert GovernorAlreadyOverridenVote(account);
         }
 
-        uint256 proposalSnapshot = proposalSnapshot(proposalId);
-        uint256 overridenWeight = VotesExtended(address(token())).getPastBalanceOf(account, proposalSnapshot);
-        address delegate = VotesExtended(address(token())).getPastDelegate(account, proposalSnapshot);
+        uint256 snapshot = proposalSnapshot(proposalId);
+        uint256 overridenWeight = VotesExtended(address(token())).getPastBalanceOf(account, snapshot);
+        address delegate = VotesExtended(address(token())).getPastDelegate(account, snapshot);
         uint8 delegateCasted = proposalVote.voteReceipt[delegate].casted;
 
         proposalVote.voteReceipt[account].hasOverriden = true;
@@ -150,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,
@@ -168,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,
@@ -178,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/governance/utils/Votes.sol

@@ -71,6 +71,15 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
         return "mode=blocknumber&from=default";
     }
 
+    /**
+     * @dev Validate that a timepoint is in the past, and return it as a uint48.
+     */
+    function _validateTimepoint(uint256 timepoint) internal view returns (uint48) {
+        uint48 currentTimepoint = clock();
+        if (timepoint >= currentTimepoint) revert ERC5805FutureLookup(timepoint, currentTimepoint);
+        return SafeCast.toUint48(timepoint);
+    }
+
     /**
      * @dev Returns the current amount of votes that `account` has.
      */
@@ -87,11 +96,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
      * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined.
      */
     function getPastVotes(address account, uint256 timepoint) public view virtual returns (uint256) {
-        uint48 currentTimepoint = clock();
-        if (timepoint >= currentTimepoint) {
-            revert ERC5805FutureLookup(timepoint, currentTimepoint);
-        }
-        return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
+        return _delegateCheckpoints[account].upperLookupRecent(_validateTimepoint(timepoint));
     }
 
     /**
@@ -107,11 +112,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
      * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined.
      */
     function getPastTotalSupply(uint256 timepoint) public view virtual returns (uint256) {
-        uint48 currentTimepoint = clock();
-        if (timepoint >= currentTimepoint) {
-            revert ERC5805FutureLookup(timepoint, currentTimepoint);
-        }
-        return _totalCheckpoints.upperLookupRecent(SafeCast.toUint48(timepoint));
+        return _totalCheckpoints.upperLookupRecent(_validateTimepoint(timepoint));
     }
 
     /**