MarketplaceV2 Manual Review Findings

MV2-01M: Inexistent Restriction of Storage Payment Amount

Type	Severity	Location
Logical Fault		MarketplaceV2.sol:L373-L378

Description:

The MarketplaceV2::_payStorage function utilized throughout the marketplace's purchase workflows does not permit the buyer to actually validate the amount of funds they will expense to pay for the storage of a Tangible NFT.

Impact:

While less impactful than the peer-to-peer price hike identified in another exhibit of the MarketplaceV2 contract, the manager of a Tangible NFT can still manipulate its configuration to impact marketplace trades of it negatively and force buyers to pay an unexpected amount of funds for rent that may not even have existed when they initialized their transaction.

Example:

Copy
364function _payStorage(
365    ITangibleNFT nft,
366    IERC20Metadata paymentToken,
367    uint256 tokenId,
368    uint256 _years
369) internal {
370    require(nft.storageRequired(), "STNR");
371    require(_years > 0, "YZ");
372
373    uint256 amount = IFactory(factory()).adjustStorageAndGetAmount(
374        nft,
375        paymentToken,
376        tokenId,
377        _years
378    );
379    //we take in default USD token
380    IERC20(address(paymentToken)).safeTransferFrom(
381        msg.sender,
382        IFactory(factory()).categoryOwnerWallet(nft),
383        amount
384    );
385    emit StorageFeePaid(address(nft), tokenId, address(paymentToken), msg.sender, amount);
386}

Recommendation:

We advise an upper bound to be enforced on the amount the buyer is willing to pay for storage, ensuring that users cannot arbitrarily toggle their ITangibleNFT::storageRequired configurations and / or set incredibly high rent prices before a sale takes place.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

A _maxAmount argument has been introduced in MarketplaceV2::_payStorage and its call chain, permitting users to control the maximum price they are willing to pay for a token's storage.

As such, we consider this exhibit fully alleviated.

MV2-02M: Insecure Validation of Tangible NFT Existence

Type	Severity	Location
Logical Fault		MarketplaceV2.sol:L603, L607

Description:

The MarketplaceV2::_onERC721Received function will validate that the caller contract is a valid Tangible NFT by evaluating that the ITangibleNFT::name is a category of the contract's factory, however, this is an incorrect approach as a malicious nft can yield any value in its ITangibleNFT::name function.

Impact:

It is presently possible for any NFT, including malicious implementations, to set sales and update the TNFT trackers via the MarketplaceV2::onERC721Received function that is insecurely guarded.

Example:

Copy
606require(
607    address(IFactory(factory()).category(ITangibleNFT(nft).name())) != address(0),
608    "Not TNFT"
609);

Recommendation:

We advise the code to instead ensure that the caller nft is part of the Tangible ecosystem by mandating that the result of the IFactory::category call is equal to the nft rather than not equal to the zero-address per the FactoryV2::newCategory implementation.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The require conditional was revised as advised, ensuring that the output of the IFactory::category lookup matches the nft itself rather than resulting in a non-zero entry.

As such, we consider this exhibit properly alleviated.

MV2-03M: Inexistent Validation of Lot Token & Price

Type	Severity	Location
Logical Fault		MarketplaceV2.sol:L237-L238, L319

Description:

The MarketplaceV2::buy function offers no form of validation for the buyer in relation to the paymentToken as well as price they will ultimately pay for a Tangible NFT. Coupled with the fact that it is trivial for a seller to update these values via the MarketplaceV2::sellBatch function, it is possible for a malicious seller to change their payment token and / or price at the last minute before a buyer's purchase operation goes through.

Impact:

It is trivial to maliciously update the price and / or payment token of a Tangible NFT right before it is purchased to extract more funds than expected by the NFT's buyer.

Example:

Copy
319function buy(ITangibleNFT nft, uint256 tokenId, uint256 _years) external nonReentrant {
320    //pay for storage
321    if ((!nft.isStorageFeePaid(tokenId) || _years > 0) && nft.storageRequired()) {
322        require(_years > 0, "YZ");
323        _payStorage(
324            nft,
325            IERC20Metadata(address(marketplaceLot[address(nft)][tokenId].paymentToken)),
326            tokenId,
327            _years
328        );
329    }
330    // if initial sale is not done, and whitelitsing is required, check if buyer is whitelisted
331    if (
332        IFactory(factory()).onlyWhitelistedForUnmintedCategory(nft) &&
333        !initialSaleCompleted[nft][tokenId]
334    ) {
335        require(IFactory(factory()).whitelistForBuyUnminted(nft, msg.sender), "NW");
336    }
337    //buy the token
338    _buy(nft, tokenId, true);
339}

Recommendation:

We advise the MarketplaceV2::buy function to accept two more arguments indicating the paymentToken they wish to pay with as well as the maximum price they are willing to fulfil for the NFT, ensuring that buyers can control the payment method as well as amount they expense for a particular Tangible NFT purchase.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The innermost MarketplaceV2::_buy function was refactored to accept a _paymentToken as well as _price argument that denotes the token that the user wishes to pay in as well as the maximum price they are willing to expend.

All call chains that lead to the MarketplaceV2::_buy function's invocation have been revised to correctly relay these values, thereby alleviating this exhibit in full.