From 403792ed0bd1c17d61dbbc2feebe5437af2dfbfb Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:01:36 -0400 Subject: [PATCH 01/15] mitigation: fix reentry (#25) --- src/NFTMint.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 77a7623..906c2ce 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -238,7 +238,7 @@ contract NFTMint is Ownable { numOrdersToFill == 0 ? orders.length : Math.min(orders.length, nextOrderIdToFill_ + numOrdersToFill); while (nextOrderIdToFill_ < finalNextOrderToFill) { - Order storage currentOrder = orders[nextOrderIdToFill_]; + Order memory currentOrder = orders[nextOrderIdToFill_]; if (msg.sender != owner() && currentOrder.orderTimestamp + 1 hours > block.timestamp) { // Only the owner can fill orders that are less than 1 hour old break; @@ -287,10 +287,10 @@ contract NFTMint is Ownable { mstore(amounts, numNonZero) } - // If the mint fails with 500_000 gas, the order is still marked as filled. - try currentOrder.mint.mintBatch{ gas: 500_000 }(currentOrder.to, ids, amounts) { } catch { } delete orders[nextOrderIdToFill_]; nextOrderIdToFill_++; + // If the mint fails with 500_000 gas, the order is still marked as filled. + try currentOrder.mint.mintBatch{ gas: 500_000 }(currentOrder.to, ids, amounts) { } catch { } } nextOrderIdToFill = uint96(nextOrderIdToFill_); From b2e82a43defaf3d4b357416edb0c42e5f184ae5e Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:04:22 -0400 Subject: [PATCH 02/15] mitigation: remove extra arity check (#24) --- src/MintERC1155.sol | 4 ---- test/MintERC1155.t.sol | 5 ----- 2 files changed, 9 deletions(-) diff --git a/src/MintERC1155.sol b/src/MintERC1155.sol index 0d247ef..1be78bd 100644 --- a/src/MintERC1155.sol +++ b/src/MintERC1155.sol @@ -10,7 +10,6 @@ import { LibString } from "solady/src/utils/LibString.sol"; /// @custom:security-contact security@partydao.org contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeable { error MintERC1155_Unauthorized(); - error MintERC1155_ArityMismatch(); error MintERC1155_TotalPercentChanceNot100(); error MintERC1155_PercentChance0(); @@ -97,9 +96,6 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab if (msg.sender != MINTER) { revert MintERC1155_Unauthorized(); } - if (ids.length != amounts.length) { - revert MintERC1155_ArityMismatch(); - } _mintBatch(to, ids, amounts, ""); } diff --git a/test/MintERC1155.t.sol b/test/MintERC1155.t.sol index 6638128..eae954f 100644 --- a/test/MintERC1155.t.sol +++ b/test/MintERC1155.t.sol @@ -93,11 +93,6 @@ contract MintERC1155Test is TestBase, LintJSON { token.mintBatch(address(this), new uint256[](0), new uint256[](0)); } - function test_mintBatch_arityMismatch() external { - vm.expectRevert(MintERC1155.MintERC1155_ArityMismatch.selector); - token.mintBatch(address(this), new uint256[](1), new uint256[](0)); - } - function test_totalEditions() external view { assertEq(token.totalEditions(), 2); } From ade3e0517944564700fd50921c8ba1a0533ba3e9 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:06:53 -0400 Subject: [PATCH 03/15] mitigation: bound editions length to max of 25 (#30) * mitigation: bound editions length to max of 25 * fix lint --- src/MintERC1155.sol | 9 ++++++++- test/MintERC1155.t.sol | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/MintERC1155.sol b/src/MintERC1155.sol index 1be78bd..81ed7df 100644 --- a/src/MintERC1155.sol +++ b/src/MintERC1155.sol @@ -12,6 +12,7 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab error MintERC1155_Unauthorized(); error MintERC1155_TotalPercentChanceNot100(); error MintERC1155_PercentChance0(); + error MintERC1155_ExcessEditions(); event ContractURIUpdated(); @@ -64,6 +65,10 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab initializer { { + if (editions_.length > 25) { + revert MintERC1155_ExcessEditions(); + } + uint256 totalPercentChance = 0; for (uint256 i = 0; i < editions_.length; i++) { editions.push(); @@ -149,7 +154,9 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab return json; } - function supportsInterface(bytes4 interfaceId) + function supportsInterface( + bytes4 interfaceId + ) public view virtual diff --git a/test/MintERC1155.t.sol b/test/MintERC1155.t.sol index eae954f..a41bb18 100644 --- a/test/MintERC1155.t.sol +++ b/test/MintERC1155.t.sol @@ -39,6 +39,20 @@ contract MintERC1155Test is TestBase, LintJSON { event ContractURIUpdated(); + function test_initialize_excessEditions() external { + MintERC1155 impl = new MintERC1155(address(this)); + + MintERC1155.Attribute[] memory attributes = new MintERC1155.Attribute[](1); + attributes[0] = MintERC1155.Attribute({ traitType: "traitType", value: "value" }); + + MintERC1155.Edition[] memory editions = new MintERC1155.Edition[](26); + + token = MintERC1155(Clones.clone(address(impl))); + + vm.expectRevert(MintERC1155.MintERC1155_ExcessEditions.selector); + token.initialize(address(this), "My Token Name", "image here", "This is a token", editions, 150); + } + function test_setContractInfo() external { vm.expectEmit(true, true, true, true); emit ContractURIUpdated(); From 9a7c94074b4d889c3494391574f8ef2a9a511f5f Mon Sep 17 00:00:00 2001 From: Brian Le Date: Mon, 12 Aug 2024 12:12:33 -0700 Subject: [PATCH 04/15] mitigation: check `onERC1155Received()` (#28) * check `onERC1155Received()` * fix redeclared vars --- src/MintERC1155.sol | 37 +++++++++++++++++++++++++------------ src/NFTMint.sol | 2 +- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/MintERC1155.sol b/src/MintERC1155.sol index 81ed7df..f013508 100644 --- a/src/MintERC1155.sol +++ b/src/MintERC1155.sol @@ -202,25 +202,38 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab * @notice Check if the given address can receive tokens from this contract * @param to Address to check if receiving tokens is safe */ - function safeBatchTransferAcceptanceCheckOnMint(address to) external view returns (bool) { + function safeTransferAcceptanceCheckOnMint(address to) external view returns (bool) { + if (to.code.length == 0) return true; + + (bool success, bytes memory res) = to.staticcall{ gas: 400_000 }( + abi.encodeCall(IERC1155Receiver.onERC1155Received, (MINTER, address(0), 1, 1, "")) + ); + if (success) { + bytes4 response = abi.decode(res, (bytes4)); + if (response != IERC1155Receiver.onERC1155Received.selector) { + return false; + } + } else { + return false; + } + uint256[] memory idOrAmountArray = new uint256[](1); idOrAmountArray[0] = 1; - bytes memory callData = abi.encodeCall( - IERC1155Receiver.onERC1155BatchReceived, (MINTER, address(0), idOrAmountArray, idOrAmountArray, "") + (success, res) = to.staticcall{ gas: 400_000 }( + abi.encodeCall( + IERC1155Receiver.onERC1155BatchReceived, (MINTER, address(0), idOrAmountArray, idOrAmountArray, "") + ) ); - - if (to.code.length > 0) { - (bool success, bytes memory res) = to.staticcall{ gas: 400_000 }(callData); - if (success) { - bytes4 response = abi.decode(res, (bytes4)); - if (response != IERC1155Receiver.onERC1155BatchReceived.selector) { - return false; - } - } else { + if (success) { + bytes4 response = abi.decode(res, (bytes4)); + if (response != IERC1155Receiver.onERC1155BatchReceived.selector) { return false; } + } else { + return false; } + return true; } diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 906c2ce..8981013 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -194,7 +194,7 @@ contract NFTMint is Ownable { } } - if (!mint.safeBatchTransferAcceptanceCheckOnMint(msg.sender)) { + if (!mint.safeTransferAcceptanceCheckOnMint(msg.sender)) { revert NFTMint_BuyerNotAcceptingERC1155(); } From 7f14d0a856d098e347461a50965e72d1a3e8482a Mon Sep 17 00:00:00 2001 From: Brian Le Date: Mon, 12 Aug 2024 12:15:26 -0700 Subject: [PATCH 05/15] prevent order if no remaining mint (#29) --- src/NFTMint.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 8981013..4cbc82e 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -164,10 +164,6 @@ contract NFTMint is Ownable { external payable { - if (amount == 0 || amount > 100) { - revert NFTMint_InvalidAmount(); - } - MintInfo storage mintInfo = mints[mint]; if (mintInfo.mintExpiration < block.timestamp) { @@ -175,6 +171,10 @@ contract NFTMint is Ownable { } uint32 modifiedAmount = uint32(Math.min(amount, mintInfo.remainingMints)); + if (modifiedAmount == 0 || modifiedAmount > 100) { + revert NFTMint_InvalidAmount(); + } + mintInfo.remainingMints -= modifiedAmount; uint256 totalCost = (mintInfo.pricePerMint + mintInfo.feePerMint) * modifiedAmount; From f6d71a0fdedd3da91288d35275eaf97b7c95d5bb Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:17:11 -0400 Subject: [PATCH 06/15] mitigation: dynamically fetch owner (#33) * mitigation: dynamically fetch owner * fix lint --- src/NFTMint.sol | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 4cbc82e..31825a3 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -72,8 +72,6 @@ contract NFTMint is Ownable { uint32 perWalletLimit; // Timestamp when the mint expires uint40 mintExpiration; - // Address of the owner of the mint - address payable owner; // Address to receive the fee address payable feeRecipient; // Merkle root for the allowlist @@ -135,7 +133,6 @@ contract NFTMint is Ownable { newMint.initialize(args.owner, args.name, args.imageURI, args.description, args.editions, args.royaltyAmountBps); MintInfo storage mintInfo = mints[newMint]; - mintInfo.owner = args.owner; mintInfo.remainingMints = args.maxMints; mintInfo.pricePerMint = args.pricePerMint; mintInfo.feePerMint = args.feePerMint; @@ -211,7 +208,7 @@ contract NFTMint is Ownable { bool mintProceedsSuccess = true; if (mintInfo.pricePerMint > 0) { (mintProceedsSuccess,) = - mintInfo.owner.call{ value: mintInfo.pricePerMint * modifiedAmount, gas: 100_000 }(""); + mint.owner().call{ value: mintInfo.pricePerMint * modifiedAmount, gas: 100_000 }(""); } bool refundSuccess = true; if (msg.value > totalCost) { From bbc55dfec5af441fe7e34fe179708327d2e1bdca Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:18:59 -0400 Subject: [PATCH 07/15] mitigation: store next nonce (#23) --- src/NFTMint.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 31825a3..e3c89a5 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -96,6 +96,9 @@ contract NFTMint is Ownable { /// @notice Next order ID to fill in the `orders` array. All orders before this index have been filled. uint96 public nextOrderIdToFill; + /// @notice Next nonce to use for random number generation + uint96 private _nextNonce; + /// @notice Mapping of mint to mint information mapping(MintERC1155 => MintInfo) public mints; /// @notice Array of all orders placed. Filled orders are deleted. Order[] public orders; @@ -229,7 +232,7 @@ contract NFTMint is Ownable { * @param numOrdersToFill The maximum number of orders to fill. Specify 0 to fill all orders. */ function fillOrders(uint96 numOrdersToFill) external { - uint256 nonce = 0; + uint256 nonce = _nextNonce; uint256 nextOrderIdToFill_ = nextOrderIdToFill; uint256 finalNextOrderToFill = numOrdersToFill == 0 ? orders.length : Math.min(orders.length, nextOrderIdToFill_ + numOrdersToFill); @@ -291,6 +294,7 @@ contract NFTMint is Ownable { } nextOrderIdToFill = uint96(nextOrderIdToFill_); + _nextNonce = uint96(nonce++); } function VERSION() external pure returns (string memory) { From a12a2c225c11d18bdb764b179be8a0f031156c9d Mon Sep 17 00:00:00 2001 From: Brian Le Date: Mon, 12 Aug 2024 15:19:47 -0400 Subject: [PATCH 08/15] add tests for `safeTransferAcceptanceCheckOnMint()` --- test/MintERC1155.t.sol | 30 ++++++++++++++++++++++++++++++ utils/EmptyContract.sol | 4 ++++ utils/MockERC1155Receiver.sol | 30 ++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+) create mode 100644 utils/EmptyContract.sol create mode 100644 utils/MockERC1155Receiver.sol diff --git a/test/MintERC1155.t.sol b/test/MintERC1155.t.sol index a41bb18..e5584df 100644 --- a/test/MintERC1155.t.sol +++ b/test/MintERC1155.t.sol @@ -5,6 +5,8 @@ import { TestBase } from "./util/TestBase.t.sol"; import { MintERC1155 } from "src/MintERC1155.sol"; import { Clones } from "@openzeppelin/contracts/proxy/Clones.sol"; import { LintJSON } from "./util/LintJSON.t.sol"; +import { MockERC1155Receiver } from "utils/MockERC1155Receiver.sol"; +import { EmptyContract } from "utils/EmptyContract.sol"; contract MintERC1155Test is TestBase, LintJSON { MintERC1155 token; @@ -115,4 +117,32 @@ contract MintERC1155Test is TestBase, LintJSON { _lintJSON(token.uri(1)); _lintJSON(token.uri(2)); } + + function test_safeTransferAcceptanceCheckOnMint_single() external { + // Receiver should accept ERC1155 tokens + address receiver = address(new MockERC1155Receiver()); + assertTrue(token.safeTransferAcceptanceCheckOnMint(receiver)); + + // Non-receiver should not accept ERC1155 tokens + address nonReceiver = address(new EmptyContract()); + assertFalse(token.safeTransferAcceptanceCheckOnMint(nonReceiver)); + + // EOA should accept ERC1155 tokens + address eoa = vm.addr(1); + assertTrue(token.safeTransferAcceptanceCheckOnMint(eoa)); + } + + function test_safeTransferAcceptanceCheckOnMint_batch() external { + // Receiver should accept ERC1155 tokens + address receiver = address(new MockERC1155Receiver()); + assertTrue(token.safeTransferAcceptanceCheckOnMint(receiver)); + + // Non-receiver should not accept ERC1155 tokens + address nonReceiver = address(new EmptyContract()); + assertFalse(token.safeTransferAcceptanceCheckOnMint(nonReceiver)); + + // EOA should accept ERC1155 tokens + address eoa = vm.addr(1); + assertTrue(token.safeTransferAcceptanceCheckOnMint(eoa)); + } } diff --git a/utils/EmptyContract.sol b/utils/EmptyContract.sol new file mode 100644 index 0000000..3caa945 --- /dev/null +++ b/utils/EmptyContract.sol @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; + +contract EmptyContract { } diff --git a/utils/MockERC1155Receiver.sol b/utils/MockERC1155Receiver.sol new file mode 100644 index 0000000..ebc58af --- /dev/null +++ b/utils/MockERC1155Receiver.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; + +import { IERC1155Receiver } from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; + +contract MockERC1155Receiver is IERC1155Receiver { + function onERC1155Received( + address, + address, + uint256, + uint256, + bytes calldata + ) external pure override returns (bytes4) { + return this.onERC1155Received.selector; + } + + function onERC1155BatchReceived( + address, + address, + uint256[] calldata, + uint256[] calldata, + bytes calldata + ) external pure override returns (bytes4) { + return this.onERC1155BatchReceived.selector; + } + + function supportsInterface(bytes4 interfaceId) external pure override returns (bool) { + return interfaceId == type(IERC1155Receiver).interfaceId; + } +} From cd465c02f9704886fd779d9d06403a2629bd7a1b Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:25:40 -0400 Subject: [PATCH 09/15] mitigation: get only percentage (#31) --- src/MintERC1155.sol | 12 ++++++++++++ src/NFTMint.sol | 14 +++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/MintERC1155.sol b/src/MintERC1155.sol index f013508..28b63a1 100644 --- a/src/MintERC1155.sol +++ b/src/MintERC1155.sol @@ -116,6 +116,18 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab return allEditions; } + /** + * @notice Get the percent chance of each edition. Used for filling orders. + * @return An ordered array of the percent chance of each edition. + */ + function getPercentChances() external view returns (uint256[] memory) { + uint256[] memory percentChances = new uint256[](editions.length); + for (uint256 i = 0; i < editions.length; i++) { + percentChances[i] = editions[i].percentChance; + } + return percentChances; + } + function uri(uint256 tokenId) public view override returns (string memory) { Edition memory edition = editions[tokenId - 1]; diff --git a/src/NFTMint.sol b/src/NFTMint.sol index e3c89a5..02497c0 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -247,12 +247,12 @@ contract NFTMint is Ownable { // Don't fill orders in the same block to ensure there is randomness break; } - MintERC1155.Edition[] memory editions = currentOrder.mint.getAllEditions(); + uint256[] memory percentChances = currentOrder.mint.getPercentChances(); - uint256[] memory ids = new uint256[](editions.length); - uint256[] memory amounts = new uint256[](editions.length); + uint256[] memory ids = new uint256[](percentChances.length); + uint256[] memory amounts = new uint256[](percentChances.length); - for (uint256 i = 0; i < editions.length; i++) { + for (uint256 i = 0; i < percentChances.length; i++) { ids[i] = i + 1; } @@ -260,8 +260,8 @@ contract NFTMint is Ownable { uint256 roll = uint256(keccak256(abi.encodePacked(nonce++, blockhash(block.number - 1)))) % 100; uint256 cumulativeChance = 0; - for (uint256 j = 0; j < editions.length; j++) { - cumulativeChance += editions[j].percentChance; + for (uint256 j = 0; j < percentChances.length; j++) { + cumulativeChance += percentChances[j]; if (roll < cumulativeChance) { amounts[j]++; break; @@ -272,7 +272,7 @@ contract NFTMint is Ownable { emit OrderFilled(currentOrder.mint, nextOrderIdToFill_, currentOrder.to, currentOrder.amount, amounts); uint256 numNonZero = 0; - for (uint256 i = 0; i < editions.length; i++) { + for (uint256 i = 0; i < percentChances.length; i++) { if (amounts[i] != 0) { if (numNonZero < i) { ids[numNonZero] = ids[i]; From 0fc96461e54e32232ac7fc2d47111c4a1fce779b Mon Sep 17 00:00:00 2001 From: Brian Le Date: Mon, 12 Aug 2024 12:30:15 -0700 Subject: [PATCH 10/15] mitigation: change feeRecipient to immutable and add min fee (#32) --- src/NFTMint.sol | 27 +++++++++++++-------------- test/NFTMint.t.sol | 46 ++++------------------------------------------ 2 files changed, 17 insertions(+), 56 deletions(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 02497c0..fae205e 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -20,7 +20,7 @@ contract NFTMint is Ownable { error NFTMint_InvalidPerWalletLimit(); error NFTMint_InvalidMaxMints(); error NFTMint_InvalidOwner(); - error NFTMint_InvalidFeeRecipient(); + error NFTMint_InsufficientFee(); event MintCreated(MintERC1155 indexed mint, MintArgs args); event OrderPlaced( @@ -38,8 +38,6 @@ contract NFTMint is Ownable { uint96 feePerMint; // Address of the owner of the mint address payable owner; - // Address to receive the fee - address payable feeRecipient; // Timestamp when the mint expires uint40 mintExpiration; // Merkle root for the allowlist @@ -72,8 +70,6 @@ contract NFTMint is Ownable { uint32 perWalletLimit; // Timestamp when the mint expires uint40 mintExpiration; - // Address to receive the fee - address payable feeRecipient; // Merkle root for the allowlist bytes32 allowlistMerkleRoot; // Mapping of addresses to the number of mints they have made @@ -103,8 +99,15 @@ contract NFTMint is Ownable { /// @notice Array of all orders placed. Filled orders are deleted. Order[] public orders; - constructor(address owner_) Ownable(owner_) { + /// @notice Address of the mint fee recipient + address payable public immutable FEE_RECIPIENT; + + /// @notice Minimum fee per mint (approximately $0.05) + uint96 public constant MIN_FEE_PER_MINT = 0.00002 ether; + + constructor(address owner_, address feeRecipient_) Ownable(owner_) { MINT_NFT_LOGIC = address(new MintERC1155(address(this))); + FEE_RECIPIENT = payable(feeRecipient_); } /** @@ -124,8 +127,8 @@ contract NFTMint is Ownable { if (args.owner == address(0)) { revert NFTMint_InvalidOwner(); } - if (args.feeRecipient == address(0) && args.feePerMint != 0) { - revert NFTMint_InvalidFeeRecipient(); + if (args.feePerMint < MIN_FEE_PER_MINT) { + revert NFTMint_InsufficientFee(); } MintERC1155 newMint = MintERC1155( @@ -139,7 +142,6 @@ contract NFTMint is Ownable { mintInfo.remainingMints = args.maxMints; mintInfo.pricePerMint = args.pricePerMint; mintInfo.feePerMint = args.feePerMint; - mintInfo.feeRecipient = args.feeRecipient; mintInfo.perWalletLimit = args.perWalletLimit; mintInfo.allowlistMerkleRoot = args.allowlistMerkleRoot; mintInfo.mintExpiration = args.mintExpiration; @@ -203,11 +205,8 @@ contract NFTMint is Ownable { ); { - bool feeSuccess = true; - if (mintInfo.feePerMint > 0) { - (feeSuccess,) = - mintInfo.feeRecipient.call{ value: mintInfo.feePerMint * modifiedAmount, gas: 100_000 }(""); - } + (bool feeSuccess,) = FEE_RECIPIENT.call{ value: mintInfo.feePerMint * modifiedAmount, gas: 100_000 }(""); + bool mintProceedsSuccess = true; if (mintInfo.pricePerMint > 0) { (mintProceedsSuccess,) = diff --git a/test/NFTMint.t.sol b/test/NFTMint.t.sol index b7f8b81..fb1d30d 100644 --- a/test/NFTMint.t.sol +++ b/test/NFTMint.t.sol @@ -9,9 +9,10 @@ import { MockFailingRecipient } from "./util/MockFailingRecipient.sol"; contract NFTMintTest is TestBase { NFTMint nftMint; + address feeRecipient = vm.createWallet("feeRecipient").addr; function setUp() external { - nftMint = new NFTMint(address(this)); + nftMint = new NFTMint(address(this), feeRecipient); } function test_createMint() public returns (MintERC1155) { @@ -47,7 +48,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 105, feePerMint: 0.001 ether, owner: payable(address(this)), - feeRecipient: payable(address(this)), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -78,7 +78,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 105, feePerMint: 0.001 ether, owner: payable(address(this)), - feeRecipient: payable(address(this)), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -110,7 +109,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 0, feePerMint: 0.001 ether, owner: payable(address(this)), - feeRecipient: payable(address(this)), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -142,7 +140,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 1, feePerMint: 0.001 ether, owner: payable(address(this)), - feeRecipient: payable(address(this)), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -174,7 +171,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 1, feePerMint: 0.001 ether, owner: payable(address(0)), - feeRecipient: payable(address(this)), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -185,38 +181,6 @@ contract NFTMintTest is TestBase { nftMint.createMint(mintArgs); } - function test_createMint_invalidFeeRecipient() external { - MintERC1155.Attribute[] memory attributes = new MintERC1155.Attribute[](1); - attributes[0] = MintERC1155.Attribute({ traitType: "traitType", value: "value" }); - - MintERC1155.Edition[] memory editions = new MintERC1155.Edition[](1); - editions[0] = MintERC1155.Edition({ - name: "Edition 1", - imageURI: "https://example.com/image1.png", - percentChance: 100, - attributes: attributes - }); - - NFTMint.MintArgs memory mintArgs = NFTMint.MintArgs({ - mintExpiration: uint40(block.timestamp + 1 days), - maxMints: 1, - editions: editions, - allowlistMerkleRoot: bytes32(0), - pricePerMint: 0.01 ether, - perWalletLimit: 1, - feePerMint: 0.001 ether, - owner: payable(address(this)), - feeRecipient: payable(address(0)), - name: "My Token Name", - imageURI: "image here", - description: "This is a description", - royaltyAmountBps: 150 - }); - - vm.expectRevert(NFTMint.NFTMint_InvalidFeeRecipient.selector); - nftMint.createMint(mintArgs); - } - event OrderPlaced( MintERC1155 indexed mint, uint256 indexed orderId, address indexed to, uint256 amount, string comment ); @@ -365,7 +329,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 105, feePerMint: 0.001 ether, owner: payable(address(this)), - feeRecipient: payable(address(this)), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -394,7 +357,7 @@ contract NFTMintTest is TestBase { attributes: attributes }); - address minter = _randomAddress(); + address minter = address(new MockFailingRecipient()); vm.deal(minter, 10 ether); vm.prank(minter); @@ -409,7 +372,6 @@ contract NFTMintTest is TestBase { perWalletLimit: 105, feePerMint: 0.001 ether, owner: payable(address(this)), - feeRecipient: payable(address(new MockFailingRecipient())), name: "My Token Name", imageURI: "image here", description: "This is a description", @@ -420,7 +382,7 @@ contract NFTMintTest is TestBase { vm.expectRevert(NFTMint.NFTMint_FailedToTransferFunds.selector); vm.prank(minter); - nftMint.order{ value: 0.011 ether }(mint, 1, "", new bytes32[](0)); + nftMint.order{ value: 0.012 ether }(mint, 1, "", new bytes32[](0)); } event OrderFilled( From 85aa8a93385a481f9499670aae4138fe09d13e40 Mon Sep 17 00:00:00 2001 From: Brian Le Date: Mon, 12 Aug 2024 17:41:38 -0400 Subject: [PATCH 11/15] mitigation: `mintBatch()` gas grief (#27) --- src/NFTMint.sol | 7 +++++-- test/NFTMint.t.sol | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index fae205e..92eb359 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -20,6 +20,7 @@ contract NFTMint is Ownable { error NFTMint_InvalidPerWalletLimit(); error NFTMint_InvalidMaxMints(); error NFTMint_InvalidOwner(); + error NFTMint_InsufficientGas(); error NFTMint_InsufficientFee(); event MintCreated(MintERC1155 indexed mint, MintArgs args); @@ -288,8 +289,10 @@ contract NFTMint is Ownable { delete orders[nextOrderIdToFill_]; nextOrderIdToFill_++; - // If the mint fails with 500_000 gas, the order is still marked as filled. - try currentOrder.mint.mintBatch{ gas: 500_000 }(currentOrder.to, ids, amounts) { } catch { } + + if (gasleft() * 63 / 64 < 1_000_000) revert NFTMint_InsufficientGas(); + // If the mint fails with 1_000_000 gas, the order is still marked as filled. + try currentOrder.mint.mintBatch{ gas: 1_000_000 }(currentOrder.to, ids, amounts) { } catch { } } nextOrderIdToFill = uint96(nextOrderIdToFill_); diff --git a/test/NFTMint.t.sol b/test/NFTMint.t.sol index fb1d30d..d2c8e54 100644 --- a/test/NFTMint.t.sol +++ b/test/NFTMint.t.sol @@ -448,5 +448,21 @@ contract NFTMintTest is TestBase { assertEq(nftMint.nextOrderIdToFill(), 1); } + function test_fillOrders_insufficientGas() external { + MintERC1155 mint = test_createMint(); + + address minter = _randomAddress(); + vm.deal(minter, 10 ether); + vm.prank(minter); + nftMint.order{ value: 1.1 ether }(mint, 100, "Test order", new bytes32[](0)); + + skip(1 days); + // Simulate low gas scenario + vm.startPrank(address(this)); + vm.expectRevert(NFTMint.NFTMint_InsufficientGas.selector); + nftMint.fillOrders{ gas: 500_000 }(0); + vm.stopPrank(); + } + receive() external payable { } } From bdaec4d4df15478f2a3fc70dd01471f72eb264cb Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 17:56:49 -0400 Subject: [PATCH 12/15] bump version --- src/MintERC1155.sol | 2 +- src/NFTMint.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/MintERC1155.sol b/src/MintERC1155.sol index 28b63a1..9809918 100644 --- a/src/MintERC1155.sol +++ b/src/MintERC1155.sol @@ -250,6 +250,6 @@ contract MintERC1155 is ERC1155Upgradeable, OwnableUpgradeable, ERC2981Upgradeab } function VERSION() external pure returns (string memory) { - return "0.1.5"; + return "0.1.6"; } } diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 92eb359..60a6ee9 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -300,6 +300,6 @@ contract NFTMint is Ownable { } function VERSION() external pure returns (string memory) { - return "0.1.5"; + return "0.1.6"; } } From e8204388868c1aa9a4a56b8f10856c427f5eaa3e Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 18:14:03 -0400 Subject: [PATCH 13/15] update deploy config --- deployments/84532.json | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/deployments/84532.json b/deployments/84532.json index 9bb3e0c..a7a5016 100644 --- a/deployments/84532.json +++ b/deployments/84532.json @@ -33,8 +33,11 @@ "address": "0xCDD6Bd714f3B1a98aa5891965D8eF39c32BFCAf0" } ], - "constructorArgs": ["PartyDAO"] + "constructorArgs": ["PartyDAO", "partydaoMultisig"] } }, - "constants": { "PartyDAO": "0x85065829b9914ea8f456911e0f6d2f9d9a1705ed" } + "constants": { + "PartyDAO": "0x85065829b9914ea8f456911e0f6d2f9d9a1705ed", + "partydaoMultisig": "0x85065829b9914ea8f456911e0f6d2f9d9a1705ed" + } } From 10d89174db9afa1975aa102acbe40112090f8b4b Mon Sep 17 00:00:00 2001 From: GitHub Actions Bot <> Date: Mon, 12 Aug 2024 22:15:02 +0000 Subject: [PATCH 14/15] CI deployment of NFTMint --- deployments/84532.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/deployments/84532.json b/deployments/84532.json index a7a5016..262cd17 100644 --- a/deployments/84532.json +++ b/deployments/84532.json @@ -31,6 +31,11 @@ "deployedArgs": "0x00000000000000000000000085065829b9914ea8f456911e0f6d2f9d9a1705ed", "version": "0.1.5", "address": "0xCDD6Bd714f3B1a98aa5891965D8eF39c32BFCAf0" + }, + { + "deployedArgs": "0x00000000000000000000000085065829b9914ea8f456911e0f6d2f9d9a1705ed00000000000000000000000085065829b9914ea8f456911e0f6d2f9d9a1705ed", + "version": "0.1.6", + "address": "0x80045B318a27CE477E9faa7f34Efcf853dbCdFFC" } ], "constructorArgs": ["PartyDAO", "partydaoMultisig"] From d2cda20bdc3622b01e5b20c4ff3fd4014afb7593 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 13 Aug 2024 01:30:56 -0400 Subject: [PATCH 15/15] chore: remove unnecessary postfix (#37) --- src/NFTMint.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NFTMint.sol b/src/NFTMint.sol index 60a6ee9..e0c8b9d 100644 --- a/src/NFTMint.sol +++ b/src/NFTMint.sol @@ -296,7 +296,7 @@ contract NFTMint is Ownable { } nextOrderIdToFill = uint96(nextOrderIdToFill_); - _nextNonce = uint96(nonce++); + _nextNonce = uint96(nonce); } function VERSION() external pure returns (string memory) {