Merge branch 'master' into optimize-clones

.github/workflows/checks.yml

@@ -76,3 +76,14 @@ jobs:
       - name: Set up environment
         uses: ./.github/actions/setup
       - uses: crytic/slither-action@v0.1.1
+
+  codespell:
+    if: github.repository != 'OpenZeppelin/openzeppelin-contracts-upgradeable'
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Run CodeSpell
+        uses: codespell-project/actions-codespell@v1.0
+        with:
+          check_filenames: true
+          skip: package-lock.json

CHANGELOG.md

@@ -12,11 +12,13 @@
  * `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481))
  * `ERC721`: optimize burn by making approval clearing implicit instead of emitting an event. ([#3538](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3538))
  * `ERC721`: Fix balance accounting when a custom `_beforeTokenTransfer` hook results in a transfer of the token under consideration. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611))
+ * `ERC721`: use unchecked arithmetic for balance updates. ([#3524](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3524))
  * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515))
  * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565))
  * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605))
  * `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591))
  * `VestingWallet`: add `releasable` getters. ([#3580](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3580))
+ * `Create2`: optimize address computation by using assembly instead of `abi.encodePacked`. ([#3600](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3600))
  * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use lesser operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640))
 
 ### Deprecations
@@ -549,7 +551,7 @@ Refer to the table below to adjust your inheritance list.
  * Now conforming to a 4-space indentation code style. ([1508](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1508))
  * `ERC20`: more gas efficient due to removed redundant `require`s. ([#1409](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1409))
  * `ERC721`: fixed a bug that prevented internal data structures from being properly cleaned, missing potential gas refunds. ([#1539](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1539) and [#1549](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1549))
- * `ERC721`: general gas savings on `transferFrom`, `_mint` and `_burn`, due to redudant `require`s and `SSTORE`s. ([#1549](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1549))
+ * `ERC721`: general gas savings on `transferFrom`, `_mint` and `_burn`, due to redundant `require`s and `SSTORE`s. ([#1549](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1549))
 
 ### Bugfixes
 

certora/specs/GovernorBase.spec

@@ -98,26 +98,26 @@ function helperFunctionsWithRevert(uint256 proposalId, method f, env e) {
  // To use env with general preserved block disable type checking [--disableLocalTypeChecking]
 invariant startAndEndDatesNonZero(uint256 pId)
         proposalSnapshot(pId) != 0 <=> proposalDeadline(pId) != 0
-        { preserved with (env e){   
+        { preserved with (env e){
                 require e.block.number > 0;
         }}
-        
+
 
 /*
- * If a proposal is canceled it must have a start and an end date 
+ * If a proposal is canceled it must have a start and an end date
  */
  // To use env with general preserved block disable type checking [--disableLocalTypeChecking]
 invariant canceledImplyStartAndEndDateNonZero(uint pId)
         isCanceled(pId) => proposalSnapshot(pId) != 0
-        {preserved with (env e){               
+        {preserved with (env e){
                 require e.block.number > 0;
         }}
 
 
 /*
- * If a proposal is executed it must have a start and an end date 
+ * If a proposal is executed it must have a start and an end date
  */
- // To use env with general preserved block disable type checking [--disableLocalTypeChecking] 
+ // To use env with general preserved block disable type checking [--disableLocalTypeChecking]
 invariant executedImplyStartAndEndDateNonZero(uint pId)
         isExecuted(pId) => proposalSnapshot(pId) != 0
         { preserved with (env e){
@@ -143,7 +143,7 @@ invariant voteStartBeforeVoteEnd(uint256 pId)
 /*
  * A proposal cannot be both executed and canceled simultaneously.
  */
-invariant noBothExecutedAndCanceled(uint256 pId) 
+invariant noBothExecutedAndCanceled(uint256 pId)
         !isExecuted(pId) || !isCanceled(pId)
 
 
@@ -154,10 +154,10 @@ rule executionOnlyIfQuoromReachedAndVoteSucceeded(uint256 pId, env e, method f){
     bool isExecutedBefore = isExecuted(pId);
     bool quorumReachedBefore = _quorumReached(e, pId);
     bool voteSucceededBefore = _voteSucceeded(pId);
-    
+
     calldataarg args;
     f(e, args);
-    
+
     bool isExecutedAfter = isExecuted(pId);
     assert (!isExecutedBefore && isExecutedAfter) => (quorumReachedBefore && voteSucceededBefore), "quorum was changed";
 }
@@ -177,16 +177,16 @@ rule executionOnlyIfQuoromReachedAndVoteSucceeded(uint256 pId, env e, method f){
  // the fact that the 3 functions themselves makes no changes, but rather call an internal function to execute.
  // That means that we do not check those 3 functions directly, however for castVote & castVoteWithReason it is quite trivial
  // to understand why this is ok. For castVoteBySig we basically assume that the signature referendum is correct without checking it.
- // We could check each function separately and pass the rule, but that would have uglyfied the code with no concrete 
+ // We could check each function separately and pass the rule, but that would have uglyfied the code with no concrete
  // benefit, as it is evident that nothing is happening in the first 2 functions (calling a view function), and we do not desire to check the signature verification.
 rule doubleVoting(uint256 pId, uint8 sup, method f) {
     env e;
-    address user = e.msg.sender;        
+    address user = e.msg.sender;
     bool votedCheck = hasVoted(e, pId, user);
 
     castVote@withrevert(e, pId, sup);
 
-    assert votedCheck => lastReverted, "double voting accured";
+    assert votedCheck => lastReverted, "double voting occurred";
 }
 
 
@@ -207,7 +207,7 @@ rule immutableFieldsAfterProposalCreation(uint256 pId, method f) {
     uint256 _voteEnd = proposalDeadline(pId);
 
     require proposalCreated(pId); // startDate > 0
-    
+
     env e; calldataarg arg;
     f(e, arg);
 
@@ -226,7 +226,7 @@ rule noStartBeforeCreation(uint256 pId) {
     // This line makes sure that we see only cases where start date is changed from 0, i.e. creation of proposal
     // We proved in immutableFieldsAfterProposalCreation that once dates set for proposal, it cannot be changed
     require !proposalCreated(pId); // previousStart == 0;
-    
+
     env e; calldataarg args;
     propose(e, args);
 
@@ -273,7 +273,7 @@ rule noExecuteOrCancelBeforeDeadline(uint256 pId, method f){
  * All proposal specific (non-view) functions should revert if proposal is executed
  */
  // In this rule we show that if a function is executed, i.e. execute() was called on the proposal ID,
- // non of the proposal specific functions can make changes again. In executedOnlyAfterExecuteFunc 
+ // non of the proposal specific functions can make changes again. In executedOnlyAfterExecuteFunc
  // we connected the executed attribute to the execute() function, showing that only execute() can
  // change it, and that it will always change it.
 rule allFunctionsRevertIfExecuted(method f) filtered { f ->
@@ -331,4 +331,3 @@ rule executedOnlyAfterExecuteFunc(address[] targets, uint256[] values, bytes[] c
     bool executedAfter = isExecuted(pId);
     assert(executedAfter != executedBefore => f.selector == execute(address[], uint256[], bytes[], bytes32).selector, "isExecuted only changes in the execute method");
 }
-

contracts/governance/extensions/GovernorVotesQuorumFraction.sol

@@ -55,7 +55,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
             return latest._value;
         }
 
-        // Otherwize, do the binary search
+        // Otherwise, do the binary search
         return _quorumNumeratorHistory.getAtBlock(blockNumber);
     }
 

contracts/token/ERC20/extensions/ERC4626.sol

@@ -147,7 +147,7 @@ abstract contract ERC4626 is ERC20, IERC4626 {
      * @dev Internal conversion function (from assets to shares) with support for rounding direction.
      *
      * Will revert if assets > 0, totalSupply > 0 and totalAssets = 0. That corresponds to a case where any asset
-     * would represent an infinite amout of shares.
+     * would represent an infinite amount of shares.
      */
     function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
         uint256 supply = totalSupply();
@@ -182,7 +182,7 @@ abstract contract ERC4626 is ERC20, IERC4626 {
         // calls the vault, which is assumed not malicious.
         //
         // Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
-        // assets are transfered and before the shares are minted, which is a valid state.
+        // assets are transferred and before the shares are minted, which is a valid state.
         // slither-disable-next-line reentrancy-no-eth
         SafeERC20.safeTransferFrom(_asset, caller, address(this), assets);
         _mint(receiver, shares);
@@ -209,7 +209,7 @@ abstract contract ERC4626 is ERC20, IERC4626 {
         // calls the vault, which is assumed not malicious.
         //
         // Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
-        // shares are burned and after the assets are transfered, which is a valid state.
+        // shares are burned and after the assets are transferred, which is a valid state.
         _burn(owner, shares);
         SafeERC20.safeTransfer(_asset, receiver, assets);
 

contracts/token/ERC721/ERC721.sol

@@ -285,7 +285,14 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata {
         // Check that tokenId was not minted by `_beforeTokenTransfer` hook
         require(!_exists(tokenId), "ERC721: token already minted");
 
-        _balances[to] += 1;
+        unchecked {
+            // Will not overflow unless all 2**256 token ids are minted to the same owner.
+            // Given that tokens are minted one by one, it is impossible in practice that
+            // this ever happens. Might change if we allow batch minting.
+            // The ERC fails to describe this case.
+            _balances[to] += 1;
+        }
+
         _owners[tokenId] = to;
 
         emit Transfer(address(0), to, tokenId);
@@ -309,13 +316,17 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata {
 
         _beforeTokenTransfer(owner, address(0), tokenId);
 
-        // Update ownership in case tokenId was transfered by `_beforeTokenTransfer` hook
+        // Update ownership in case tokenId was transferred by `_beforeTokenTransfer` hook
         owner = ERC721.ownerOf(tokenId);
 
         // Clear approvals
         delete _tokenApprovals[tokenId];
 
-        _balances[owner] -= 1;
+        unchecked {
+            // Cannot overflow, as that would require more tokens to be burned/transferred
+            // out than the owner initially received through minting and transferring in.
+            _balances[owner] -= 1;
+        }
         delete _owners[tokenId];
 
         emit Transfer(owner, address(0), tokenId);
@@ -344,14 +355,21 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata {
 
         _beforeTokenTransfer(from, to, tokenId);
 
-        // Check that tokenId was not transfered by `_beforeTokenTransfer` hook
+        // Check that tokenId was not transferred by `_beforeTokenTransfer` hook
         require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner");
 
         // Clear approvals from the previous owner
         delete _tokenApprovals[tokenId];
 
-        _balances[from] -= 1;
-        _balances[to] += 1;
+        unchecked {
+            // `_balances[from]` cannot overflow for the same reason as described in `_burn`:
+            // `from`'s balance is the number of token held, which is at least one before the current
+            // transfer.
+            // `_balances[to]` could overflow in the conditions described in `_mint`. That would require
+            // all 2**256 token ids to be minted, which in practice is impossible.
+            _balances[from] -= 1;
+            _balances[to] += 1;
+        }
         _owners[tokenId] = to;
 
         emit Transfer(from, to, tokenId);

contracts/utils/Create2.sol

@@ -31,16 +31,14 @@ library Create2 {
         uint256 amount,
         bytes32 salt,
         bytes memory bytecode
-    ) internal returns (address) {
-        address addr;
+    ) internal returns (address addr) {
         require(address(this).balance >= amount, "Create2: insufficient balance");
         require(bytecode.length != 0, "Create2: bytecode length is zero");
         /// @solidity memory-safe-assembly
         assembly {
             addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt)
         }
         require(addr != address(0), "Create2: Failed on deploy");
-        return addr;
     }
 
     /**
@@ -59,8 +57,27 @@ library Create2 {
         bytes32 salt,
         bytes32 bytecodeHash,
         address deployer
-    ) internal pure returns (address) {
-        bytes32 _data = keccak256(abi.encodePacked(bytes1(0xff), deployer, salt, bytecodeHash));
-        return address(uint160(uint256(_data)));
+    ) internal pure returns (address addr) {
+        /// @solidity memory-safe-assembly
+        assembly {
+            let ptr := mload(0x40) // Get free memory pointer
+
+            // |                   | ↓ ptr ...  ↓ ptr + 0x0B (start) ...  ↓ ptr + 0x20 ...  ↓ ptr + 0x40 ...   |
+            // |-------------------|---------------------------------------------------------------------------|
+            // | bytecodeHash      |                                                        CCCCCCCCCCCCC...CC |
+            // | salt              |                                      BBBBBBBBBBBBB...BB                   |
+            // | deployer          | 000000...0000AAAAAAAAAAAAAAAAAAA...AA                                     |
+            // | 0xFF              |            FF                                                             |
+            // |-------------------|---------------------------------------------------------------------------|
+            // | memory            | 000000...00FFAAAAAAAAAAAAAAAAAAA...AABBBBBBBBBBBBB...BBCCCCCCCCCCCCC...CC |
+            // | keccak(start, 85) |            ↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑ |
+
+            mstore(add(ptr, 0x40), bytecodeHash)
+            mstore(add(ptr, 0x20), salt)
+            mstore(ptr, deployer) // Right-aligned with 12 preceding garbage bytes
+            let start := add(ptr, 0x0b) // The hashed data starts at the final garbage byte which we will set to 0xff
+            mstore8(start, 0xff)
+            addr := keccak256(start, 85)
+        }
     }
 }

test/token/ERC20/extensions/ERC20FlashMint.test.js

@@ -97,7 +97,7 @@ contract('ERC20FlashMint', function (accounts) {
       const receiverInitialBalance = new BN(200000);
       const flashFee = new BN(5000);
 
-      beforeEach('init reciever balance & set flash fee', async function () {
+      beforeEach('init receiver balance & set flash fee', async function () {
         this.receiver = await ERC3156FlashBorrowerMock.new(true, true);
         const receipt = await this.token.mint(this.receiver.address, receiverInitialBalance);
         await expectEvent(receipt, 'Transfer', { from: ZERO_ADDRESS, to: this.receiver.address, value: receiverInitialBalance });