Fix ERC721Consecutive balance update on batch size 1

Merge pull request from GHSA-878m-3g6q-594q Co-authored-by: Francisco Giordano <[email protected]> (cherry picked from commit 8ba26f3)

Author: Amxx

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## 4.8.2
+
+-`ERC721Consecutive`: Fixed a bug when `_mintConsecutive` is used for batches of size 1 that could lead to balance overflow. Refer to the breaking changes section in the changelog for a note on the behavior of `ERC721._beforeTokenTransfer`.
+
+### Breaking changes
+
+-`ERC721`: The internal function `_beforeTokenTransfer` no longer updates balances, which it previously did when `batchSize` was greater than 1. This change has no consequence unless a custom ERC721 extension is explicitly invoking `_beforeTokenTransfer`. Balance updates in extensions must now be done explicitly using `__unsafe_increaseBalance`, with a name that indicates that there is an invariant that has to be manually verified.
+
 ## 4.8.1 (2023-01-13)
 
 *`ERC4626`: Use staticcall instead of call when fetching underlying ERC-20 decimals. ([#3943](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3943))

contracts/token/ERC721/ERC721.sol

@@ -467,18 +467,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata{
 function _beforeTokenTransfer(
 addressfrom,
 addressto,
-uint256, /* firstTokenId */
+uint256firstTokenId,
 uint256batchSize
- ) internalvirtual{
-if (batchSize >1){
-if (from !=address(0)){
- _balances[from] -= batchSize;
- }
-if (to !=address(0)){
- _balances[to] += batchSize;
- }
- }
- }
+ ) internalvirtual{}
 
 /**
  * @dev Hook that is called after any token transfer. This includes minting and burning. If{ERC721Consecutive} is
@@ -500,4 +491,16 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata{
 uint256firstTokenId,
 uint256batchSize
  ) internalvirtual{}
+
+/**
+ * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an{ownerOf} override.
+ *
+ * WARNING: Anyone calling this MUST ensure that the balances remain consistent with the ownership. The invariant
+ * being that for any address `a` the value returned by `balanceOf(a)` must be equal to the number of tokens such
+ * that `ownerOf(tokenId)` is `a`.
+ */
+// solhint-disable-next-line func-name-mixedcase
+function __unsafe_increaseBalance(addressaccount, uint256amount) internal{
+ _balances[account] += amount;
+ }
 }

contracts/token/ERC721/extensions/ERC721Consecutive.sol

@@ -96,6 +96,11 @@ abstract contract ERC721Consecutive is IERC2309, ERC721{
 // push an ownership checkpoint & emit event
 uint96 last = first + batchSize -1;
  _sequentialOwnership.push(last, uint160(to));
+
+// The invariant required by this function is preserved because the new sequentialOwnership checkpoint
+// is attributing ownership of `batchSize` new tokens to account `to`.
+__unsafe_increaseBalance(to, batchSize);
+
 emitConsecutiveTransfer(first, last, address(0), to);
 
 // hook after

test/token/ERC721/extensions/ERC721Consecutive.t.sol

@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity^0.8.0;
+
+import"../../../../contracts/token/ERC721/extensions/ERC721Consecutive.sol";
+import"forge-std/Test.sol";
+
+function toSingleton(addressaccount) purereturns (address[] memory){
+address[] memory accounts =newaddress[](1);
+ accounts[0] = account;
+return accounts;
+}
+
+contractERC721ConsecutiveTargetisStdUtils, ERC721Consecutive{
+uint256public totalMinted =0;
+
+constructor(address[] memoryreceivers, uint256[] memorybatches) ERC721("", ""){
+for (uint256 i =0; i < batches.length; i++){
+address receiver = receivers[i % receivers.length];
+uint96 batchSize =uint96(bound(batches[i], 0, _maxBatchSize()));
+_mintConsecutive(receiver, batchSize);
+ totalMinted += batchSize;
+ }
+ }
+
+function burn(uint256tokenId) public{
+_burn(tokenId);
+ }
+}
+
+contractERC721ConsecutiveTestisTest{
+function test_balance(addressreceiver, uint256[] calldatabatches) public{
+ vm.assume(receiver !=address(0));
+
+ERC721ConsecutiveTarget token =newERC721ConsecutiveTarget(toSingleton(receiver), batches);
+
+assertEq(token.balanceOf(receiver), token.totalMinted());
+ }
+
+function test_ownership(addressreceiver, uint256[] calldatabatches, uint256[2] calldataunboundedTokenId) public{
+ vm.assume(receiver !=address(0));
+
+ERC721ConsecutiveTarget token =newERC721ConsecutiveTarget(toSingleton(receiver), batches);
+
+if (token.totalMinted() >0){
+uint256 validTokenId =bound(unboundedTokenId[0], 0, token.totalMinted() -1);
+assertEq(token.ownerOf(validTokenId), receiver);
+ }
+
+uint256 invalidTokenId =bound(unboundedTokenId[1], token.totalMinted(), type(uint256).max);
+ vm.expectRevert();
+ token.ownerOf(invalidTokenId);
+ }
+
+function test_burn(addressreceiver, uint256[] calldatabatches, uint256unboundedTokenId) public{
+ vm.assume(receiver !=address(0));
+
+ERC721ConsecutiveTarget token =newERC721ConsecutiveTarget(toSingleton(receiver), batches);
+
+// only test if we minted at least one token
+uint256 supply = token.totalMinted();
+ vm.assume(supply >0);
+
+// burn a token in [0; supply[
+uint256 tokenId =bound(unboundedTokenId, 0, supply -1);
+ token.burn(tokenId);
+
+// balance should have decreased
+assertEq(token.balanceOf(receiver), supply -1);
+
+// token should be burnt
+ vm.expectRevert();
+ token.ownerOf(tokenId);
+ }
+
+function test_transfer(
+address[2] calldataaccounts,
+uint256[2] calldataunboundedBatches,
+uint256[2] calldataunboundedTokenId
+ ) public{
+ vm.assume(accounts[0] !=address(0));
+ vm.assume(accounts[1] !=address(0));
+ vm.assume(accounts[0] != accounts[1]);
+
+address[] memory receivers =newaddress[](2);
+ receivers[0] = accounts[0];
+ receivers[1] = accounts[1];
+
+// We assume _maxBatchSize is 5000 (the default). This test will break otherwise.
+uint256[] memory batches =newuint256[](2);
+ batches[0] =bound(unboundedBatches[0], 1, 5000);
+ batches[1] =bound(unboundedBatches[1], 1, 5000);
+
+ERC721ConsecutiveTarget token =newERC721ConsecutiveTarget(receivers, batches);
+
+uint256 tokenId0 =bound(unboundedTokenId[0], 0, batches[0] -1);
+uint256 tokenId1 =bound(unboundedTokenId[1], 0, batches[1] -1) + batches[0];
+
+assertEq(token.ownerOf(tokenId0), accounts[0]);
+assertEq(token.ownerOf(tokenId1), accounts[1]);
+assertEq(token.balanceOf(accounts[0]), batches[0]);
+assertEq(token.balanceOf(accounts[1]), batches[1]);
+
+ vm.prank(accounts[0]);
+ token.transferFrom(accounts[0], accounts[1], tokenId0);
+
+assertEq(token.ownerOf(tokenId0), accounts[1]);
+assertEq(token.ownerOf(tokenId1), accounts[1]);
+assertEq(token.balanceOf(accounts[0]), batches[0] -1);
+assertEq(token.balanceOf(accounts[1]), batches[1] +1);
+
+ vm.prank(accounts[1]);
+ token.transferFrom(accounts[1], accounts[0], tokenId1);
+
+assertEq(token.ownerOf(tokenId0), accounts[1]);
+assertEq(token.ownerOf(tokenId1), accounts[0]);
+assertEq(token.balanceOf(accounts[0]), batches[0]);
+assertEq(token.balanceOf(accounts[1]), batches[1]);
+ }
+}

test/token/ERC721/extensions/ERC721Consecutive.test.js

@@ -12,7 +12,8 @@ contract('ERC721Consecutive', function (accounts){
 constsymbol='NFT';
 constbatches=[
 {receiver: user1,amount: 0},
-{receiver: user1,amount: 3},
+{receiver: user1,amount: 1},
+{receiver: user1,amount: 2},
 {receiver: user2,amount: 5},
 {receiver: user3,amount: 0},
 {receiver: user1,amount: 7},