TangibleNFTV2 Code Style Findings

TNT-01C: Inefficient Array Shift

Type	Severity	Location
Gas Optimization		TangibleNFTV2.sol:L419, L421, L423, L425

Description:

The referenced array shift operation will assign the last element of the array to the index of the element to-be-removed, however, this operations is inefficiently performed when the index of the element-to-be-removed is the actual last element of the array.

Example:

Copy
418// take last element
419uint256 last = tokenFeatures[tokenId][tokenFeatures[tokenId].length - 1];
420// set it to index
421tokenFeatures[tokenId][tokenFeatureAdded[tokenId][feature].index] = last;
422//remove from array
423tokenFeatures[tokenId].pop();
424// delete mapping and update index of the last
425tokenFeatureAdded[tokenId][last].index = tokenFeatureAdded[tokenId][feature].index;

Recommendation:

We advise the code to avoid shifting elements if the element-to-be-removed is the last element of the array, performing a direct pop operation instead.

Alleviation (2ad448279d):

The code was updated to directly perform a pop operation when it is the last element, however, the featureInfo[feature].index the last element is assigned to may be equal to features.length - 1 rendering the shift operation still unnecessary.

We advise the array shift to be properly optimized under all execution paths rather than solely for the last operation performed.

Alleviation (1f394a00cc):

The optimization was correctly applied by ensuring that the shift operation is solely executed when the last element in the array is different from the element being shifted.

As such, we consider this exhibit to be fully addressed.

TNT-02C: Inefficient Conditional Clauses

Type	Severity	Location
Gas Optimization		TangibleNFTV2.sol:L666-L670, L674-L683

Description:

The referenced if-else structures will all ultimately yield a bool variable.

Example:

Copy
660/**
661 * @notice This internal method is used to return the boolean value of whether storage has expired for a token.
662 * @param tokenId TNFT identifier to see if storage has been expired.
663 * @return If true, storage has not expired, otherwise false.
664 */
665function _isStorageFeePaid(uint256 tokenId) internal view returns (bool) {
666    //logic for no storage
667    if (!_shouldPayStorage()) {
668        return true;
669    }
670    return storageEndTime[tokenId] > block.timestamp;
671}
672
673function _shouldPayStorage() internal view returns (bool) {
674    if (storageRequired) {
675        if (
676            (storagePriceFixed && storagePricePerYear == 0) ||
677            (!storagePriceFixed && storagePercentagePricePerYear == 0)
678        ) {
679            return false;
680        }
681        return true;
682    }
683    return false;
684}

Recommendation:

We advise all clauses to be combined into a single, concise one thus optimizing the legibility as well as gas cost of the statements.

Alleviation (2ad448279d):

The optimization has been applied solely for the TangibleNFTV2::_shouldPayStorage function, rendering it partially applied.

Alleviation (2135afa89e):

The optimization has been fully applied to the first referenced segment, addressing this exhibit fully.

TNT-03C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		TangibleNFTV2.sol:L392, L394, L396-L398, L417, L419, L421, L423, L425, L426

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

Copy
381function addMetadata(
382    uint256 tokenId,
383    uint256[] calldata _features
384) external onlyCategoryOwner(ITangibleNFT(address(this))) {
385    require(_ownerOf(tokenId) != address(0), "token not minted");
386    ITNFTMetadata tnftMetadata = ITNFTMetadata(IFactory(factory()).tnftMetadata());
387    uint256 length = _features.length;
388
389    for (uint256 i; i < length; ) {
390        uint256 feature = _features[i];
391        require(tnftMetadata.featureInType(tnftType, feature), "feature not in tfntType");
392        require(!tokenFeatureAdded[tokenId][feature].added, "already added");
393        // add to array
394        tokenFeatures[tokenId].push(feature);
395        // add in map
396        tokenFeatureAdded[tokenId][feature].added = true;
397        tokenFeatureAdded[tokenId][feature].feature = feature;
398        tokenFeatureAdded[tokenId][feature].index = tokenFeatures[tokenId].length - 1;
399        emit TnftFeature(tokenId, feature, true);
400
401        unchecked {
402            ++i;
403        }
404    }
405}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

All referenced inefficient mapping lookups have been optimized to the greatest extent possible, significantly reducing the gas cost of the functions the statements were located in.

TNT-04C: Redundant Parenthesis Statement

Type	Severity	Location
Code Style		TangibleNFTV2.sol:L631

Description:

The referenced statement is redundantly wrapped in parenthesis (()).

Example:

Copy
631(_factory == from) ||

Recommendation:

We advise them to be safely omitted, increasing the legibility of the codebase.

Alleviation (2ad448279d9e8e4b6edd94bcd2eb22129b6f7357):

The redundant parenthesis in the referenced statement have been safely omitted.