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

Flawed logics in determining a NFT transaction occurs and pays out in `collectUsdcFromSelling()`

Author Revealed upon completion

Root + Impact

Description

  • As specified in the README, the protocol should allow sellers collecting its proceeds (listing.price - fees) after selling an NFT.

  • The normal calling order would be like this

    • buyer calls buy(listingId) and succeeds. <- this set its listing.isActive to false.

    • seller calls collectUsdcFromSelling(listingId) and succeeds.


  • Now an attacker can:

    • attacker calls buy(listingId) and succeeds

    • attacker calls list(listingId) <- This call assigns the buyer address to the listing.seller and set the listing.price .

    • attacker calls cancelListing(listingId) <- This call sets the listing.isActive to false.

    • attacker calls collectUsdcFromSelling(listingId) <- Now the attacker collects the listing.price - fees from the protocol, given the protocol still hold enough USDC balance for the pay out.

The root cause is that the protocol is using listing.isActive to determine if a NFT transaction has occured in collectUsdcFromSelling()but it is also used to indicate if the listing is active or not and can be turned on or off by list() and cancelListing().

// Root cause in the codebase with @> marks to highlight the relevant section
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
@> require(!listing.isActive, "Listing must be inactive to collect USDC");
​
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:

  • The attacker can make the above call orders: 1) buy, 2) list, 3) cancelListing, 4) collectUsdcFromSelling to exploit this vulnerability.

Impact:

  • The attacker can withdraw all the USDC balance of the protocol/smart contract.

Proof of Concept

The attacker can perform the following series of calls to exploit this vulnerability

// called by attacker
vm.startPrank(attacker);
​
// Assume attacker is not a whitelisted user and need to buy one NFT to attack the protocol
nftDealers.buy(1);
nftDealers.list(1, 10e6);
nftDealers.cancelListing(1);
nftDealers.collectUsdcFromSelling(1);
nftDealers.collectUsdcFromSelling(1);
nftDealers.collectUsdcFromSelling(1);
//... keep calling collectUsdcFromSelling() function until the function reverts

Recommended Mitigation

Without introducing another state variable, an easy mitigation is to ensure buy() and collectUsdcFromSelling() are being called atomically. They can be seen in the following mitigation code.

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;
+ _collectUsdcFromSelling(_listingId);
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
​
- function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
+ function _collectUsdcFromSelling(uint256 _listingId) internal onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
​
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);
+ usdc.safeTransfer(listing.seller, amountToSeller);
}
​

Support

FAQs

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

Give us feedback!