DropERC721-C #690
DropERC721-C #690
Conversation
WalkthroughA new ERC721 drop contract ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DropERC721C
participant Platform
participant SaleRecipient
participant RoyaltyRecipient
User->>DropERC721C: lazyMint(amount, baseURI, data)
DropERC721C-->>User: batchId
User->>DropERC721C: claim(tokenId)
DropERC721C->>Platform: distribute platform fee
DropERC721C->>SaleRecipient: distribute sale proceeds
DropERC721C->>RoyaltyRecipient: distribute royalty (if applicable)
DropERC721C-->>User: transfer token
User->>DropERC721C: reveal(batchIndex, key)
DropERC721C-->>User: revealedURI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.gitmodules(1 hunks)contracts/prebuilts/drop/DropERC721C.sol(1 hunks)foundry.toml(1 hunks)lib/creator-token-standards(1 hunks)
🔇 Additional comments (5)
foundry.toml (1)
43-43: Remapping is correct; verify submodule inclusion in CI/CD
The path prefix matches the library’ssrclayout and follows the existing trailing-slash convention, soforgeshould resolve imports properly.
Please confirm that
.gitmodulestrackslib/creator-token-standardsat the intended commit, and- your build pipeline runs
git submodule update --init --recursivebeforeforge build,otherwise compilation will fail on clean clones.
lib/creator-token-standards (1)
1-1: Ensure submodule is fully declared and policy-compliantOnly the commit pointer is present. Please verify that:
.gitmodules(path+URL) andfoundry.tomlremap entries are committed in the same PR so CI/clones work out-of-the-box.- The pinned SHA corresponds to a vetted, tagged release that satisfies our license & security policies.
If either item is missing or non-compliant, update before merge.
.gitmodules (1)
58-60: LGTM! Submodule configuration is correct.The new
creator-token-standardssubmodule entry is properly configured and follows the standard format consistent with other submodules in the project.contracts/prebuilts/drop/DropERC721C.sol (2)
75-76: Hardcoded fee recipient address requires documentation.The
DEFAULT_FEE_RECIPIENTis permanently set to0x1Af20C6B23373350aD464700B5965CE4B0D2aD94and will receive 1% of all primary sales. This creates an immutable fee collection point that should be clearly documented for transparency. Consider:
- Adding comprehensive documentation explaining this fee structure
- Making this configurable through initialization if flexibility is needed
122-122: Verify the intent of granting TRANSFER_ROLE to address(0).Granting
TRANSFER_ROLEtoaddress(0)is unusual and could have unintended security implications. This might bypass transfer restrictions in the_beforeTokenTransfershook. Please confirm this is intentional and document the reasoning.
| _setupRole(_transferRole, _defaultAdmin); | ||
| _setupRole(_transferRole, address(0)); | ||
| _setupRole(_metadataRole, _defaultAdmin); | ||
| _setRoleAdmin(_metadataRole, _metadataRole); |
There was a problem hiding this comment.
Metadata role self-administration creates privilege escalation risk.
Setting _metadataRole as its own admin means any account with METADATA_ROLE can grant this role to other accounts. This could lead to unintended privilege escalation where a metadata manager can create additional metadata managers without admin approval.
Consider setting DEFAULT_ADMIN_ROLE as the admin for metadata role:
-_setRoleAdmin(_metadataRole, _metadataRole);
+_setRoleAdmin(_metadataRole, DEFAULT_ADMIN_ROLE);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _setRoleAdmin(_metadataRole, _metadataRole); | |
| _setRoleAdmin(_metadataRole, DEFAULT_ADMIN_ROLE); |
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol at line 124, the code sets
_metadataRole as its own admin, allowing any holder of METADATA_ROLE to grant it
to others, causing a privilege escalation risk. Change the admin role of
_metadataRole to DEFAULT_ADMIN_ROLE instead by calling
_setRoleAdmin(_metadataRole, DEFAULT_ADMIN_ROLE) to ensure only admins can grant
metadata roles.
| if (encryptedURI.length != 0 && provenanceHash != "") { | ||
| _setEncryptedData(nextTokenIdToLazyMint + _amount, _data); |
There was a problem hiding this comment.
Incorrect bytes32 comparison with empty string.
The condition provenanceHash != "" is comparing a bytes32 with an empty string literal, which is not the correct way to check for an empty bytes32 value.
Apply this fix:
-if (encryptedURI.length != 0 && provenanceHash != "") {
+if (encryptedURI.length != 0 && provenanceHash != bytes32(0)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (encryptedURI.length != 0 && provenanceHash != "") { | |
| _setEncryptedData(nextTokenIdToLazyMint + _amount, _data); | |
| if (encryptedURI.length != 0 && provenanceHash != bytes32(0)) { | |
| _setEncryptedData(nextTokenIdToLazyMint + _amount, _data); |
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol around lines 216 to 217, the code
incorrectly compares a bytes32 variable provenanceHash to an empty string
literal. To fix this, replace the comparison provenanceHash != "" with
provenanceHash != bytes32(0) to correctly check if provenanceHash is not empty.
| uint256 platformFeesTw = (totalPrice * DEFAULT_FEE_BPS) / MAX_BPS; | ||
| uint256 platformFees = (totalPrice * platformFeeBps) / MAX_BPS; | ||
|
|
||
| bool validMsgValue; | ||
| if (_currency == CurrencyTransferLib.NATIVE_TOKEN) { | ||
| validMsgValue = msg.value == totalPrice; | ||
| } else { | ||
| validMsgValue = msg.value == 0; | ||
| } | ||
| require(validMsgValue, "!V"); | ||
|
|
||
| CurrencyTransferLib.transferCurrency(_currency, _msgSender(), DEFAULT_FEE_RECIPIENT, platformFeesTw); | ||
| CurrencyTransferLib.transferCurrency(_currency, _msgSender(), platformFeeRecipient, platformFees); | ||
| CurrencyTransferLib.transferCurrency( | ||
| _currency, | ||
| _msgSender(), | ||
| saleRecipient, | ||
| totalPrice - platformFees - platformFeesTw | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Double fee structure should be clearly documented.
The contract implements a dual fee system:
- A hardcoded 1% fee to
DEFAULT_FEE_RECIPIENT(line 307) - A configurable platform fee to
platformFeeRecipient(line 308)
Both fees are deducted from the buyer's payment. This means the total fees could be higher than expected (e.g., if platform fee is 5%, total fees would be 6%). This dual fee structure should be prominently documented to ensure transparency for users and integrators.
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol around lines 307 to 325, add clear
comments explaining the dual fee structure: a fixed 1% fee sent to
DEFAULT_FEE_RECIPIENT and a separate configurable platform fee sent to
platformFeeRecipient, both deducted from the buyer's payment. This documentation
should clarify that total fees are the sum of these two amounts to ensure
transparency for users and integrators.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
contracts/prebuilts/drop/DropERC721C.sol (3)
124-124: Metadata role self-administration creates privilege escalation risk.Setting
_metadataRoleas its own admin means any account withMETADATA_ROLEcan grant this role to other accounts. This could lead to unintended privilege escalation where a metadata manager can create additional metadata managers without admin approval.Consider setting
DEFAULT_ADMIN_ROLEas the admin for metadata role:-_setRoleAdmin(_metadataRole, _metadataRole); +_setRoleAdmin(_metadataRole, DEFAULT_ADMIN_ROLE);
216-217: Incorrect bytes32 comparison with empty string.The condition
provenanceHash != ""is comparing abytes32with an empty string literal, which is not the correct way to check for an empty bytes32 value.Apply this fix:
-if (encryptedURI.length != 0 && provenanceHash != "") { +if (encryptedURI.length != 0 && provenanceHash != bytes32(0)) {
307-325: Double fee structure should be clearly documented.The contract implements a dual fee system:
- A hardcoded 1% fee to
DEFAULT_FEE_RECIPIENT(line 307)- A configurable platform fee to
platformFeeRecipient(line 308)Both fees are deducted from the buyer's payment. This means the total fees could be higher than expected (e.g., if platform fee is 5%, total fees would be 6%). This dual fee structure should be prominently documented to ensure transparency for users and integrators.
Add clear comments explaining the dual fee structure and consider documenting this in the contract's NatSpec documentation.
🧹 Nitpick comments (1)
contracts/prebuilts/drop/DropERC721C.sol (1)
372-376: Inconsistent error handling pattern.The
_requireCallerIsContractOwnerfunction uses a customNotAuthorized()error, which is good practice. However, other access control checks in the contract userequire()statements with string messages, creating inconsistency.Consider standardizing error handling across the contract by either:
- Using custom errors consistently throughout, or
- Using require statements with descriptive messages consistently
For better gas efficiency, custom errors are preferred:
function _canSetPlatformFeeInfo() internal view override returns (bool) { - return hasRole(DEFAULT_ADMIN_ROLE, _msgSender()); + if (!hasRole(DEFAULT_ADMIN_ROLE, _msgSender())) { + revert NotAuthorized(); + } + return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/prebuilts/drop/DropERC721C.sol(1 hunks)
🔇 Additional comments (4)
contracts/prebuilts/drop/DropERC721C.sol (4)
286-288: Supply validation logic is correct and comprehensive.The
_beforeClaimfunction properly validates both lazy mint bounds and global max supply constraints. The logic correctly handles the case wheremaxTotalSupplyis 0 (unlimited) and ensures tokens cannot be claimed beyond what's been lazy minted.
422-426: Transfer role restriction logic is well-implemented.The transfer restriction logic correctly allows minting and burning (transfers to/from address(0)) while enforcing role-based restrictions for regular transfers. The logic properly checks both sender and recipient roles, ensuring either party having the transfer role allows the transfer.
448-466: Message sender resolution correctly prioritizes ERC2771.The
_msgSender()and_msgData()overrides properly resolve the diamond problem in multiple inheritance by explicitly usingERC2771ContextUpgradeableimplementation. This ensures meta-transaction support works correctly.
20-21: Creator-token-standards submodule integration verifiedThe
@limitbreak/creator-token-standardssubmodule is correctly initialized and mapped:
- Submodule present at lib/creator-token-standards (commit 980a63b33591d568b6e04b45f37deba05a55f787)
- Remapped in foundry.toml under
@limitbreak/creator-token-standards/=lib/creator-token-standards/src/- Imports in contracts/prebuilts/drop/DropERC721C.sol (
CreatorTokenBase.sol,AutomaticValidatorTransferApproval.sol) resolve to the expected pathsPlease ensure the referenced commit has been audited and approved for security.
| address public constant DEFAULT_FEE_RECIPIENT = 0x1Af20C6B23373350aD464700B5965CE4B0D2aD94; | ||
| uint16 private constant DEFAULT_FEE_BPS = 100; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded fee recipient and rate create centralization risk.
The contract hardcodes a specific fee recipient address and 1% fee rate. This creates a permanent dependency on this address and prevents fee adjustment, which could be problematic for long-term governance or if the address becomes compromised.
Consider making the default fee recipient and rate configurable through admin functions:
-address public constant DEFAULT_FEE_RECIPIENT = 0x1Af20C6B23373350aD464700B5965CE4B0D2aD94;
-uint16 private constant DEFAULT_FEE_BPS = 100;
+address public defaultFeeRecipient;
+uint16 public defaultFeeBps;Add setter functions with appropriate access controls to update these values.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol around lines 75 to 77, the fee
recipient address and fee rate are hardcoded, causing centralization risk and
lack of flexibility. To fix this, remove the constant declarations and instead
declare these as private or internal variables. Then, add setter functions with
proper access control (e.g., onlyOwner or admin modifier) to allow updating the
fee recipient address and fee rate securely. This will enable governance or
admin to adjust fees if needed.
| function isApprovedForAll(address owner, address operator) public view virtual override returns (bool isApproved) { | ||
| isApproved = super.isApprovedForAll(owner, operator); | ||
|
|
||
| if (!isApproved) { | ||
| if (autoApproveTransfersFromValidator) { | ||
| isApproved = operator == address(getTransferValidator()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Auto-approval logic introduces potential security bypass.
The isApprovedForAll override automatically approves the transfer validator when autoApproveTransfersFromValidator is true. This could bypass intended approval checks if the transfer validator is compromised or if the auto-approval setting is unintentionally enabled.
Add explicit documentation and consider adding an event when auto-approval is used:
function isApprovedForAll(address owner, address operator) public view virtual override returns (bool isApproved) {
isApproved = super.isApprovedForAll(owner, operator);
if (!isApproved) {
if (autoApproveTransfersFromValidator) {
+ // Auto-approving transfer validator - ensure this is intended behavior
isApproved = operator == address(getTransferValidator());
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isApprovedForAll(address owner, address operator) public view virtual override returns (bool isApproved) { | |
| isApproved = super.isApprovedForAll(owner, operator); | |
| if (!isApproved) { | |
| if (autoApproveTransfersFromValidator) { | |
| isApproved = operator == address(getTransferValidator()); | |
| } | |
| } | |
| } | |
| function isApprovedForAll(address owner, address operator) public view virtual override returns (bool isApproved) { | |
| isApproved = super.isApprovedForAll(owner, operator); | |
| if (!isApproved) { | |
| if (autoApproveTransfersFromValidator) { | |
| // Auto-approving transfer validator - ensure this is intended behavior | |
| isApproved = operator == address(getTransferValidator()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol around lines 139 to 147, the
isApprovedForAll function auto-approves the transfer validator which may cause
security risks. Add explicit NatSpec comments above the function explaining the
auto-approval behavior and its risks. Additionally, emit a custom event inside
the conditional block when auto-approval is granted to provide on-chain
visibility of such approvals.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
contracts/prebuilts/drop/DropERC721C.sol (5)
73-75: Hard-coded fee recipient and rate remain non-configurable
This still embeds an immutable address and 1 % fee, posing governance and upgradeability risks. Make them configurable under admin control instead ofconstant.
121-121: Metadata role is self-admin → privilege escalation
Using_metadataRoleas its own admin lets any metadata manager grant that role to others. Switch admin toDEFAULT_ADMIN_ROLE.
136-144: Auto-approval path needs explicit documentation / event
The silent auto-approval of the transfer validator can mask unintended approvals. Add NatSpec explaining the risk and emit an event when triggered.
213-214: Incorrect empty-check on bytes32 provenance hash
provenanceHash != ""compares abytes32to a string literal. Usebytes32(0)instead.
299-306: Dual-fee calculation needs explicit documentation
Two separate fees (fixed 1 % + configurable platform fee) are deducted from buyers. Document prominently to avoid integrator confusion.
🧹 Nitpick comments (1)
contracts/prebuilts/drop/DropERC721C.sol (1)
243-258: Missing events for metadata base-URI changes
updateBatchBaseURIandfreezeBatchBaseURImodify token metadata but emit no events. Emit dedicated events so indexers and clients can track these critical state changes.
| /// @dev Lets a contract admin set the global maximum supply for collection's NFTs. | ||
| function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| maxTotalSupply = _maxTotalSupply; | ||
| emit MaxTotalSupplyUpdated(_maxTotalSupply); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against lowering maxTotalSupply below minted count
setMaxTotalSupply allows any admin to set a value smaller than _totalMinted(), permanently blocking further claims. Add a check:
require(_maxTotalSupply >= _totalMinted(), "max supply < minted");🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol around lines 264 to 268, the
setMaxTotalSupply function allows setting maxTotalSupply to a value less than
the number of already minted tokens, which can block further claims. Add a
require statement at the start of the function to ensure the new maxTotalSupply
is not less than _totalMinted(), using require(_maxTotalSupply >=
_totalMinted(), "max supply < minted"); to prevent this issue.
| (address platformFeeRecipient, uint16 platformFeeBps) = getPlatformFeeInfo(); | ||
|
|
||
| address saleRecipient = _primarySaleRecipient == address(0) ? primarySaleRecipient() : _primarySaleRecipient; | ||
|
|
||
| uint256 totalPrice = _quantityToClaim * _pricePerToken; | ||
| uint256 platformFeesTw = (totalPrice * DEFAULT_FEE_BPS) / MAX_BPS; | ||
| uint256 platformFees = (totalPrice * platformFeeBps) / MAX_BPS; | ||
|
|
||
| bool validMsgValue; | ||
| if (_currency == CurrencyTransferLib.NATIVE_TOKEN) { | ||
| validMsgValue = msg.value == totalPrice; | ||
| } else { | ||
| validMsgValue = msg.value == 0; | ||
| } | ||
| require(validMsgValue, "!V"); | ||
|
|
||
| CurrencyTransferLib.transferCurrency(_currency, _msgSender(), DEFAULT_FEE_RECIPIENT, platformFeesTw); | ||
| CurrencyTransferLib.transferCurrency(_currency, _msgSender(), platformFeeRecipient, platformFees); | ||
| CurrencyTransferLib.transferCurrency( | ||
| _currency, | ||
| _msgSender(), | ||
| saleRecipient, | ||
| totalPrice - platformFees - platformFeesTw | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-entrancy exposure in fee collection
_collectPriceOnClaim forwards Ether/ERC20 to external recipients before the mint occurs and without nonReentrant. A malicious recipient could re-enter claim (directly or via fallback) and drain funds / exceed supply.
Add OpenZeppelin ReentrancyGuard to the contract or restructure so state changes happen before external calls.
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol around lines 299 to 323, the
function _collectPriceOnClaim sends Ether/ERC20 payments to external recipients
before completing minting and lacks reentrancy protection. To fix this, add
OpenZeppelin's ReentrancyGuard to the contract and apply the nonReentrant
modifier to _collectPriceOnClaim, or alternatively, restructure the function so
that all state changes, including minting, occur before any external calls to
transfer funds.
Summary by CodeRabbit
New Features
Chores