Root + Impact
Description
-
mintNft() and buy() are payment functions that should only accept USDC as the payment token. Any ETH sent alongside these calls should either be rejected or refunded to the caller.
-
Both functions are marked payable but neither use msg.value nor refund it. The contract has no receive(), no fallback(), and no withdrawEth() function. Any ETH sent by a user is permanently trapped inside the contract with no recovery path.
@> function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
activeListingsCounter--;
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
@> function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
if (msg.sender == address(0)) revert InvalidAddress();
require(tokenIdCounter < MAX_SUPPLY, "Max supply reached");
require(msg.sender != owner, "Owner can't mint NFTs");
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
Risk
Likelihood:
Impact:
Proof of Concept
This test uses the same setup as the NFTDealersTest file, extended with ETH funding for both users via vm.deal. It proves that both mintNft and buy are marked payable and can therefore accept ETH, but the contract provides no withdrawal function, locking any sent ETH permanently.
function testEthSentToMintNftAndBuyAreLockedForever() public revealed {
uint256 ethToSend = 1 ether;
vm.deal(userWithEvenMoreCash, 10 ether);
vm.deal(userWithCash, 10 ether);
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
usdc.mint(userWithEvenMoreCash, 20e6);
uint256 contractEthBefore = address(nftDealers).balance;
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft{value: ethToSend}();
vm.stopPrank();
assertEq(address(nftDealers).balance, contractEthBefore + ethToSend, "ETH locked after mintNft");
uint32 price = 500e6;
vm.startPrank(userWithEvenMoreCash);
nftDealers.list(1, price);
vm.stopPrank();
usdc.mint(userWithCash, price);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), price);
nftDealers.buy{value: ethToSend}(1);
vm.stopPrank();
uint256 contractEthAfter = address(nftDealers).balance;
assertEq( contractEthAfter, contractEthBefore + ethToSend * 2, "ETH locked after buy");
}
Recommended Mitigation
Remove the payable modifier from functions that are not intended to receive ETH.
- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
activeListingsCounter--;
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ function mintNft() external onlyWhenRevealed onlyWhitelisted {
if (msg.sender == address(0)) revert InvalidAddress();
require(tokenIdCounter < MAX_SUPPLY, "Max supply reached");
require(msg.sender != owner, "Owner can't mint NFTs");
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
