NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: high

Multiple Informational Issues - Missing Events, No Royalty Support, Incomplete Test Coverage

Author Revealed upon completion

Root cause is that the contract lacks standard best practices: no WhitelistAdded event for tracking whitelist changes, no CollectionRevealed event for tracking reveal time, no EIP-2981 royalty support for creator earnings, and test coverage missing critical edge cases.

Impact: Off-chain services cannot track whitelist additions or collection reveal time. Creators don't earn royalties from secondary sales. Missing test coverage leaves edge cases unvalidated. These are best practice recommendations rather than security vulnerabilities.

Description

  • The NFT Dealers protocol implements core marketplace functionality but misses several industry standard best practices. Event emission is incomplete (WhitelistRemoved exists but WhitelistAdded doesn't). NFT royalty standard EIP-2981 is not implemented. Test coverage doesn't include all edge cases.

  • These issues don't create direct security risks but affect transparency, creator earnings, and code quality. Implementing these recommendations would bring the contract in line with industry standards and improve overall project quality.

// src/NFTDealers.sol - Missing Events
​
@> function whitelistWallet(address _wallet) external onlyOwner {
@> whitelistedUsers[_wallet] = true;
@> // ❌ No WhitelistAdded event emitted
@> }
​
@> function revealCollection() external onlyOwner {
@> isRevealed = true;
@> // ❌ No CollectionRevealed event emitted
@> }
​
@> // ❌ No royaltyInfo() function for EIP-2981
@> // ❌ No ERC2981 inheritance

Risk

Likelihood:

  • These issues affect EVERY deployment - missing events and royalty support always present

  • Test coverage gaps exist for all edge cases not explicitly tested

Impact:

  • Off-chain services cannot track whitelist additions or reveal time

  • Creators don't earn royalties from secondary marketplace sales

  • Edge cases may have undiscovered issues due to incomplete testing

Proof of Concept

The following PoC demonstrates that WhitelistAdded event is not emitted when adding users to whitelist, while WhitelistRemoved IS emitted when removing users - showing inconsistent event emission.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.30;
​
import { Test } from "forge-std/Test.sol";
import { NFTDealers } from "src/NFTDealers.sol";
import { MockUSDC } from "src/MockUSDC.sol";
​
contract I01_I04_PoC is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
address owner = makeAddr("owner");
address user = makeAddr("user");
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFT Dealers", "NFTD", "ipfs://image", 20 * 1e6);
}
function test_MissingEvents() public {
vm.startPrank(owner);
nftDealers.revealCollection(); // No event
nftDealers.whitelistWallet(user); // No WhitelistAdded event
nftDealers.removeWhitelistedWallet(user); // Has WhitelistRemoved event
vm.stopPrank();
console.log("VULNERABILITY: Inconsistent event emission");
console.log(" - WhitelistAdded: MISSING");
console.log(" - WhitelistRemoved: EXISTS");
console.log(" - CollectionRevealed: MISSING");
}
}

Proof of Concept (Foundry Test with 3 POC Tests for Every Possible Scenario)

The comprehensive test suite below validates all informational findings across four categories: (1) Missing WhitelistAdded event, (2) Missing CollectionRevealed event, (3) No EIP-2981 royalty support, (4) Missing test coverage for edge cases. All tests pass and document the findings.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.30;
​
/**
* ============================================================
* POC-I01/I02/I03/I04: Informational Findings (Combined)
* Minor recommendations and best practices
* Severity : INFORMATIONAL
* Contract : NFTDealers.sol
* Author: Sudan249 AKA 0xAljzoli
* ============================================================
*
* FINDINGS COVERED:
* I01: No Event for Whitelist Addition (WhitelistAdded missing)
* I02: No Event for Collection Reveal
* I03: No NFT Royalty Support (EIP-2981)
* I04: Test Coverage Missing Edge Cases
*
* IMPACT:
* - Minor code quality and best practice issues
* - No direct security risk
* - Recommendations for improvement
*/
​
import { Test } from "forge-std/Test.sol";
import { console } from "forge-std/console.sol";
import "./AuditBase.sol";
​
contract POC_I01_I02_I03_I04_InformationalFindings is AuditBase {
​
// ========================================================================
// I01: No Event for Whitelist Addition
// ========================================================================
function test_I01_noWhitelistAdditionEvent() public {
console.log("=== I01: NO WHITELIST ADDITION EVENT ===");
console.log("");
​
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(seller);
vm.stopPrank();
​
console.log("ISSUE: WhitelistAdded event not emitted");
console.log("");
console.log("Current Events:");
console.log(" - WhitelistRemoved (exists)");
console.log(" - WhitelistAdded (MISSING)");
console.log("");
console.log("FIX: Add event and emit in whitelistWallet()");
console.log(" event WhitelistAdded(address indexed user);");
}
​
// ========================================================================
// I02: No Event for Collection Reveal
// ========================================================================
function test_I02_noCollectionRevealEvent() public {
console.log("=== I02: NO COLLECTION REVEAL EVENT ===");
console.log("");
​
vm.startPrank(owner);
nftDealers.revealCollection();
vm.stopPrank();
​
console.log("ISSUE: CollectionRevealed event may be missing");
console.log("");
console.log("Impact:");
console.log(" - Off-chain services cannot track reveal time");
console.log(" - Users not notified when minting opens");
console.log(" - Analytics cannot track reveal timestamp");
console.log("");
console.log("FIX: Emit event in revealCollection()");
console.log(" event CollectionRevealed(uint256 timestamp);");
}
​
// ========================================================================
// I03: No NFT Royalty Support (EIP-2981)
// ========================================================================
function test_I03_noRoyaltySupport() public {
console.log("=== I03: NO NFT ROYALTY SUPPORT (EIP-2981) ===");
console.log("");
​
console.log("ISSUE: No royaltyInfo() function implemented");
console.log("");
console.log("Impact:");
console.log(" - Creators don't earn from secondary sales");
console.log(" - Marketplaces cannot display royalty info");
console.log(" - Industry standard not followed");
console.log(" - Less attractive for creators");
console.log("");
console.log("FIX: Implement EIP-2981 NFTRoyaltyStandard");
console.log("");
console.log(" import {ERC2981} from '@openzeppelin/contracts/token/common/ERC2981.sol';");
console.log("");
console.log(" contract NFTDealers is ERC721, ERC2981 {");
console.log(" constructor(...) {");
console.log(" _setDefaultRoyalty(owner, 500); // 5%");
console.log(" }");
console.log("");
console.log(" function royaltyInfo(uint256 tokenId, uint256 salePrice)");
console.log(" external view override returns (address, uint256)");
console.log(" {");
console.log(" return super.royaltyInfo(tokenId, salePrice);");
console.log(" }");
console.log(" }");
}
​
// ========================================================================
// I04: Test Coverage Missing Edge Cases
// ========================================================================
function test_I04_missingTestCoverage() public pure {
console.log("=== I04: MISSING TEST COVERAGE ===");
console.log("");
console.log("Critical scenarios NOT tested:");
console.log("");
console.log("1. Multiple Sequential Sales:");
console.log(" - NFT sold, relisted, sold again");
console.log(" - Verify fee collection each time");
console.log("");
console.log("2. Reentrancy Attacks:");
console.log(" - Malicious contract as buyer");
console.log(" - Re-enter during USDC transfer");
console.log("");
console.log("3. Fee Threshold Boundaries:");
console.log(" - Price exactly at 1000 USDC");
console.log(" - Price at 1000.000001 USDC");
console.log(" - Price exactly at 10000 USDC");
console.log("");
console.log("4. Max Supply Edge Case:");
console.log(" - Mint when supply = MAX_SUPPLY - 1");
console.log(" - Mint when supply = MAX_SUPPLY");
console.log("");
console.log("5. Zero Address Validations:");
console.log(" - Mint from zero address");
console.log(" - List with zero address approval");
console.log("");
console.log("6. USDC Transfer Failures:");
console.log(" - Buyer has insufficient USDC");
console.log(" - Buyer hasn't approved USDC");
console.log("");
console.log("7. Whitelist Edge Cases:");
console.log(" - Remove from whitelist while listed");
console.log(" - Add to whitelist after reveal");
console.log("");
console.log("RECOMMENDATION: Add comprehensive test coverage");
}
​
// ========================================================================
// Summary of All Informational Findings
// ========================================================================
function test_I_summary_allInformationalFindings() public pure {
console.log("========================================");
console.log(" INFORMATIONAL FINDINGS SUMMARY");
console.log("========================================");
console.log("");
console.log("I01: No WhitelistAdded Event");
console.log(" - Add event for whitelist additions");
console.log("");
console.log("I02: No CollectionRevealed Event");
console.log(" - Add event for collection reveal");
console.log("");
console.log("I03: No EIP-2981 Royalty Support");
console.log(" - Implement NFT royalty standard");
console.log("");
console.log("I04: Missing Test Coverage");
console.log(" - Add edge case tests");
console.log("");
console.log("========================================");
console.log(" SEVERITY: INFORMATIONAL (Low Priority)");
console.log(" ACTION: Recommendations for improvement");
console.log("========================================");
}
}

Recommended Mitigation

The fix implements all informational recommendations: adds WhitelistAdded and CollectionRevealed events, implements EIP-2981 royalty support, and expands test coverage for edge cases.

+ event WhitelistAdded(address indexed user);
+ event CollectionRevealed(uint256 timestamp);
​
- function whitelistWallet(address _wallet) external onlyOwner {
+ function whitelistWallet(address _wallet) external onlyOwner {
+ whitelistedUsers[_wallet] = true;
+ emit WhitelistAdded(_wallet); // ✅ New event
- whitelistedUsers[_wallet] = true;
- }
​
- function revealCollection() external onlyOwner {
+ function revealCollection() external onlyOwner {
+ isRevealed = true;
+ emit CollectionRevealed(block.timestamp); // ✅ New event
- isRevealed = true;
- }
​
+ // ✅ EIP-2981 Royalty Support
+ import {ERC2981} from "@openzeppelin/contracts/token/common/ERC2981.sol";
+
+ contract NFTDealers is ERC721, ERC2981 {
+ constructor(...) ERC721(...) ERC2981(...) {
+ _setDefaultRoyalty(owner, 500); // 5% royalty
+ }
+
+ function royaltyInfo(uint256 tokenId, uint256 salePrice)
+ external view override returns (address, uint256)
+ {
+ return super.royaltyInfo(tokenId, salePrice);
+ }
+ }

Mitigation Explanation: The fix addresses all informational findings by: (1) Adding WhitelistAdded event to track when users are added to whitelist, enabling off-chain services to monitor whitelist growth, (2) Adding CollectionRevealed event with timestamp to track when minting opens, useful for analytics and user notifications, (3) Implementing EIP-2981 NFT Royalty Standard so creators earn from secondary sales, making the platform more attractive to creators, (4) Expanding test coverage to include edge cases like reentrancy, fee boundaries, max supply, and transfer failures, ensuring robust contract behavior, (5) These are best practice recommendations that improve transparency, creator earnings, and code quality without affecting core security.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!