Dria
Swan
NFTHardhat
21,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Sellers can brick tasks from completing by transfering the NFTs away

pyro

Summary

Any seller who's NFT is selected can brick the rest of the sellers from receiving their profits, by just transferring the NFT away. This can be done also by the buyer (if one of his contracts gets to be the seller) in order to avoid paying anything.

Vulnerability Details

Sellers list NFTs and buyers buy them. However buyer cannot chose which NFT to be, but they get recommended 1 or more from the highest scoring generator. The process is:

sellers lists -> buyer makes a request -> generators respond to it -> verifiers validate it -> responses get scored -> highest gets returned from getBestResponse -> that response is returned by oracleResult , which is finally used by purchase

With that out of the way we can see that when the buyer chooses to buy he gets offered an array of items (which all need to be bought, he cannot chose which or how many) and the buyer buys them all.

However an issue occurs here, as the items bought still belong to the seller. More precisely on list the seller creates a new ERC721 and gets minted ID 1:

address asset = address(swanAssetFactory.deploy(_name, _symbol, _desc, msg.sender));

However as that asset remains in the seller he has 100% control over it, and if an asset belonging to this seller gets to be one of the suggested assets then right before the buy our seller can call transferFrom on the asset and transfer it to another address. Later when purchase is called from the buyer it will loop and try to purchase all of the assets at once

for (uint256 i = 0; i < assets.length; i++) {
address asset = assets[i];
​
// must not exceed the roundly buy-limit
uint256 price = swan.getListingPrice(asset);
spendings[round] += price;
if (spendings[round] > amountPerRound) {
revert BuyLimitExceeded(spendings[round], amountPerRound);
}
​
// add to inventory
inventory[round].push(asset);
​
// make the actual purchase
swan.purchase(asset);
}

And when it reaches our faulty asset purchase, this time inside Swan will revert as it tries to transfer the asset from our seller, but the asset is inside another contract.

// transfer asset from seller to Swan, and then from Swan to buyer
// this ensure that only approval to Swan is enough for the sellers
SwanAsset(_asset).transferFrom(listing.seller, address(this), 1);
SwanAsset(_asset).transferFrom(address(this), listing.buyer, 1);

Impact

  1. Sellers grief other sellers from their profits for minimal cost

  2. Buyer has a high likelihood of blocking a round from completion, getting all of the royalties, while paying 0 to any seller.

  3. The whole game experience is ruined

Tools Used

Manual review

Recommendations

Either change _update inside SwanAsset, or transfer the NFTs inside Swan when the seller lists them. The simpler solution is to mint the NFT directly to Swan:

contract SwanAsset is ERC721, Ownable {
// code ...
​
description = _description;
createdAt = block.timestamp;
​
- ERC721._mint(_owner, 1);
+ ERC721._mint(_operator, 1);
​
// Swan (operator) is approved to by the owner immediately.
ERC721._setApprovalForAll(_owner, _operator, true);
}
}

If you plan for the seller to move his NFTs between marketplaces, then considered transferring the NFT to Swan when the seller lists and transferring it back to him when he unlists (currently not implemented).

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

DoS in BuyerAgent::purchase

Support

FAQs

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