Omniscia Tangible Audit

BasketManager Code Style Findings

BasketManager Code Style Findings

BMR-01C: Generic Typographic Mistake

Description:

The referenced line contains a typographical mistake (i.e. private variable without an underscore prefix) or generic documentational error (i.e. copy-paste) that should be corrected.

Example:

src/BasketManager.sol
67bytes32 internal hashCache;

Recommendation:

We advise this to be done so to enhance the legibility of the codebase.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

An underscore prefix has been properly introduced to the referenced internal variable, addressing this exhibit.

BMR-02C: Inefficient Loop Limit Evaluation

Description:

The linked for loop evaluates its limit inefficiently on each iteration.

Example:

src/BasketManager.sol
321for (uint256 i; i < baskets.length;) {

Recommendation:

We advise the statements within the for loop limit to be relocated outside to a local variable declaration that is consequently utilized for the evaluation to significantly reduce the codebase's gas cost. We should note the same optimization is applicable for storage reads present in such limits as they are newly read on each iteration (i.e. length members of arrays in storage).

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The referenced function is no longer present in the codebase rendering the exhibit no longer applicable.

BMR-03C: Inefficient On-Chain Sorting of Features

Description:

The BasketManager::createHash function will sort the _features array on-chain which incurs a significant gas cost for the caller.

Example:

src/BasketManager.sol
330/**
331 * @notice This method is used to create a unique hash given a category and list of sub categories.
332 * @param _tnftType Category identifier.
333 * @param _location ISO Code for country.
334 * @param _features List of subcategories.
335 */
336function createHash(uint256 _tnftType, uint16 _location, uint256[] memory _features) public pure returns (bytes32 hashedFeatures) {
337    hashedFeatures = keccak256(abi.encodePacked(_tnftType, _location, _features.sort()));
338}

Recommendation:

As is standard across the DeFi space, we advise the code to instead expect the _features to already be sorted and to validate their sort order which is gas optimal and would not perform any mutations on the array.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The code was updated with a BasketManager::_verifySortedNoDuplicates function that will ensure the input features of a Basket creation call are correctly sorted in a strictly-ascending manner. As such, we consider this exhibit properly addressed and the code's efficiency significantly improved.

BMR-04C: Multiple Mappings w/ Shared Key

Description:

The hashedFeaturesForBasket, isBasket, basketNames, and basketSymbols mappings all utilize the same key and are assigned to at the same time.

Example:

src/BasketManager.sol
211hashedFeaturesForBasket[address(newBasketBeacon)] = hashCache;
212isBasket[address(newBasketBeacon)] = true;
213

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

Recommendation:

We advise a single mapping to be utilized that points to a struct entry with all relevant members, greatly optimizing the deployment gas cost of a basket as well as the gas cost involved in simultaneously accessing the referenced variables.

Alleviation (106fc61dcd):

The referenced data entries have been relocated to a single getBasketInfo data entry per our recommendation except for isBasket which was converted into a getter function.

To note, this function is technically incorrect as a hash of zero is still valid but we consider this inconsequential. As a final note, the syntax employed is slightly non-standard and a direct return of the condition itself is advised.

Alleviation (d7d2436ede):

The Tangible team revisited this exhibit and re-introduced the isBasket flag to the BasketInfo structure yielding its value in the new BasketManager::isBasket function.

As this led to a change of the syntax in the function as well, the exhibit's follow-up recommendations have been applied in full.

BMR-05C: Repetitive Invocations of Getter Functions

Description:

The FactoryModifiers::factory and IFactory::tnftMetadata functions are repetitively invoked multiple times within the same BasketManager::deployBasket function.

Example:

src/BasketManager.sol
150function deployBasket(
151    string memory _name,
152    string memory _symbol,
153    uint256 _tnftType,
154    address _rentToken,
155    uint16 _location,
156    uint256[] memory _features,
157    address[] memory _tangibleNFTDeposit,
158    uint256[] memory _tokenIdDeposit
159) external returns (IBasket, uint256[] memory basketShares) {
160    // verify _tanfibleNFTDeposit array and _tokenIdDeposit array are the same size.
161    require(_tangibleNFTDeposit.length == _tokenIdDeposit.length, "Differing lengths");
162

163    // verify deployer is depositing an initial token into basket.
164    require(_tangibleNFTDeposit.length !=0, "Must be an initial deposit");
165

166    // verify _features does not have more features that what is allowed.
167    require(featureLimit >= _features.length, "Too many features");
168

169    // verify _tnftType is a supported type in the Metadata contract.
170    (bool added,,) = ITNFTMetadata(IFactory(factory()).tnftMetadata()).tnftTypes(_tnftType);
171    require(added, "Invalid tnftType");
172

173    // verify _name is unique and available
174    require(!nameHashTaken[keccak256(abi.encodePacked(_name))], "Name not available");
175

176    // verify _symbol is unique and available
177    require(!symbolHashTaken[keccak256(abi.encodePacked(_symbol))], "Symbol not available");
178

179    // create unique hash for new basket
180    hashCache = createHash(_tnftType, _location, _features);
181

182    // might not be necessary -> hash is checked when Basket is initialized
183    require(checkBasketAvailability(hashCache), "Basket already exists");
184

185    // check features are valid.
186    for (uint256 i; i < _features.length;) {
187        require(ITNFTMetadata(IFactory(factory()).tnftMetadata()).featureInType(_tnftType, _features[i]), "Feature not supported in type");
188        unchecked {
189            ++i;
190        }
191    }
192

193    // create new basket beacon proxy
194    BasketBeaconProxy newBasketBeacon = new BasketBeaconProxy(
195        address(beacon),
196        abi.encodeWithSelector(Basket.initialize.selector,
197            _name,
198            _symbol,
199            factory(),
200            _tnftType,
201            _rentToken,
202            _features,
203            _location,
204            msg.sender
205        )
206    );
207

208    // store hash and new newBasketBeacon
209    baskets.push(address(newBasketBeacon));
210

211    hashedFeaturesForBasket[address(newBasketBeacon)] = hashCache;
212    isBasket[address(newBasketBeacon)] = true;
213

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

217    nameHashTaken[keccak256(abi.encodePacked(_name))] = true;
218    symbolHashTaken[keccak256(abi.encodePacked(_symbol))] = true;
219

220    fetchBasketByHash[hashCache] = address(newBasketBeacon);
221

222    // transfer initial TNFT from newBasketBeacon owner to this contract and approve transfer of TNFT to new basket
223    for (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    }
237

238    // call batchDepositTNFT
239    basketShares = IBasket(address(newBasketBeacon)).batchDepositTNFT(_tangibleNFTDeposit, _tokenIdDeposit);
240

241    emit BasketCreated(msg.sender, address(newBasketBeacon));
242    return (IBasket(address(newBasketBeacon)), basketShares);
243}

Recommendation:

We advise them to be invoked once and their results to be stored to local variables that are consequently utilized in place of the independent function invocations.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The Tangible team proceeded to acknowledge this exhibit due to the fact that the code block has reached its stack depth limit. As such, we will consider it nullified as it is inapplicable to the code as-is.

BMR-06C: Sub-Optimal Availability Mappings

Description:

The BasketManager::deployBasket function will validate that a particular _name or _symbol has not been already reserved by encoding the string payload tightly, performing a keccak256 operation on the resulting payload, and using the final value as a key to the nameHashTaken or symbolHashTaken mappings respectively.

This approach is inefficient, as a mapping will perform a keccak256 operation on its key regardless, meaning that two keccak256 operations are performed per access of the mapping instead of one.

Example:

src/BasketManager.sol
173// verify _name is unique and available
174require(!nameHashTaken[keccak256(abi.encodePacked(_name))], "Name not available");
175

176// verify _symbol is unique and available
177require(!symbolHashTaken[keccak256(abi.encodePacked(_symbol))], "Symbol not available");

Recommendation:

We advise the mapping declarations to be updated to use a string value as a key, ensuring a single keccak256 operation is performed and permitting the function to pass in the _name and _symbol values as keys directly with no pre-processing.

Alleviation (106fc61dcd38fe29a81d1984ccde9171f5f231af):

The referenced mapping declarations were updated to accept a string input for their key directly, addressing this exhibit as advised.

Prev

Basket.sol (BTE-C)

Next

BasketsVrfConsumer.sol (BVC-C)

On this page