Root + Impact
Description
-
The mint and buy functions are payable which means users can send Eth when calling them, however, it will not be calculated against the price of NFTs.
-
The cost of any NFT will be deducted from the minter or buyer's USDC balance. Therefore, the paid Eth will be wasted from the user's point of view.
@> 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);
}
@> 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);
}
Risk
Likelihood: Low
Impact: High
Proof of Concept
Please copy the following function to the test file and run it using test forge --mt testSendingEthWithMintOrBuyCanCauseTrappedEth.
function testSendingEthWithMintOrBuyCanCauseTrappedEth()
public
whitelisted
revealed
{
deal(userWithCash, 10 ether);
deal(userWithEvenMoreCash, 10 ether);
uint256 initialContractBalance = address(nftDealers).balance;
vm.startBroadcast(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft{value: 1 ether}();
nftDealers.list(1, 21e6);
vm.stopBroadcast();
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), 21e6);
nftDealers.buy{value: 1 ether}(1);
vm.stopBroadcast();
uint256 finalContractBalance = address(nftDealers).balance;
assertGt(
finalContractBalance,
initialContractBalance,
"Sending ETH with mint or buy should be possible"
);
}
Recommended Mitigation
Is solution is quite simple, just make the functions non-payabale as follows.
- 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);
}
- 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);
}
