BasketManager Manual Review Findings

BMR-01M: Improper NFT Receipt Flow

Type	Severity	Location
Standard Conformity		BasketManager.sol:L231, L301-L306

Description:

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

Impact:

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

Example:

Copy
222// transfer initial TNFT from newBasketBeacon owner to this contract and approve transfer of TNFT to new basket
223for (uint256 i; i < _tokenIdDeposit.length;) {
224    IPriceOracle oracle = ITangiblePriceManager(address(IFactory(factory()).priceManager())).oracleForCategory(ITangibleNFT(_tangibleNFTDeposit[i]));
225    IRWAPriceNotificationDispatcher notificationDispatcher = IGetNotificationDispatcher(address(oracle)).notificationDispatcher();
226
227    if (!INotificationWhitelister(address(notificationDispatcher)).whitelistedReceiver(address(newBasketBeacon))) {
228        INotificationWhitelister(address(notificationDispatcher)).whitelistAddressAndReceiver(address(newBasketBeacon));
229    }
230
231    IERC721(_tangibleNFTDeposit[i]).safeTransferFrom(msg.sender, address(this), _tokenIdDeposit[i]);
232    IERC721(_tangibleNFTDeposit[i]).approve(address(newBasketBeacon), _tokenIdDeposit[i]);
233    unchecked {
234        ++i;
235    }
236}

Recommendation:

The BasketManager::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 BasketManager::onERC721Received function to be omitted and the BasketManager::deployBasket 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 the BasketManager 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.

BMR-02M: Improper Transfer of EIP-20 Asset

Type	Severity	Location
Standard Conformity		BasketManager.sol:L255

Description:

The BasketManager::withdrawERC20 function will mandate that the ERC20::transfer function invoked yields a bool variable which is not the case for a wide variety of tokens, inclusive of the widely adopted USDT token.

Impact:

The BasketManager::withdrawERC20 function implements a restrictive security check that would cause the USDT asset and other identical tokens to be irrecoverable from the BasketManager.

Example:

Copy
245/**
246 * @notice Withdraws any ERC20 token balance of this contract into the multisig wallet.
247 * @param _contract Address of an ERC20 compliant token.
248 */
249function withdrawERC20(address _contract) external onlyFactoryOwner {
250    require(_contract != address(0), "Address cannot be zero address");
251
252    uint256 balance = IERC20(_contract).balanceOf(address(this));
253    require(balance > 0, "Insufficient token balance");
254
255    require(IERC20(_contract).transfer(msg.sender, balance), "Transfer failed on ERC20 contract");
256}

Recommendation:

We advise the function to utilize the SafeERC20 library by OpenZeppelin or implement a similar flow, opportunistically evaluating the yielded bool as true solely if it exists.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The SafeERC20 library of the OpenZeppelin dependency is now properly imported to the codebase and its SafeERC20::safeTransfer function is correctly invoked in place of the potentially unhandled EIP-20 ERC20::transfer invocation, addressing this exhibit.

BMR-03M: Incorrect Hashing of Data

Type	Severity	Location
Logical Fault		BasketManager.sol:L214, L215

Description:

The BasketManager::deployBasket function will assign hashed values to the basketNames and basketSymbols mappings incorrectly, causing baskets to never expose their name and symbol properly.

Impact:

As the name and symbol of a basket is crucial in its dissemination across the community, we consider this to be a minor severity vulnerability.

Example:

Copy
214basketNames[address(newBasketBeacon)] = keccak256(abi.encodePacked(_name));
215basketSymbols[address(newBasketBeacon)] = keccak256(abi.encodePacked(_symbol));

Recommendation:

We advise the code to instead assign the _name and _symbol values directly by updating the basketNames and basketSymbols mappings to point to string entries.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The code was revised and introduced a new getBasketInfo data entry whereby the basketName and basketSymbol values are correctly stored.

As such, we consider this exhibit indirectly alleviated.

BMR-04M: Unscalable Iteration of Baskets

Type	Severity	Location
Language Specific		BasketManager.sol:L313-L328

Description:

The BasketManager::checkBasketAvailability function that is always invoked on a basket's deployment will iterate through all baskets to validate that the hashCache has not already been deployed.

Impact:

As a Denial-of-Service will eventually manifest due to the inefficient implementation, we consider this vulnerability to be of minor severity.

Example:

Copy
313/**
314 * @notice This method checks whether a basket with featuresHash can be created or is taken.
315 * @dev A featuresHash is a unique hash assigned each basket contract and is created based on the unique
316 *      combination of features that basket supports. No 2 baskets that support same combo can co-exist.
317 * @param _featuresHash unique bytes32 hash created from combination of features
318 * @return If true, features combo is available to be created. If false, already exists.
319 */
320function checkBasketAvailability(bytes32 _featuresHash) public view returns (bool) {
321    for (uint256 i; i < baskets.length;) {
322        if (hashedFeaturesForBasket[baskets[i]] == _featuresHash) return false;
323        unchecked {
324            ++i;
325        }
326    }
327    return true;
328}

Recommendation:

Given that the gas cost of the function increases unboundedly, the BasketManager::deployBasket function will eventually become inaccessible. As the hashCache is already utilized as a key to the fetchBasketByHash mapping, we advise the require check to validate that the fetchBasketByHash entry is zero effectively performing the same security check without iterating through all baskets.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The BasketManager::checkBasketAvailability function has been removed from the codebase and a fetchBasketByHash validation has been introduced in its place within the BasketManager::deployBasket function, alleviating this exhibit in full.

BMR-05M: Basket Deployment Race Condition

Type	Severity	Location
Logical Fault		BasketManager.sol:L180, L183, L337

Description:

The BasketManager::deployBasket function is susceptible to an on-chain race condition whereby a deployment can be front-runned by a transaction with the same _tnftType, _location, and _features configuration.

Impact:

We consider this to be a medium-severity vulnerability as the race condition can trivially manifest and the impact is that the user would have no alternative but to participate in the exploiter's basket which would incur a deposit fee unfairly thereby affecting user funds.

Example:

Copy
179// create unique hash for new basket
180hashCache = createHash(_tnftType, _location, _features);
181
182// might not be necessary -> hash is checked when Basket is initialized
183require(checkBasketAvailability(hashCache), "Basket already exists");

Recommendation:

We advise the approach to be revised, as a user can arbitrarily DoS the creation of a basket by another user by using the same configuration. As a viable alternative, we advise this limitation to be imposed per user rather than globally for all users as it is unfair for a user to be forced to participate in the basket of another and pay the deposit fees due to the aforementioned race condition.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team opted to instead apply the tax of a Basket regardless of whether the user is the first depositor or not, rendering a race-condition attack profit-less and thus addressing this exhibit indirectly.

BMR-06M: Bypass of Basket Uniqueness Invariant

Type	Severity	Location
Input Sanitization		BasketManager.sol:L180, L183

Description:

The BasketManager::deployBasket presently ensures that each basket is uniquely attached to a combination of a Tangible NFT type, an ISO location, and the features it supports.

This uniqueness security trait can be breached by introducing duplicate entries in the _features array, leading to the generation of a different hash but a functionally equivalent Basket.

Impact:

Presently, any Basket deployed with _features of a length less than the featureLimit can be re-deployed with the exact same functional configuration by duplicating any of its features in the _features array breaching a significant invariant of the BasketManager.

Example:

Copy
179// create unique hash for new basket
180hashCache = createHash(_tnftType, _location, _features);
181
182// might not be necessary -> hash is checked when Basket is initialized
183require(checkBasketAvailability(hashCache), "Basket already exists");

Recommendation:

We advise the code to prevent duplicate entries in the _features array, ensuring that the hash uniqueness trait is properly upheld by the contract.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The code was updated to ensure that the _features array contains no duplicates, preventing the misbehaviour described and thus alleviating this exhibit.