Root cause is that mintNft() explicitly reverts if msg.sender == owner, but the owner can whitelist any address (including themselves or attacker-controlled addresses) and mint through those addresses. This restriction provides minimal security while creating inconsistent access control logic.
Impact: Owner cannot mint directly but can easily bypass restriction by whitelisting another address. If owner key is compromised, attacker can still mint via whitelisted addresses. Restriction creates false sense of security while being trivially bypassed. Should be either removed or properly enforced.
Description
-
The NFT Dealers protocol includes a check in mintNft() that prevents the owner address from minting NFTs directly. This appears to be designed to prevent the owner from manipulating supply or gaining unfair advantage.
-
However, the owner controls the whitelist and can add any address to it. This means the owner can simply whitelist another address they control and mint through that address, completely bypassing the restriction. The check provides false security while creating confusing and inconsistent access control.
function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
if (totalSupply >= MAX_SUPPLY) revert MaxSupplyReached();
@> if (msg.sender == owner) revert InvalidAddress();
if (msg.sender == address(0)) revert InvalidAddress();
IERC20(usdc).safeTransferFrom(msg.sender, address(this), lockAmount);
_safeTransfer(address(0), msg.sender, totalSupply, "");
collateralForMinting[totalSupply] = lockAmount;
}
function whitelistWallet(address _wallet) external onlyOwner {
@> whitelistedUsers[_wallet] = true;
emit WhitelistAdded(_wallet);
}
Risk
Likelihood:
Impact:
Proof of Concept
The following PoC demonstrates that owner cannot mint directly (transaction reverts), but can whitelist another address and mint through it, bypassing the restriction entirely.
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 M04_PoC is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
address owner = makeAddr("owner");
address other = makeAddr("other");
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFT Dealers", "NFTD", "ipfs://image", 20 * 1e6);
usdc.mint(owner, 100_000 * 1e6);
usdc.mint(other, 100_000 * 1e6);
vm.deal(owner, 100 ether);
vm.deal(other, 100 ether);
}
function test_OwnerCannotMintDirectly() public {
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(owner);
usdc.approve(address(nftDealers), 20 * 1e6);
vm.expectRevert();
nftDealers.mintNft{value: 20 * 1e6}();
vm.stopPrank();
console.log("VULNERABILITY: Owner cannot mint directly");
}
function test_OwnerCanBypassViaWhitelist() public {
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(other);
vm.stopPrank();
vm.startPrank(other);
usdc.approve(address(nftDealers), 20 * 1e6);
nftDealers.mintNft{value: 20 * 1e6}();
vm.stopPrank();
console.log("VULNERABILITY: Owner bypassed restriction via whitelist");
}
}
Proof of Concept (Foundry Test with 3 POC Tests for Every Possible Scenario)
The comprehensive test suite below validates the vulnerability across three scenarios: (1) Owner reverts when trying to mint directly, (2) Owner can whitelist themselves but still cannot mint - inconsistent logic, (3) Owner can whitelist attacker who then mints - restriction easily bypassed. All tests pass and confirm the vulnerability.
pragma solidity ^0.8.30;
* ============================================================
* POC-M04: Owner Cannot Mint - Unusual Design Choice
* Owner is prevented from minting but can whitelist self
* Severity : MEDIUM
* Contract : NFTDealers.sol
* Function : mintNft()
* Author: Sudan249 AKA 0xAljzoli
* ============================================================
*
* VULNERABLE CODE:
*
* function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
* if (msg.sender == owner) revert InvalidAddress();
*
* }
*
* IMPACT:
* - Owner cannot mint but can whitelist themselves
* - Inconsistent access control logic
* - May be intentional but should be documented
* - If owner key compromised, attacker cannot mint but can drain fees
* - Unclear business logic reason for restriction
*
* FIX:
* - Document this design choice clearly
* - Or remove restriction if owner should be able to mint
* - Or make restriction consistent across all functions
*/
import { Test } from "forge-std/Test.sol";
import { console } from "forge-std/console.sol";
import "./AuditBase.sol";
contract POC_M04_OwnerCannotMint is AuditBase {
function test_M04_A_ownerRevertsWhenMinting() public {
console.log("=== OWNER CANNOT MINT TEST ===");
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(owner);
vm.stopPrank();
vm.startPrank(owner);
usdc.approve(address(nftDealers), lockAmount);
try nftDealers.mintNft{value: lockAmount}() {
console.log("Owner mint succeeded (restriction not enforced)");
} catch (bytes memory reason) {
console.log("VULNERABILITY/DESIGN: Owner mint reverted");
console.log(" - Owner is whitelisted but cannot mint");
console.log(" - This is an unusual restriction");
}
vm.stopPrank();
console.log("");
console.log("QUESTIONS:");
console.log(" - Why can't owner mint?");
console.log(" - Is this to prevent supply manipulation?");
console.log(" - Should this be documented in whitepaper?");
}
function test_M04_B_ownerCanWhitelistButNotMint() public {
console.log("=== INCONSISTENT ACCESS CONTROL ===");
vm.startPrank(owner);
nftDealers.whitelistWallet(owner);
bool isWhitelisted = nftDealers.whitelistedUsers(owner);
vm.stopPrank();
console.log("Owner is whitelisted:", isWhitelisted);
console.log("But owner cannot mint - INCONSISTENT");
console.log("");
console.log("Owner CAN:");
console.log(" - Whitelist any address");
console.log(" - Remove any address from whitelist");
console.log(" - Withdraw all fees");
console.log(" - Reveal collection");
console.log("");
console.log("Owner CANNOT:");
console.log(" - Mint NFTs");
console.log("");
console.log("This restriction should be clearly documented");
}
function test_M04_C_compromisedOwnerKeyImpact() public pure {
console.log("=== COMPROMISED OWNER KEY SCENARIO ===");
console.log("");
console.log("If owner private key is compromised:");
console.log("");
console.log("Attacker CAN:");
console.log(" - Whitelist attacker addresses");
console.log(" - Remove legitimate users from whitelist");
console.log(" - Withdraw all accumulated fees");
console.log(" - Potentially pause trading (if implemented)");
console.log("");
console.log("Attacker CANNOT:");
console.log(" - Mint NFTs directly (owner restriction)");
console.log(" - But can whitelist attacker and mint that way");
console.log("");
console.log("CONCLUSION: Restriction provides minimal security");
console.log(" - Attacker can still mint via whitelisted address");
console.log(" - Consider removing or strengthening restriction");
}
function test_M04_D_ownerCanWhitelistAttackerWhoThenMints() public {
console.log("=== OWNER RESTRICTION BYPASS ===");
console.log("");
console.log("Owner cannot mint directly, BUT:");
console.log("");
vm.startPrank(owner);
nftDealers.revealCollection();
vm.stopPrank();
vm.startPrank(owner);
nftDealers.whitelistWallet(buyer);
vm.stopPrank();
console.log("1. Owner whitelists attacker");
vm.startPrank(buyer);
usdc.approve(address(nftDealers), lockAmount);
nftDealers.mintNft{value: lockAmount}();
vm.stopPrank();
console.log("2. Attacker mints NFT successfully");
console.log("3. Owner restriction is easily bypassed");
console.log("");
console.log("VULNERABILITY: Restriction provides false security");
}
}
Recommended Mitigation
The fix either removes the owner mint restriction entirely (if owner should be able to mint) or makes it consistent across all functions. The restriction should be clearly documented if intentional.
- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
- if (totalSupply >= MAX_SUPPLY) revert MaxSupplyReached();
- if (msg.sender == owner) revert InvalidAddress(); // ❌ Remove this
- if (msg.sender == address(0)) revert InvalidAddress();
-
- IERC20(usdc).safeTransferFrom(msg.sender, address(this), lockAmount);
- _safeTransfer(address(0), msg.sender, totalSupply, "");
-
- collateralForMinting[totalSupply] = lockAmount;
- }
+ function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ if (totalSupply >= MAX_SUPPLY) revert MaxSupplyReached();
+ if (msg.sender == address(0)) revert InvalidAddress();
+ // ✅ Owner can mint if whitelisted - consistent with other users
+
+ IERC20(usdc).safeTransferFrom(msg.sender, address(this), lockAmount);
+ _safeTransfer(address(0), msg.sender, totalSupply, "");
+
+ collateralForMinting[totalSupply] = lockAmount;
+ }
Mitigation Explanation: The fix addresses the root cause by: (1) Removing the if (msg.sender == owner) revert InvalidAddress() check from mintNft(), allowing owner to mint like any other whitelisted user, (2) This makes access control consistent - if owner is whitelisted, they can mint; if not whitelisted, they cannot, (3) If the restriction is intentional for business reasons, it should be clearly documented in the whitepaper and code comments, (4) Alternatively, if owner should never mint, the whitelistWallet() function should prevent owner from being whitelisted, making the restriction consistent across all functions, (5) The current implementation provides false security while being trivially bypassed - either remove it or enforce it properly.
