FactoryV2 Manual Review Findings

FV2-01M: Incorrect Adjustment of Tangible Labs Address

Type	Severity	Location
Logical Fault		FactoryV2.sol:L325

Description:

The FactoryV2::_setContract logic will attempt to migrate the fingerprint approval managers of each tnft owned by the previous tangibleLabs address, however, in doing so it may overwrite manually set fingerprint approval managers.

Impact:

Manually set fingerprint approval managers will be incorrectly overwritten when the tangibleLabs address is updated.

Example:

Copy
319old = tangibleLabs;
320for (uint256 i; i < ownedByLabs.length; ) {
321    ITangibleNFT tnft = ownedByLabs[i];
322    categoryOwner[tnft] = _contractAddress;
323
324    // fingerprint approval manager should change also
325    _setFingerprintApprovalManager(tnft, _contractAddress);
326    emit CategoryOwner(address(tnft), msg.sender);
327
328    unchecked {
329        ++i;
330    }
331}

Recommendation:

We advise the code to solely set the fingerprint approval manager to the _contractAddress if the previous fingerprint approval manager was the old address. In any other case, the data entry should remain untouched.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The Tangible team evaluated this exhibit and has specified that it represents intended behaviour.

Specifically, a migration should involve all addresses being updated and the approval managers to be configured at a secondary transaction. Based on this, we consider the exhibit nullified as it represents desirable behaviour.

To note, this finding led to a different misbehaviour being uncovered in FactoryV2::newCategory that was corrected whereby fingerprintApprovalManager entries should solely be configured for the tangibleLabs team.

FV2-02M: Incorrect Price Assumption

Type	Severity	Location
Mathematical Operations		FactoryV2.sol:L440

Description:

The paymentToken ultimately utilized to pay for the storage of a tnft may have a significantly lower value-per-unit than the token that the ITangibleNFT::storagePricePerYear value was configured for.

Impact:

Depending on the price parity between the paymentToken and the storagePricePerYear value, the impact could result in a significantly lower evaluation of storage costs.

Example:

Copy
440toDecimals(tnft.storagePricePerYear(), decimals, paymentToken.decimals()) *

Recommendation:

We advise the code of ITangibleNFT to properly convert between the two assets, respecting the decimal as well as value difference between them.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The Tangible team specified that the paymentToken will solely represent a stablecoin asset and that the storagePricePerYear is denominated in the United States Dollar (USD).

The problem that arises from this is that the stablecoins are never at parity with the US dollar, as evidenced by Chainlink's oracles. While the deviations may be small, they can still open up arbitrage opportunities that can be exacerbated depending on the sum of funds that is considered at parity with the USD.

In any case, we consider this exhibit acknowledged and will request the Tangible team to revisit in case they wish to perform any alleviation for it.

FV2-03M: Inexistent Prevention of Default Payment Token Configuration

Type	Severity	Location
Input Sanitization		FactoryV2.sol:L272-L275

Description:

The FactoryV2::configurePaymentToken function permits the defUSD token to be disabled from the paymentTokens configuration mapping.

Impact:

It is presently possible to disable the default payment method of the system which is undesirable based on the require check imposed in the FactoryV2::setDefaultStableUSD function.

Example:

Copy
272function configurePaymentToken(IERC20 token, bool value) external onlyOwner {
273    paymentTokens[token] = value;
274    emit PaymentToken(address(token), value);
275}

Recommendation:

We advise the code to ensure that the token being adjusted is not the defUSD via a require check.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The Tangible team evaluated this exhibit and stated that this is by design to ensure that the team is able to react to extreme market events, such as the de-pegging of the USDR token. As such, we consider it desirable behaviour and thus void.

FV2-04M: Incorrect Decimal Division

Type	Severity	Location
Mathematical Operations		FactoryV2.sol:L446

Description:

The referenced division will improperly divide the tokenPrice value, which is in paymentToken decimals, by the storageDecimals which are unrelated.

Impact:

The amount to be paid for a particular set of years of storage will be significantly lower than the expected one depending on the configuration of the storageDecimals value.

Example:

Copy
436uint8 decimals = tnft.storageDecimals();
437
438if (storagePriceFixed) {
439    amount =
440        toDecimals(tnft.storagePricePerYear(), decimals, paymentToken.decimals()) *
441        _years;
442} else {
443    require(tokenPrice > 0, "Price 0");
444    amount =
445        (tokenPrice * tnft.storagePercentagePricePerYear() * _years) /
446        (100 * (10 ** decimals));
447}

Recommendation:

We advise the code to not perform a decimal normalization as the amount will ultimately be paid in the paymentToken and thus contain the correct number of decimals for it.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The Tangible team evaluated this exhibit and stated that the calculation is correct due to the value of storagePricePerYear being in a different denomination. The Tangible team proceeded to document the relevant code block and we thus consider this exhibit to be alleviated.

FV2-05M: Inexistent Validation of Seize Expiry Duration

Type	Severity	Location
Logical Fault		FactoryV2.sol:L684-L689

Description:

The FactoryV2::setCategoryStorageExpire function will permit any numDays value to be set for a storage's expiry, enabling the FactoryV2::seizeTnft function to be invoked with a potential expiryDays of even 1.

Impact:

It is presently possible for a malicious category owner to set a significantly low daysBeforeSeize value, causing TNFT tokens that should otherwise be in the custody of other users to be seizable.

Example:

Copy
679/**
680 * @notice This function allows the category owner to add a storage expiration to a category.
681 * @param tnft TangibleNFT contract.
682 * @param numDays amount of days before storage expires.
683 */
684function setCategoryStorageExpire(
685    ITangibleNFT tnft,
686    uint256 numDays
687) external onlyCategoryOwner(tnft) {
688    daysBeforeSeize[tnft] = numDays;
689}
690
691/**
692 * @notice This function allows a category owner to seize NFTs that are expired
693 * @dev If a token is expired, the storage has not been paid for.
694 * @param tnft TangibleNFT contract.
695 * @param tokenIds Array of tokenIds to seize.
696 */
697function seizeTnft(
698    ITangibleNFT tnft,
699    uint256[] memory tokenIds
700) external onlyCategoryOwner(tnft) {
701    uint256 length = tokenIds.length;
702    uint256 expiryDays = daysBeforeSeize[tnft] != 0
703        ? daysBeforeSeize[tnft]
704        : DEFAULT_SEIZE_DAYS;
705    for (uint256 i; i < length; ) {
706        uint256 token = tokenIds[i];
707
708        require(tnft.storageEndTime(token) + expiryDays * 1 days < block.timestamp);
709
710        address ownerTnft = tnft.ownerOf(token);
711        tnft.safeTransferFrom(ownerTnft, categoryOwner[tnft], token);
712
713        unchecked {
714            ++i;
715        }
716    }
717}

Recommendation:

We advise a sensible minimum amount of days to be enforced by the FactoryV2::setCategoryStorageExpire function, preventing malicious seize operations from being performed.

Alleviation (2ad448279d):

The Tangible team evaluated this exhibit and stated that they do not wish to influence how integrators and vendors integrate and configure the protocol.

While we understand their approach, we believe that enforcing sensible and fairness-oriented minimums is an acceptable trait of a decentralized protocol.

Based on the aforementioned rationale, we will consider this submission as acknowledged rather than nullified.

Alleviation (2135afa89e):

The Tangible team revisited this exhibit and is now enforcing a minimum duration of 30 days for a particular NFT being seizable, alleviating this exhibit in full.

FV2-06M: Insufficient Validation of Seizure Validity

Type	Severity	Location
Logical Fault		FactoryV2.sol:L708

Description:

The FactoryV2::seizeTnft function will permit a Tangible NFT to be seized if its rent has not been paid for a significant amount of time, however, the system does not actually validate that the Tangible NFT has rent in the first place.

As such, it is possible for a Tangible NFT without any rent to be seized at any time incorrectly.

Impact:

It is presently possible to execute unauthorized seizures in two ways which can have a harmful effect in the operation of a Tangible NFT.

Example:

Copy
697function seizeTnft(
698    ITangibleNFT tnft,
699    uint256[] memory tokenIds
700) external onlyCategoryOwner(tnft) {
701    uint256 length = tokenIds.length;
702    uint256 expiryDays = daysBeforeSeize[tnft] != 0
703        ? daysBeforeSeize[tnft]
704        : DEFAULT_SEIZE_DAYS;
705    for (uint256 i; i < length; ) {
706        uint256 token = tokenIds[i];
707
708        require(tnft.storageEndTime(token) + expiryDays * 1 days < block.timestamp);
709
710        address ownerTnft = tnft.ownerOf(token);
711        tnft.safeTransferFrom(ownerTnft, categoryOwner[tnft], token);
712
713        unchecked {
714            ++i;
715        }
716    }
717}

Recommendation:

We advise the code to ensure that the Tangible NFT ITangibleNFT::storageRequired value is true, preventing contracts without storage being required from being seized.

Additionally and to further avoid unauthorized seizures, we advise the toggle of the storageRequired state in the TangibleNFTV2::toggleStorageRequired function to implement a "grace" period of at least a few days that permits Tangible NFT holders to properly pay their rent.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The code within FactoryV2::seizeTnft was significantly refactored to accommodate for NFTs that do not need to pay rent but have been blacklisted, in which case they should still be seizable.

As a result of these changes, we consider the original vulnerability no longer applicable and thus the code corrected.