Basket Code Style Findings

BTE-01C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		Basket.sol:L296, L297, L348, L351, L673, L687, L786, L790, L798, L799, L860, L1044, L1076, L1077

Description:

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

Example:

Copy
284/**
285 * @notice This method is executed upon the callback of vrf for entropy. This method will use the randomWord to select
286 *         a ranom index from the `depositedTnfts` array to be next redeemable NFT.
287 * @param randomWord Random uint seed received from vrf.
288 */
289function fulfillRandomSeed(uint256 randomWord) external onlyBasketVrfConsumer {
290
291    // choose a nft to be next redeemable
292
293    uint256 index;
294    index = randomWord % depositedTnfts.length;
295
296    address tnft = depositedTnfts[index].tnft;
297    uint256 tokenId = depositedTnfts[index].tokenId;
298
299    nextToRedeem = RedeemData(tnft, tokenId);
300    emit RedeemableChosen(tnft, tokenId);
301    
302    seedRequestInFlight = false;
303    pendingSeedRequestId = 0;
304}

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 (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team evaluated this exhibit but opted to acknowledge it in the current iteration of the codebase

BTE-02C: Inexistent Error Messages

Type	Severity	Location
Code Style		Basket.sol:L170, L488, L491

Description:

The linked require checks have no error messages explicitly defined.

Example:

Copy
170require(msg.sender == _getBasketVrfConsumer());

Recommendation:

We advise each to be set so to increase the legibility of the codebase and aid in validating the require checks' conditions.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

A proper textual description of the condition validated has been introduced to all referenced require checks thus addressing this exhibit.

BTE-03C: Multiple Expensive Balance Measurements

Type	Severity	Location
Gas Optimization		Basket.sol:L876, L883, L886

Description:

The Basket::_transferRent function will inefficiently query the balance of the contract itself multiple times.

Example:

Copy
874// start iterating through the master claimable rent array claiming rent for each token.
875uint256 index;
876while (_withdrawAmount > primaryRentToken.balanceOf(address(this)) && index < counter) {
877
878    IRentManager rentManager = _getRentManager(claimableRent[index].tnft);
879    uint256 tokenId = claimableRent[index].tokenId;
880
881    if (rentManager.claimableRentForToken(tokenId) > 0) {
882
883        uint256 preBal = primaryRentToken.balanceOf(address(this));
884        uint256 claimedRent = rentManager.claimRentForToken(tokenId);
885
886        require(primaryRentToken.balanceOf(address(this)) == (preBal + claimedRent), "claiming error");
887    }
888
889    unchecked {
890        ++index;
891    }
892}

Recommendation:

We advise the balance to be tracked locally, significantly reducing the overall gas cost required by the function.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The code was updated to track the balance of the contract before the while loop and to validate that the pre-loop balance plus the cumulative claimedRent value equals the post-loop balance of the contract, significantly optimizing the gas cost of the function whilst retaining the same security guarantees.

BTE-04C: Potentially Inefficient Array Shifts

Type	Severity	Location
Gas Optimization		Basket.sol:L794, L798, L803

Description:

The Basket::_redeemTNFT function will perform multiple array shift operations as it removes an entry from at minimum two arrays and at maximum three arrays. These operations are inefficiently performed under certain conditions.

Example:

Copy
792uint256 index;
793(index,) = _isDepositedTnft(_tangibleNFT, _tokenId);
794depositedTnfts[index] = depositedTnfts[depositedTnfts.length - 1];
795depositedTnfts.pop();
796
797(index,) = _isTokenIdLibrary(_tangibleNFT, _tokenId);
798tokenIdLibrary[_tangibleNFT][index] = tokenIdLibrary[_tangibleNFT][tokenIdLibrary[_tangibleNFT].length - 1];
799tokenIdLibrary[_tangibleNFT].pop();
800
801if (tokenIdLibrary[_tangibleNFT].length == 0) {
802    (index,) = _isSupportedTnft(_tangibleNFT); 
803    tnftsSupported[index] = tnftsSupported[tnftsSupported.length - 1];
804    tnftsSupported.pop();
805}

Recommendation:

We advise the referenced assignments to be performed solely when the index that is being removed is not the last element in the array as otherwise they will execute a costly self-assignment redundantly.

Alleviation (106fc61dcd):

The code was updated per our recommendation solely for the two out of three referenced instances, rendering it partially alleviated.

Alleviation (d7d2436ede):

The last referenced array shift instance was optimized as well rendering the exhibit fully applied.

BTE-05C: Redundant Assignment & Addition Operations

Type	Severity	Location
Gas Optimization		Basket.sol:L600, L732

Description:

The referenced assignment-and-addition operations (+=) are redundant as they are performed on a variable that has a value of 0.

Example:

Copy
598unchecked {
599    // get total value of nfts in basket.
600    totalValue += totalNftValue;
601
602    // get value of rent accrued by this contract.
603    totalValue += totalRentValue * decimalsDiff();
604}

Recommendation:

We advise direct assignments to be performed and the statements to be relocated outside their unchecked code blocks as no form of arithmetic is needed to be performed for them.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

Both referenced addition-and-assignments statements have been replaced by direct assignments, optimizing the code's gas cost.

BTE-06C: Redundant Evaluation of Claimable Rent

Type	Severity	Location
Gas Optimization		Basket.sol:L881

Description:

The way the claimableRent array is constructed guarantees that all entries within it have a non-zero rent balance to be claimed.

Example:

Copy
848// iterate through all TNFT contracts supported by this basket.
849for (uint256 i; i < tnftsSupported.length;) {
850    address tnft = tnftsSupported[i];
851
852    // for each TNFT supported, make a batch call to the rent manager for all rent claimable for the array of tokenIds.
853    uint256[] memory claimables = _getRentManager(tnft).claimableRentForTokenBatch(tokenIdLibrary[tnft]);
854
855    // iterate through the array of claimable rent for each tokenId for each TNFT and push it to the master claimableRent array.
856    for (uint256 j; j < claimables.length;) {
857        uint256 amountClaimable = claimables[j];
858
859        if (amountClaimable > 0) {
860            claimableRent[counter] = RentData(tnft, tokenIdLibrary[tnft][j], amountClaimable);
861            unchecked {
862                ++counter;
863            }
864        }
865        unchecked {
866            ++j;
867        }
868    }
869    unchecked {
870        ++i;
871    }
872}
873
874// start iterating through the master claimable rent array claiming rent for each token.
875uint256 index;
876while (_withdrawAmount > primaryRentToken.balanceOf(address(this)) && index < counter) {
877
878    IRentManager rentManager = _getRentManager(claimableRent[index].tnft);
879    uint256 tokenId = claimableRent[index].tokenId;
880
881    if (rentManager.claimableRentForToken(tokenId) > 0) {
882
883        uint256 preBal = primaryRentToken.balanceOf(address(this));
884        uint256 claimedRent = rentManager.claimRentForToken(tokenId);
885
886        require(primaryRentToken.balanceOf(address(this)) == (preBal + claimedRent), "claiming error");
887    }
888
889    unchecked {
890        ++index;
891    }
892}

Recommendation:

We advise the referenced if conditional to be omitted, greatly optimizing the function's gas cost.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The redundant evaluation of claimable rent has been omitted, optimizing the loop's gas cost significantly.

BTE-07C: Redundant Invocations of Decimals

Type	Severity	Location
Gas Optimization		Basket.sol:L590, L651

Description:

The Basket::decimals function is guaranteed to yield a value of 1e18 on all invocations as its value is never overridden.

Example:

Copy
590sharePrice = (getTotalValueOfBasket() * 10 ** decimals()) / totalSupply();

Recommendation:

To ensure consistency with the rebase system that uses 1e18 as a base, we advise the refenced instances to utilize the 1e18 representation and potentially the contract-level constant of it we have proposed in a separate exhibit.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team evaluated this exhibit but opted to acknowledge it in the current iteration of the codebase

BTE-08C: Redundant Parenthesis Statements

Type	Severity	Location
Code Style		Basket.sol:L751

Description:

The referenced statements are redundantly wrapped in parenthesis' (()).

Example:

Copy
751if ((nextToRedeem.tnft == address(0)) && (!seedRequestInFlight)) {

Recommendation:

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

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The redundant parenthesis have been omitted as advised, increasing the legibility of the codebase.

BTE-09C: Redundant Yield of Boolean

Type	Severity	Location
Gas Optimization		Basket.sol:L1044, L1049, L1077, L1082

Description:

The Basket::_isDepositedTnft and Basket::_isTokenIdLibrary functions are invoked in locations where they are guaranteed to yield true.

Example:

Copy
1035/**
1036 * @notice This helper method returns whether a provided TNFT token exists in the depositedTnfts array and if so, where in the array.
1037 * @param _tnft contract address.
1038 * @param _tokenId TokenId of token being fetched.
1039 * @return index -> Where in the `depositedTnfts` array the specified token resides.
1040 * @return exists -> If token exists in `depositedTnfts`, will be true. Otherwise false.
1041 */
1042function _isDepositedTnft(address _tnft, uint256 _tokenId) internal view returns (uint256 index, bool exists) {
1043    for(uint256 i; i < depositedTnfts.length;) {
1044        if (depositedTnfts[i].tokenId == _tokenId && depositedTnfts[i].tnft == _tnft) return (i, true);
1045        unchecked {
1046            ++i;
1047        }
1048    }
1049    return (0, false);
1050}

Recommendation:

We advise the bool value yielded by each function to be omitted, optimizing their overall gas cost.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The referenced functions are no longer present in the codebase, rendering the exhibit no longer applicable.

BTE-10C: Repetitive Value Literals

Type	Severity	Location
Code Style		Basket.sol:L229, L567, L587

Description:

The linked value literals are repeated across the codebase multiple times.

Example:

Copy
229_setRebaseIndex(1 ether);

Recommendation:

We advise each to be set to its dedicated constant variable instead optimizing the legibility of the codebase.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team evaluated this exhibit but opted to acknowledge it in the current iteration of the codebase

BTE-11C: Suboptimal Struct Declaration Styles

Type	Severity	Location
Code Style		Basket.sol:L299, L564, L690, L718, L754, L860

Description:

The linked declaration styles of the referenced structs are using index-based argument initialization.

Example:

Copy
299nextToRedeem = RedeemData(tnft, tokenId);

Recommendation:

We advise the key-value declaration format to be utilized instead in each instance, greatly increasing the legibility of the codebase.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team evaluated this exhibit but opted to acknowledge it in the current iteration of the codebase