Basket Manual Review Findings

BTE-01M: Inexplicable Update of Primary Rent Token

Type	Severity	Location
Centralization Concern		Basket.sol:L1030-L1033

Description:

The primaryRentToken data entry is integral within the Basket contract's normal operation and an adjustment of it would significantly influence the evaluation of the Basket, the deposit / withdrawal flows, and the overall system in relation to how it manages rent.

Impact:

While the impact of adjusting the token would be severe, we have assigned a severity of unknown as it relies entirely on how the function is invoked by the privileged Tangible team.

Example:

Copy
1030function _updatePrimaryRentToken(address _primaryRentToken) internal {
1031    require(_primaryRentToken != address(0), "Cannot be == address(0)");
1032    primaryRentToken = IERC20Metadata(_primaryRentToken);
1033}

Recommendation:

We advise a migration to be performed if the primaryRentToken is meant to be adjusted in the future. In its current state, adjusting it would cause the evaluation of the Basket to immediately fluctuate and thus potentially lead to loss of funds and other severe side effects.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team evaluated this exhibit and specified that it is meant as an emergency countermeasure in case the rent token stops working.

The team further elaborated that the function will be invoked by their multi-signature wallet and that a strategy will have been devised and put in effect to smoothly transition the token.

As such, we consider this exhibit alleviated based on the fact that the Tangible team responsibly utilizes the function.

BTE-02M: Inexplicable Usage of Unchecked Arithmetic

Type	Severity	Location
Mathematical Operations		Basket.sol:L598-L604, L747

Description:

The referenced unchecked arithmetic code blocks do not perform any validation on their arguments and thus can theoretically overflow.

Example:

Copy
593/**
594 * @notice This method returns the total value of NFTs and claimable rent in this basket contract.
595 * @return totalValue -> total value in 18 decimals.
596 */
597function getTotalValueOfBasket() public view returns (uint256 totalValue) {
598    unchecked {
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    }
605}

Recommendation:

While the likelihood of an overflow is low, we still advise them to be safely performed to avoid potential complications from such overflows occurring in the distant future.

Alleviation (106fc61dcd):

Only one of the two unchecked code blocks referenced has been removed, rendering this exhibit partially alleviated.

Alleviation (d7d2436ede):

The last unchecked code block has been omitted, ensuring all arithmetic operations referenced are performed within safe bounds and thus alleviating this exhibit.

BTE-03M: Improper Assumptions of Interaction

Type	Severity	Location
Logical Fault		Basket.sol:L485, L487-L488

Description:

The Basket::reinvestRent assumes that the target will consume the full approval assigned to it which may not be the case in case the trade performed ultimately requires less funds than expected.

Impact:

In a practical scenario, the purchase of a Tangible NFT will have a fluctuating price that may change between a transaction's submission and a transaction's execution by the network. As such, a scenario in which rent has been "over-allocated" for the purchase of an NFT is likely.

Example:

Copy
473/**
474 * @notice This method allows thew factory owner to reinvest accrued rent.
475 * @dev Ideally `target` would be a contract and `data` would be a method that allows us to purchase another
476 *      property and deposits it into the basket to yield more rent.
477 * @param target Address of contract with reinvest mechanism.
478 * @param rentBalance Amount of rent balance being allocated for reinvestment.
479 * @param data calldata payload for function call.
480 */
481function reinvestRent(address target, uint256 rentBalance, bytes calldata data) external {
482    require(canWithdraw[msg.sender], "Not authorized");
483
484    uint256 basketValueBefore = getTotalValueOfBasket();
485    primaryRentToken.approve(target, rentBalance);
486
487    (bool success,) = target.call(data);
488    require(success);
489    
490    totalRentValue -= rentBalance;
491    require(getTotalValueOfBasket() >= basketValueBefore);
492}

Recommendation:

We advise the code to instead dynamically measure the balance that is utilized and to erase the approval after the external interaction, guaranteeing that the totalRentValue correctly matches the primaryRentToken balance of the contract and that no lingering approvals remain that can be exploited at a secondary transaction.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The code will now properly erase any lingering approval for the target and will additionally calculate the amountUsed as the delta between the pre- and post-balance measurements of the primaryRentToken, alleviating this exhibit in full.

BTE-04M: Improper NFT Receipt Flow

Type	Severity	Location
Standard Conformity		Basket.sol:L543-L545, L699

Description:

The Basket contract will acquire deposited Tangible NFTs by transferring them to itself using an ERC721::safeTransferFrom operation which necessitates the presence of the Basket::onERC721Received function, however, this flow is inefficient.

Impact:

Any Tangible NFTs directly transferred to the Basket will effectively be lost as no rescue function for NFTs exists, rendering this exhibit to be of minor severity.

Example:

Copy
698// take token from depositor
699IERC721(_tangibleNFT).safeTransferFrom(msg.sender, address(this), _tokenId);

Recommendation:

The Basket::onERC721Received function as defined in the EIP-721 standard is meant to facilitate unaware contracts to process incoming NFTs rather than deliberate cross-contract NFT transfers.

As such, we advise the Basket::onERC721Received function to be omitted and the Basket::_depositTNFT function to use an ERC721::transferFrom operation instead, optimizing the gas cost of the overall deployment flow as well as preventing accidental transfers and thus loss of NFT assets to each Basket deployment.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The EIP-721 receipt function is no longer present in the codebase and the referenced ERC721::safeTransferFrom instance has been replaced by an ERC721::transferFrom instruction, optimizing the code as advised and preventing accidental lock of NFTs.

BTE-05M: Incorrect Share Quotations

Type	Severity	Location
Mathematical Operations		Basket.sol:L412-L428, L437-L444, L717-L719

Description:

The Basket::getBatchQuoteIn and Basket::getQuoteIn functions will yield incorrect results as they do not take the deposit fee into account.

Impact:

The quote-in functions of the Basket contract will yield incorrect results that do not match what an actual deposit would yield, leading to potential complications should these functions be integrated by on-chain contracts.

Example:

Copy
403/**
404 * @notice This method is used to quote a batch amount of basket tokens transferred to depositor if a specfied token is deposted.
405 * @dev Does NOT include the amount of basket tokens subtracted for deposit fee.
406 *      The amount of tokens quoted will be slightly different if the same tokens are deposited via batchDepositTNFT.
407 *      Reason being, when tokens are deposited sequentially via batch, the share price will fluctuate in between deposits.
408 * @param _tangibleNFTs Array of TangibleNFT contract addresses of NFTs being quoted.
409 * @param _tokenIds Array of TokenIds of NFTs being quoted.
410 * @return shares -> Array of Erc20 basket tokens quoted for each NFT respectively.
411 */
412function getBatchQuoteIn(address[] memory _tangibleNFTs, uint256[] memory _tokenIds) external view returns (uint256[] memory shares) {
413    uint256 len = _tangibleNFTs.length;
414    shares = new uint256[](len);
415    for (uint i; i < len;) {
416
417        // calculate usd value of TNFT with 18 decimals
418        uint256 usdValue = _getUSDValue(_tangibleNFTs[i], _tokenIds[i]);
419        require(usdValue > 0, "Unsupported TNFT");
420
421        // calculate shares for depositor
422        shares[i] = _quoteShares(usdValue);
423
424        unchecked {
425            ++i;
426        }
427    }
428}
429
430/**
431 * @notice This method is used to quote an amount of basket tokens transferred to depositor if a specfied token is deposted.
432 * @dev Does NOT include the amount of basket tokens subtracted for deposit fee.
433 * @param _tangibleNFT TangibleNFT contract address of NFT being quoted.
434 * @param _tokenId TokenId of NFT being quoted.
435 * @return shares -> Amount of Erc20 basket tokens quoted for NFT.
436 */
437function getQuoteIn(address _tangibleNFT, uint256 _tokenId) external view returns (uint256 shares) {
438    // calculate usd value of TNFT with 18 decimals
439    uint256 usdValue = _getUSDValue(_tangibleNFT, _tokenId);
440    require(usdValue > 0, "Unsupported TNFT");
441
442    // calculate shares for depositor
443    shares = _quoteShares(usdValue);
444}

Recommendation:

We advise them to yield a value that takes the deposit fee into account, ensuring callers can expect a deposit to actually yield the measurement of the quote functions.

Alleviation (106fc61dcd):

The fee is accounted for in the latest implementation solely for the Basket::getQuoteIn function while the Basket::getBatchQuoteIn function remains unfixed, rendering this exhibit partially alleviated.

Alleviation (d7d2436ede):

The Basket::getBatchQuoteIn function was updated to account for the depositFee as well, alleviating this exhibit in full.

BTE-06M: Inexistent Application of Rent Fee

Type	Severity	Location
Logical Fault		Basket.sol:L809-L814

Description:

The Basket::_redeemTNFT will claim the rent pending for the _tokenId being withdrawn but will not distribute it to the revenue distributor as it does not apply the rent fee to it.

Impact:

It is presently possible to bypass the rent fee for a Tangible NFT by redeeming it ahead of a rebase operation.

Example:

Copy
766/**
767 * @notice Internal method for redeeming a specified TNFT from this basket contract.
768 * @param _redeemer EOA address of redeemer. note: msg.sender
769 * @param _budget Budget of basket tokens willing to redeem
770 */
771function _redeemTNFT(
772    address _redeemer,
773    uint256 _budget
774) internal nonReentrant {
775    require(balanceOf(_redeemer) >= _budget, "Insufficient balance");
776    require(!seedRequestInFlight, "new seed is being generated");
777    require(nextToRedeem.tnft != address(0), "None redeemable");
778
779    address _tangibleNFT = nextToRedeem.tnft;
780    uint256 _tokenId = nextToRedeem.tokenId;
781    uint256 _usdValue = valueTracker[_tangibleNFT][_tokenId];
782    uint256 _sharesRequired = _quoteShares(_usdValue);
783
784    delete nextToRedeem;
785
786    require(tokenDeposited[_tangibleNFT][_tokenId], "Invalid token");
787    require(_budget >= _sharesRequired, "Insufficient budget");
788
789    // update contract
790    tokenDeposited[_tangibleNFT][_tokenId] = false;
791
792    uint256 index;
793    (index,) = _isDepositedTnft(_tangibleNFT, _tokenId);
794    depositedTnfts[index] = depositedTnfts[depositedTnfts.length - 1];
795    depositedTnfts.pop();
796
797    (index,) = _isTokenIdLibrary(_tangibleNFT, _tokenId);
798    tokenIdLibrary[_tangibleNFT][index] = tokenIdLibrary[_tangibleNFT][tokenIdLibrary[_tangibleNFT].length - 1];
799    tokenIdLibrary[_tangibleNFT].pop();
800
801    if (tokenIdLibrary[_tangibleNFT].length == 0) {
802        (index,) = _isSupportedTnft(_tangibleNFT); 
803        tnftsSupported[index] = tnftsSupported[tnftsSupported.length - 1];
804        tnftsSupported.pop();
805    }
806
807    IRentManager rentManager = _getRentManager(_tangibleNFT);
808
809    // redeem rent from redeemed TNFT to this contract.
810    if (rentManager.claimableRentForToken(_tokenId) > 0) {
811        uint256 preBal = primaryRentToken.balanceOf(address(this));
812        uint256 received = rentManager.claimRentForToken(_tokenId);
813        require(primaryRentToken.balanceOf(address(this)) == (preBal + received), "claiming error");
814    }
815
816    // unregister from price notifications
817    IRWAPriceNotificationDispatcher notificationDispatcher = _getNotificationDispatcher(_tangibleNFT);
818    INotificationWhitelister(address(notificationDispatcher)).unregisterForNotification(_tokenId);
819
820    // Transfer tokenId to user
821    IERC721(_tangibleNFT).safeTransferFrom(address(this), _redeemer, _tokenId);
822
823    totalNftValue -= _usdValue;
824    _burn(_redeemer, _sharesRequired);
825
826    // fetch new seed
827    _sendRequestForSeed();
828
829    emit TNFTRedeemed(_redeemer, _tangibleNFT, _tokenId);
830}

Recommendation:

We advise the fee to be properly imposed as otherwise the redemption of a fee exposes a way to bypass the rent fee.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team evaluated this exhibit and clarified that the fee is meant to be applied during rebase operations. As such, the rent may be claimed in the Basket::_redeemTNFT flow but a fee will be applied on the next rebase to it.

Based on these clarifications, we consider this exhibit nullified as the code behaves as expected.

BTE-07M: Inexistent Association of Redeemed NFT

Type	Severity	Location
Logical Fault		Basket.sol:L280-L282, L821

Description:

The Basket::redeemTNFT function does not allow the caller to validate the NFT they are redeeming, leading to a potentially different outcome between a transaction's submission and a transaction's execution in the network.

Impact:

Although of low likelihood, it is possible for a redemption operation to result in a different NFT than expected to be withdrawn either nefariously, accidentally, or due to network congestion.

Example:

Copy
280function redeemTNFT(uint256 _budget) external {
281    _redeemTNFT(msg.sender, _budget);
282}

Recommendation:

We advise an additional argument to be introduced, ensuring that the nextToRedeem entry within the contract matches the NFT the caller wishes to redeem.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Basket::_redeemTNFT function and its external counterpart have been updated to accept a _token argument that represents the concatenated hash of the _tangibleNFT and _tokenId being redeemed, ensuring that the caller can validate which Tangible NFT they will receive.

BTE-08M: Inexistent Validation of Rent Fee

Type	Severity	Location
Input Sanitization		Basket.sol:L381-L383

Description:

The Basket::updateRentFee function permits any rent fee to be specified, potentially consuming all rent revenue as a fee as well as setting a value greater than 100% that would cause rebase operations to fail.

Impact:

A misconfiguration of the rent fee can cause a Denial-of-Service attack for rebase operations and its value can exceed what users reasonably expect of their baskets.

Example:

Copy
381function updateRentFee(uint16 _rentFee) external onlyFactoryOwner {
382    rentFee = _rentFee;
383}

Recommendation:

We advise the code to at minimum ensure the new _rentFee is less than 100% (100_00) and as a recommendation to ensure it is at most a sensible value defined by the Tangible team.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

A require check was introduced ensuring that the rentFee cannot exceed 50% of the total value, alleviating this exhibit in full.

BTE-09M: Potential Re-Roll of Redeemable NFT

Type	Severity	Location
Logical Fault		Basket.sol:L289-L304, L311-L313

Description:

The Basket::fulfillRandomSeed function contains no re-roll protection and the Basket::_sendRequestForSeed does not prevent a new request to be made whilst one is already in flight.

Impact:

In case of manual intervention due to misbehaviour of the Gelato VRF, should multiple requests be made and eventually fulfilled they will all be processed "re-rolling" the redeemed NFT incorrectly.

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}
305
306/**
307 * @notice This method allows the factory owner to manually send a request for entropy to vrf.
308 * @dev This method should only be used as a last resort if a vrf callback reverts or if there resides a stale redeemable.
309 * @return requestId -> request identifier created by vrf coordinator.
310 */
311function sendRequestForSeed() external onlyFactoryOwner returns (uint256 requestId) {
312    return _sendRequestForSeed();
313}
314
315/**
316 * @notice Internal method that handles making requests to vrf through the BasketsVrfConsumer contract.
317 * @return requestId -> request identifier created by vrf coordinator.
318 */
319function _sendRequestForSeed() internal returns (uint256 requestId) {
320    if (depositedTnfts.length != 0) {
321        seedRequestInFlight = true;
322
323        requestId = IBasketsVrfConsumer(_getBasketVrfConsumer()).makeRequestForRandomWords();
324
325        pendingSeedRequestId = requestId;
326        emit RequestSentToVrf(requestId);
327    }
328}

Recommendation:

While we consider it correct for randomness to be re-requested via the administrative Basket::sendRequestForSeed function in case of failure of the VRF provider, we do not consider it correct for multiple randomness requests that are eventually fulfilled to be processed while a nextToRedeem entry already exists.

We advise the Basket::fulfillRandomSeed function to simply return early if a valid nextToRedeem entry exists and has not been "claimed" yet.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Basket::fulfillRandomSeed function was updated per our recommendation, ending execution early if a valid nextToRedeem entry exists and thus preventing the misbehaviour described by this exhibit from manifesting.

BTE-10M: Inexistent Synchronization of Rebase State

Type	Severity	Location
Logical Fault		Basket.sol:L553-L578, L672, L771-L774

Description:

The Basket contract represents a rebasing EIP-20 asset, however, the way the EIP-20 asset is minted and burned does not mandate a rebase synchronization beforehand leading to potential arbitrage opportunities that can be harmfully exploited.

Impact:

At minimum, we have demonstrated that a medium-severity impact is present via a re-entrancy. This particular discrepancy between the rebase state and the deposit / redemption of assets can lead to issues in the overall accounting of the contract and we strongly advise the Tangible team to treat this as a significant vulnerability.

Example:

Copy
553/**
554 * @notice This function allows for the Basket token to "rebase" and will update the multiplier based
555 * on the amount of rent accrued by the basket tokens.
556 */
557function rebase() public {
558    uint256 previousRentalIncome = totalRentValue;
559    uint256 totalRentalIncome = _getRentBal();
560
561    uint256 collectedRent = totalRentalIncome - previousRentalIncome;
562
563    // Take 10% off collectedRent and send to revenue contract
564    uint256 rentDistribution = (collectedRent * rentFee) / 100_00;
565    collectedRent -= rentDistribution;
566
567    uint256 rebaseIndexDelta = (collectedRent * decimalsDiff()) * 1e18 / getTotalValueOfBasket();
568
569    uint256 rebaseIndex = rebaseIndex();
570
571    rebaseIndex += rebaseIndexDelta;
572    totalRentValue = totalRentalIncome;
573
574    _transferRent(_getRevenueDistributor(), rentDistribution);
575    _setRebaseIndex(rebaseIndex);
576
577    emit RebaseExecuted(msg.sender, totalRentValue, rebaseIndex);
578}

Recommendation:

We advise the deposit and redemption flows for Tangible NFTs to ensure that the state has been rebased, preventing such arbitrage opportunities from being present.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team closely evaluated the ramifications of this exhibit and has opted to acknowledge the behaviour described. Specifically, they have introduced a re-entrancy guard as part of a separate exhibit's alleviation to address the medium-risk concern and have opted to acknowledge the arbitrage opportunities that may manifest.

Given that rent is vested, it is unrealistic for the rebase operations to occur in "real-time" and thus the Tangible team wishes to acknowledge the natural arbitrage opportunities that will present themselves.

BTE-11M: Nefarious Rebase Re-Entrancy Dilution

Type	Severity	Location
Logical Fault		Basket.sol:L567, L821, L823

Description:

The Basket::rebase function does not protect against re-entrancies and relies on the Basket::getTotalValueOfBasket function which in turn utilizes the totalNftValue data entry.

During the Basket::_redeemTNFT execution flow, the NFT will be transferred to the _redeemer in a re-entrant way before the totalNftValue is updated. This permits the _redeemer to re-enter the contract's Basket::rebase function and dilute the rebase improperly.

Impact:

It is presently possible to hijack Basket::rebase operations and forcefully dilute each share's proportionate increase by re-entering the contract.

Example:

Copy
766/**
767 * @notice Internal method for redeeming a specified TNFT from this basket contract.
768 * @param _redeemer EOA address of redeemer. note: msg.sender
769 * @param _budget Budget of basket tokens willing to redeem
770 */
771function _redeemTNFT(
772    address _redeemer,
773    uint256 _budget
774) internal nonReentrant {
775    require(balanceOf(_redeemer) >= _budget, "Insufficient balance");
776    require(!seedRequestInFlight, "new seed is being generated");
777    require(nextToRedeem.tnft != address(0), "None redeemable");
778
779    address _tangibleNFT = nextToRedeem.tnft;
780    uint256 _tokenId = nextToRedeem.tokenId;
781    uint256 _usdValue = valueTracker[_tangibleNFT][_tokenId];
782    uint256 _sharesRequired = _quoteShares(_usdValue);
783
784    delete nextToRedeem;
785
786    require(tokenDeposited[_tangibleNFT][_tokenId], "Invalid token");
787    require(_budget >= _sharesRequired, "Insufficient budget");
788
789    // update contract
790    tokenDeposited[_tangibleNFT][_tokenId] = false;
791
792    uint256 index;
793    (index,) = _isDepositedTnft(_tangibleNFT, _tokenId);
794    depositedTnfts[index] = depositedTnfts[depositedTnfts.length - 1];
795    depositedTnfts.pop();
796
797    (index,) = _isTokenIdLibrary(_tangibleNFT, _tokenId);
798    tokenIdLibrary[_tangibleNFT][index] = tokenIdLibrary[_tangibleNFT][tokenIdLibrary[_tangibleNFT].length - 1];
799    tokenIdLibrary[_tangibleNFT].pop();
800
801    if (tokenIdLibrary[_tangibleNFT].length == 0) {
802        (index,) = _isSupportedTnft(_tangibleNFT); 
803        tnftsSupported[index] = tnftsSupported[tnftsSupported.length - 1];
804        tnftsSupported.pop();
805    }
806
807    IRentManager rentManager = _getRentManager(_tangibleNFT);
808
809    // redeem rent from redeemed TNFT to this contract.
810    if (rentManager.claimableRentForToken(_tokenId) > 0) {
811        uint256 preBal = primaryRentToken.balanceOf(address(this));
812        uint256 received = rentManager.claimRentForToken(_tokenId);
813        require(primaryRentToken.balanceOf(address(this)) == (preBal + received), "claiming error");
814    }
815
816    // unregister from price notifications
817    IRWAPriceNotificationDispatcher notificationDispatcher = _getNotificationDispatcher(_tangibleNFT);
818    INotificationWhitelister(address(notificationDispatcher)).unregisterForNotification(_tokenId);
819
820    // Transfer tokenId to user
821    IERC721(_tangibleNFT).safeTransferFrom(address(this), _redeemer, _tokenId);
822
823    totalNftValue -= _usdValue;
824    _burn(_redeemer, _sharesRequired);
825
826    // fetch new seed
827    _sendRequestForSeed();
828
829    emit TNFTRedeemed(_redeemer, _tangibleNFT, _tokenId);
830}

Recommendation:

We advise the ReentrancyGuardUpgradeable::nonReentrant modifier to be introduced to the Basket::rebase function as well as the Basket::reinvestRent function which can also be influenced albeit with escalated privileges.

Additionally, we advise the Checks-Effects-Interactions pattern to be applied to the Basket::_redeemTNFT function by relocating the totalNftValue subtraction and Basket::_burn operation prior to the transfer of the NFT asset.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team proceeded with introducing the ReentrancyGuard::nonReentrant modifier to the Basket::rebase function and to apply the CEI pattern to the Basket::_redeemTNFT function updating the totalNftValue and performing the burn operation prior to the transfer of the NFT asset.

While the Basket::reinvestRent function was not protected, it represents a privileged function and the Tangible team has expressed valid scenarios in which they may wish to re-enter the contract as part of rent re-investments (i.e. by potentially increasing the portfolio of NFTs within the basket).

As such, we consider this exhibit alleviated based on the actions taken by the Tangible team as well as their honest operation of the Basket::reinvestRent function.

BTE-12M: Potentially Malicious Rent Token

Type	Severity	Location
Logical Fault		Basket.sol:L363-L365

Description:

The primaryRentToken of the Basket can solely be adjusted by a privileged member (FactoryModifiers::onlyFactoryOwner), however, its initialization is based on untrusted user input.

Impact:

The primaryRentToken of the Basket is highly sensitive, and a malicious token implementation could steal the rent of newly deposited NFTs to the basket in a worst case scenario.

Example:

Copy
357/**
358 * @notice This method allows the factory owner to update the `primaryRentToken` state variable.
359 * @dev If the rent token is being changed indefinitely, make sure to change the address of the rent token being used
360 *      to initialize new baskets on the BasketManager.
361 * @param _primaryRentToken New address for `primaryRentToken`.
362 */
363function updatePrimaryRentToken(address _primaryRentToken) external onlyFactoryOwner {
364    _updatePrimaryRentToken(_primaryRentToken);
365}

Recommendation:

Given its sensitive nature, we advise the _rentToken passed in to the Basket::initialize function to either be validated as part of the Tangible ecosystem or to be specified by the BasketManager contract instead of the BasketManager::deployBasket caller.

We consider either of the two proposed solutions as adequate in remediating this exhibit.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The BasketManager::deployBasket function was updated to omit its input _rentToken argument and instead use a contract-level administrator-defined primaryRentToken variable, addressing this exhibit in full.

BTE-13M: Incorrect Calculation of Rental Income

Type	Severity	Location
Logical Fault		Basket.sol:L559

Description:

The Basket::rebase function will incorrectly calculate rental income as it will not claim it when a rebase operation occurs.

Should the rentFee be sufficiently small that no claims need to be performed, multiple Basket::rebase operations can be performed to artificially inflate the evaluation of the Basket leading to significant attacks against depositors.

Impact:

The current rebasing system is incorrect as it will track the same pending-claim rent multiple times.

Example:

Copy
553/**
554 * @notice This function allows for the Basket token to "rebase" and will update the multiplier based
555 * on the amount of rent accrued by the basket tokens.
556 */
557function rebase() public {
558    uint256 previousRentalIncome = totalRentValue;
559    uint256 totalRentalIncome = _getRentBal();
560
561    uint256 collectedRent = totalRentalIncome - previousRentalIncome;
562
563    // Take 10% off collectedRent and send to revenue contract
564    uint256 rentDistribution = (collectedRent * rentFee) / 100_00;
565    collectedRent -= rentDistribution;
566
567    uint256 rebaseIndexDelta = (collectedRent * decimalsDiff()) * 1e18 / getTotalValueOfBasket();
568
569    uint256 rebaseIndex = rebaseIndex();
570
571    rebaseIndex += rebaseIndexDelta;
572    totalRentValue = totalRentalIncome;
573
574    _transferRent(_getRevenueDistributor(), rentDistribution);
575    _setRebaseIndex(rebaseIndex);
576
577    emit RebaseExecuted(msg.sender, totalRentValue, rebaseIndex);
578}

Recommendation:

We advise the code to either properly claim pending rent, or to track the "calculated" pending rent by the Basket::_getRentBal function invocation within Basket::rebase operations, ensuring that only newly accumulated rent is tracked in rebase operations.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

We revisited this exhibit after discussing with the Tangible team and validated that it is incorrect given that the totalRentValue will be overridden after the calculation and thus properly accounted for in consequent calls.

As such, we consider this exhibit nullified.

BTE-14M: Significantly Dangerous Untrusted Call

Type	Severity	Location
Logical Fault		Basket.sol:L487, L488

Description:

The Basket contract is capable of performing an arbitrary call to an arbitrary address which can be exploited to siphon funds out of the contract.

Impact:

As an example, a payload can be crafted that approves a rentBalance of 0 and performs a direct EIP-721 transfer of an asset in custody of the Basket. The operation would successfully execute, extracting the Tangible NFT from the contract incorrectly. Other such impersonation payloads can be crafted to cause less severe interactions, such as a request of randomness that is untracked by the Basket.

Example:

Copy
473/**
474 * @notice This method allows thew factory owner to reinvest accrued rent.
475 * @dev Ideally `target` would be a contract and `data` would be a method that allows us to purchase another
476 *      property and deposits it into the basket to yield more rent.
477 * @param target Address of contract with reinvest mechanism.
478 * @param rentBalance Amount of rent balance being allocated for reinvestment.
479 * @param data calldata payload for function call.
480 */
481function reinvestRent(address target, uint256 rentBalance, bytes calldata data) external {
482    require(canWithdraw[msg.sender], "Not authorized");
483
484    uint256 basketValueBefore = getTotalValueOfBasket();
485    primaryRentToken.approve(target, rentBalance);
486
487    (bool success,) = target.call(data);
488    require(success);
489    
490    totalRentValue -= rentBalance;
491    require(getTotalValueOfBasket() >= basketValueBefore);
492}

Recommendation:

At minimum, we advise the potential targets to be sanitized (f.e. whitelisted exchange addresses). Our formal recommendation is to adopt a security-first approach, utilizing a special-purpose "router" contract that will in turn perform the arbitrary calls. This will ensure that the Basket::reinvestRent function cannot be utilized to impersonate the Basket contract.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The targets at which arbitrary calls can be made via the Basket::reinvestRent function are now validated via a trustedTarget mapping that is configured by the factory's owner (i.e. the Tangible team).

As such, we consider this exhibit properly alleviated provided that the Tangible team correctly vets the entries they add to the trustedTarget data entry.