Nested Finance contest - Dravee's results

QA Report

Table of Contents:

• QA Report
  • Foreword
  • Comparisons
    • Comparison that should be inclusive in NestedRecords.sol
  • Variables
    • Missing Address(0) checks
    • Check if a value is in an array before a push
    • Variables that should be grouped together in a struct
      • File: FeeSplitter.sol
  • Arithmetics
    • Possible division by 0
  • Revert Strings
    • File: NestedFactory.sol
      • Inconsistent Revert string
    • File: MixinOperatorResolver.sol
      • Inconsistent Revert string (1)
      • Misleading + Inconsistent Revert string (2)
    • File: OwnableProxyDelegation.sol
      • Inconsistent Revert string (1)
      • Inconsistent Revert string (2)
    • File: ZeroExOperator.sol
      • Inconsistent Revert string (1)
      • Inconsistent Revert string (2)
  • Comments
    • File: NestedFactory.sol
      • Missing comment "@return" (1)
      • Missing comment "@return" (2)
      • Missing comment "@param" (1)
    • File: NestedRecords.sol
      • Missing comment "@return" (1)
      • Missing comment "@return" (2)
      • Misleading comment on "@return"
    • File: MixinOperatorResolver.sol
      • Missing 2 comments "@param" & changeable "@return" comment/variable
    • File: ExchangeHelpers.sol
      • Missing comment "@return"

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Comparisons

Comparison that should be inclusive in NestedRecords.sol

File: NestedRecords.sol
123:         require(records[_nftId].tokens.length < maxHoldingsCount, "NRC: TOO_MANY_TOKENS"); //@audit should be inclusive

As length isn't 0 indexed, I believe, as an example to illustrate, that if maxHoldingsCount == 1, then records[_nftId].tokens.length == 1 should be a passing condition. Therefore, I suggest changing < with <=

Variables

Missing Address(0) checks

File: MixinOperatorResolver.sol
22:     constructor(address _resolver) {
23:         resolver = OperatorResolver(_resolver); //@audit missing address(0) check on immutable just like in the constructors in FeeSplitter.sol and NestedFactory.sol
24:     }

Check if a value is in an array before a push

In NestedRecords.sol's store function, it's possible to push an existing address _token several times in the same array

File: NestedRecords.sol
130:         records[_nftId].tokens.push(_token); //@audit : should check existence

The previous lines of codes don't prevent this. The store function has the modifier onlyFactory and the only impact seem to be a possible maximization of records[_nftId].tokens.length (so that it reaches maxHoldingsCount).

Variables that should be grouped together in a struct

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).
By regrouping, it's then possible to delete all related fields with a simple delete newStruct[previousSameKeyForAllPreviousMaps].

File: FeeSplitter.sol

2 maps can be grouped together, as they use the same _account key:

62:     struct TokenRecords {
63:         uint256 totalShares;
64:         uint256 totalReleased;
65:         mapping(address => uint256) shares; //@audit group 
66:         mapping(address => uint256) released; //@audit group 
67:     }

I'd suggest these 2 related data get grouped in a struct, let's name it AccountInfo:

struct AccountInfo {   
    uint256 shares;   
    uint256 released;   
}   

And it would be used in this manner (where address is _account):

    struct TokenRecords {
        uint256 totalShares;
        uint256 totalReleased;
        mapping(address => AccountInfo) accountInfo;
    }   

Arithmetics

Possible division by 0

There are no checks that the denominator is != 0 here:

File: FeeSplitter.sol
327:     function _computeShareCount(
328:         uint256 _amount,
329:         uint256 _weight,
330:         uint256 _totalWeights
331:     ) private pure returns (uint256) {
332:         return (_amount * _weight) / _totalWeights; // @audit _totalWeights can be equal to 0, see FeeSplitter.sol:L184
333:     }

Revert Strings

File: NestedFactory.sol

Inconsistent Revert string

44:         require(_exists(_tokenId), "URI query for nonexistent token");

All other revert strings in NestedAsset.sol begin with NA: . Only this one doesn't. It's possible to gain consistency and still have an < 32 bytes size string with the following: "NA: URI query - inexistent token"

File: MixinOperatorResolver.sol

Inconsistent Revert string (1)

100:             require(tokens[0] == _outputToken, "OH: INVALID_OUTPUT_TOKEN");//@audit LOW comment : MOR like above

Here, "OH: INVALID_OUTPUT_TOKEN" should be replaced with "MOR: INVALID_OUTPUT_TOKEN"

Misleading + Inconsistent Revert string (2)

101:             require(tokens[1] == _inputToken, "OH: INVALID_OUTPUT_TOKEN"); //@audit LOW comment : INVALID_INPUT_TOKEN //@audit LOW comment : MOR

Here, "OH: INVALID_OUTPUT_TOKEN" should be replaced with "MOR: INVALID_INPUT_TOKEN"

File: OwnableProxyDelegation.sol

Inconsistent Revert string (1)

25:         require(!initialized, "OFP: INITIALIZED"); //@audit low OFP doesn't make sense, use OPD instead (example: OwnableFactoryHandler is OFH, MixinOperatorResolver is MOR)

Is most contracts, the capital letters from the contract's name are used as a prefix in the revert strings (OwnableFactoryHandler has OFH, MixinOperatorResolver has MOR). Here, OFP doesn't really reflect OwnableProxyDelegation. It should be OPD.

Inconsistent Revert string (2)

26:         require(StorageSlot.getAddressSlot(_ADMIN_SLOT).value == msg.sender, "OFP: FORBIDDEN");//@audit should be "OPD: FORBIDDEN" 

Same as above: OFP should be OPD.

File: ZeroExOperator.sol

Inconsistent Revert string (1)

32:         require(success, "ZEO: SWAP_FAILED");
...
36:         require(amountBought != 0, "ZeroExOperator::performSwap: amountBought cant be zero"); //@audit LOW do like line 32 : "ZEO: amountBought cant be zero" < 32 bytes & consistent

As said before, the capital letters from the contract's name are used as a prefix in the revert strings. Here, the revert string's size is > 32 bytes and isn't using the same style as 4 lines above it. ZeroExOperator::performSwap should be ZEO.

Inconsistent Revert string (2)

32:         require(success, "ZEO: SWAP_FAILED");
...
37:         require(amountSold != 0, "ZeroExOperator::performSwap: amountSold cant be zero");//@audit do like line 32 : "ZEO: amountSold cant be zero" < 32 bytes & consistent

Same as above: ZeroExOperator::performSwap should be ZEO.

Comments

File: NestedFactory.sol

Missing comment "@return" (1)

403:     /// @dev Call the operator to submit the order and add the output
404:     /// assets to the reserve (if needed).
405:     /// @param _inputToken Token used to make the orders
406:     /// @param _outputToken Expected output token
407:     /// @param _nftId The nftId
408:     /// @param _order The order calldata
409:     /// @param _toReserve True if the output is store in the reserve/records, false if not. //@audit missing @return 
410:     function _submitOrder(
411:         address _inputToken,
412:         address _outputToken,
413:         uint256 _nftId,
414:         Order calldata _order,
415:         bool _toReserve
416:     ) private returns (uint256 amountSpent) {

Missing comment "@return" (2)

474:     /// @dev Choose between ERC20 (safeTransfer) and ETH (deposit), to transfer from the Reserve
475:     ///      or the user wallet, to the factory.
476:     /// @param _nftId The NFT id
477:     /// @param _inputToken The token to receive
478:     /// @param _inputTokenAmount Amount to transfer
479:     /// @param _fromReserve True to transfer from the reserve
480:     /// @return Token transfered (in case of ETH)
481:     ///         The real amount received after the transfer to the factory //@audit missing @return (not the description, just the keyword)
482:     function _transferInputTokens(
483:         uint256 _nftId,
484:         IERC20 _inputToken,
485:         uint256 _inputTokenAmount,
486:         bool _fromReserve
487:     ) private returns (IERC20, uint256) {

Missing comment "@param" (1)

562:     /// @dev Transfer from factory and collect fees
563:     /// @param _token The token to transfer
564:     /// @param _amount The amount (with fees) to transfer
565:     /// @param _dest The address receiving the funds //@audit missing @param
566:     function _safeTransferWithFees(
567:         IERC20 _token,
568:         uint256 _amount,
569:         address _dest,
570:         uint256 _nftId
571:     ) private {

File: NestedRecords.sol

Missing comment "@return" (1)

162:     /// @param _nftId The id of the NFT> //@audit missing @return
163:     function getAssetTokens(uint256 _nftId) public view returns (address[] memory) {

Missing comment "@return" (2)

183:     /// @param _token The address of the token //@audit missing @return
184:     function getAssetHolding(uint256 _nftId, address _token) public view returns (uint256) {

Misleading comment on "@return"

Here, the comment @return The holdings, which is the unique @return comment, suggests a returned mapping(address => uint256) holdings as seen on struct NftRecord. However, the function is actually returning a uint256[] and an address[]. Therefore, two @return are required and the previous one should be deleted.

Code:

188:     /// @notice Returns the holdings associated to a NestedAsset
189:     /// @param _nftId the id of the NestedAsset
190:     /// @return The holdings //@audit "The holdings" suggests a "mapping(address => uint256)" but a uint256[] and an address[] are returned.  
191:     function tokenHoldings(uint256 _nftId) public view returns (address[] memory, uint256[] memory) {

File: MixinOperatorResolver.sol

Missing 2 comments "@param" & changeable "@return" comment/variable

    /// @dev Build the calldata (with safe datas) and call the Operator
    /// @param _order The order to execute //@audit missing @param _inputToken and @param _outputToken
    /// @return success If the operator call is successful
    /// @return amounts The amounts from the execution (used and received) //@audit why not use uint256[2]?
    ///         - amounts[0] : The amount of output token
    ///         - amounts[1] : The amount of input token USED by the operator (can be different than expected)
    function callOperator(
        INestedFactory.Order calldata _order,
        address _inputToken,
        address _outputToken
    ) internal returns (bool success, uint256[] memory amounts) {

I suggest changing the returned uint256[] to uint256[2]

File: ExchangeHelpers.sol

Missing comment "@return"

10:     /// @dev Perform a swap between two tokens
11:     /// @param _sellToken Token to exchange
12:     /// @param _swapTarget The address of the contract that swaps tokens
13:     /// @param _swapCallData Call data provided by 0x to fill the quote //@audit missing @return
14:     function fillQuote(
15:         IERC20 _sellToken,
16:         address _swapTarget,
17:         bytes memory _swapCallData
18:     ) internal returns (bool) {

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • File: NestedRecords.sol
    • function store()
      • Cache records[_nftId].reserve
      • Inclusive comparison
    • function deleteAsset()
      • Unchecked block
  • File: NestedFactory.sol
    • function removeOperator()
      • Unchecked block
    • function destroy()
      • Unchecked block (1)
      • Unchecked block (2)
    • function _submitInOrders()
      • Unchecked block (1)
      • Unchecked block (2)
    • function _submitOutOrders()
      • Unchecked block (1)
      • Unchecked block (2)
      • Unchecked block (3)
      • Unchecked block (4)
    • function _safeSubmitOrder()
      • Unchecked block
    • function _transferToReserveAndStore()
      • Unchecked block
    • function _transferInputTokens()
      • Unchecked block
    • function _safeTransferWithFees()
      • Unchecked block
  • File: FeeSplitter.sol
    • function updateShareholder()
      • Cache totalWeights
    • function sendFees()
      • Unchecked block
    • function sendFeesWithRoyalties()
      • Unchecked block
      • Cache totalWeights
    • function getAmountDue()
      • Cache _tokenRecords.totalShares
    • function _addShareholder()
      • A private function used only once can get inlined
  • File: ZeroExOperator.sol
    • function performSwap()
      • Unchecked block (1)
      • Unchecked block (2)
  • File: INestedFactory.sol
    • Storage
      • Tightly pack struct BatchedInputOrders
      • Tightly pack struct BatchedOutputOrders
      • Only use 1 struct
  • General recommendations
    • Variables
      • No need to explicitly initialize variables with default values
    • Comparisons
    • Amounts should be checked for 0 before calling a transfer
    • For-Loops
      • ++i costs less gas compared to i++
      • Increments can be unchecked
      • An array's length should be cached to save gas in for-loops
    • Errors
      • Reduce the size of error messages (Long revert Strings)
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• Storage-reading optimizations

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). In the paragraphs below, please see the @audit-issue tags in the pieces of code's comments for more information about SLOADs that could be saved by caching the mentioned storage variables in memory variables.

• Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

File: NestedRecords.sol

function store()

111:     function store(
...
118:         if (amount != 0) {
119:             require(records[_nftId].reserve == _reserve, "NRC: RESERVE_MISMATCH"); //@audit records[_nftId].reserve SLOAD 1 
120:             updateHoldingAmount(_nftId, _token, amount + _amount);
121:             return;
122:         }
123:         require(records[_nftId].tokens.length < maxHoldingsCount, "NRC: TOO_MANY_TOKENS"); //@audit should be inclusive
124:         require(
125:             _reserve != address(0) && (_reserve == records[_nftId].reserve || records[_nftId].reserve == address(0)), //@audit records[_nftId].reserve SLOAD 1 & 2
126:             "NRC: INVALID_RESERVE"
127:         );
...

Cache records[_nftId].reserve

Caching this in memory can save around 1 SLOAD

Inclusive comparison

By definition, maxHoldingsCount is the The maximum number of holdings for an NFT record. Here, as an example, if maxHoldingsCount == 1 and records[_nftId].tokens.length == 1, the function will revert. I believe this check should be inclusive (like this records[_nftId].tokens.length <= maxHoldingsCount). This is both a Low-risk issue and a gas issue as < costs 3 more gas than <= due to the additional ISZERO opcode (even with the Optimizer)

function deleteAsset()

88:     function deleteAsset(uint256 _nftId, uint256 _tokenIndex) public onlyFactory {
89:         address[] storage tokens = records[_nftId].tokens;
90:         address token = tokens[_tokenIndex];
91: 
92:         require(records[_nftId].holdings[token] != 0, "NRC: HOLDING_INACTIVE");
93: 
94:         delete records[_nftId].holdings[token];
95:         tokens[_tokenIndex] = tokens[tokens.length - 1]; //@audit gas: can't underflow
96:         tokens.pop();
97:     }

Unchecked block

If tokens.length == 1, all assets would be deleted. If tokens.length == 0, line 90 would've thrown an error and trigger a revert. As it's impossible for line 95 to underflow, it should be wrapped inside an unchecked block.

File: NestedFactory.sol

function removeOperator()

111:     function removeOperator(bytes32 operator) external override onlyOwner {
112:         uint256 operatorsLength = operators.length;
113:         for (uint256 i = 0; i < operatorsLength; i++) {
114:             if (operators[i] == operator) {
115:                 operators[i] = operators[operatorsLength - 1];  //@audit can't underflow
...

Unchecked block

Line 115 can't underflow due to operatorsLength > 0 (the for-loop wouldn't iterate otherwise). Therefore, line 115 should be wrapped inside an unchecked block.

function destroy()

200:     function destroy(
...
211:         uint256 buyTokenInitialBalance = _buyToken.balanceOf(address(this));
212: 
213:         for (uint256 i = 0; i < tokensLength; i++) {
214:             uint256 amount = nestedRecords.getAssetHolding(_nftId, tokens[i]);
215:             reserve.withdraw(IERC20(tokens[i]), amount);
216: 
217:             _safeSubmitOrder(tokens[i], address(_buyToken), amount, _nftId, _orders[i]);
218:             nestedRecords.freeHolding(_nftId, tokens[i]);
219:         }
220: 
221:         // Amount calculation to send fees and tokens
222:         uint256 amountBought = _buyToken.balanceOf(address(this)) - buyTokenInitialBalance;//@audit can't underflow 
223:         uint256 amountFees = amountBought / 100; // 1% Fee
224:         amountBought -= amountFees; //@audit can't underflow (equivalent to "amountBought = amountBought - (amountBought / 100)")
...

Unchecked block (1)

As buyTokenInitialBalance is <= to the final _buyToken.balanceOf(address(this)), line 222 can't underflow. Therefore, line 222 should be wrapped inside an unchecked block.

Unchecked block (2)

As amountBought -= amountFees is equivalent to amountBought = amountBought - (amountBought / 100), the result can't underflow. Therefore, line 223 should be wrapped inside an unchecked block.

function _submitInOrders()

311:     function _submitInOrders(
...
337:         require(amountSpent <= _inputTokenAmount - feesAmount, "NF: OVERSPENT");
338: 
339:         uint256 underSpentAmount = _inputTokenAmount - feesAmount - amountSpent; //@audit can't underflow
340:         if (underSpentAmount != 0) {
341:             tokenSold.safeTransfer(_fromReserve ? address(reserve) : _msgSender(), underSpentAmount);
342:         }
343: 
344:         // If input is from the reserve, update the records
345:         if (_fromReserve) {
346:             _decreaseHoldingAmount(_nftId, address(tokenSold), _inputTokenAmount - underSpentAmount); //@audit can't underflow
347:         }
...

Unchecked block (1)

Line 339 can't underflow due to the require statement line 337. Therefore, line 339 should be wrapped inside an unchecked block.

Unchecked block (2)

As underSpentAmount = _inputTokenAmount - feesAmount - amountSpent (line 339): _inputTokenAmount >= underSpentAmount. Therefore, line 346 can't underflow and should be wrapped inside an unchecked block.

function _submitOutOrders()

357:     function _submitOutOrders(
...
365:         amountBought = _batchedOrders.outputToken.balanceOf(address(this));
...
385:             require(amountSpent <= _inputTokenAmount, "NF: OVERSPENT");
386: 
387:             uint256 underSpentAmount = _inputTokenAmount - amountSpent; //@audit can't underflow
388:             if (underSpentAmount != 0) {
389:                 _inputToken.safeTransfer(address(reserve), underSpentAmount);
390:             }
391: 
392:             _decreaseHoldingAmount(_nftId, address(_inputToken), _inputTokenAmount - underSpentAmount); //@audit can't underflow
393:         }
394: 
395:         amountBought = _batchedOrders.outputToken.balanceOf(address(this)) - amountBought; //@audit can't underflow
396:         feesAmount = amountBought / 100; // 1% Fee //@audit HIGH free stuff under 100 ? Check on Remix. That's one of Secureum's audit findings
397: 
398:         if (_toReserve) {
399:             _transferToReserveAndStore(_batchedOrders.outputToken, amountBought - feesAmount, _nftId);//@audit can't underflow
400:         }
401:     }

Unchecked block (1)

Line 387 can't underflow due to the require statement line 385. Therefore, line 387 should be wrapped inside an unchecked block.

Unchecked block (2)

As underSpentAmount = _inputTokenAmount - amountSpent: _inputTokenAmount >= underSpentAmount. Therefore, line 392 can't underflow and should be wrapped inside an unchecked block.

Unchecked block (3)

As the initial _batchedOrders.outputToken.balanceOf(address(this)) line 365 is <= to the final _batchedOrders.outputToken.balanceOf(address(this)) line 395: line 395 can't underflow. Therefore, line 395 should be wrapped inside an unchecked block.

Unchecked block (4)

As amountBought - feesAmount is equivalent to amountBought - (amountBought / 100), the result can't underflow. Therefore, line 399 should be wrapped inside an unchecked block.

function _safeSubmitOrder()

435:     function _safeSubmitOrder(
...
445:             if (_amountToSpend > amounts[1]) {
446:                 IERC20(_inputToken).safeTransfer(_msgSender(), _amountToSpend - amounts[1]); //@audit should be unchecked (see L445)
447:             }
...

Unchecked block

Line 446 can't underflow due to the require statement line 445. Therefore, line 446 should be wrapped inside an unchecked block.

function _transferToReserveAndStore()

458:     function _transferToReserveAndStore(
459:         IERC20 _token,
460:         uint256 _amount,
461:         uint256 _nftId
462:     ) private {
463:         address reserveAddr = address(reserve);
464:         uint256 balanceReserveBefore = _token.balanceOf(reserveAddr);
465: 
466:         // Send output to reserve
467:         _token.safeTransfer(reserveAddr, _amount);
468: 
469:         uint256 balanceReserveAfter = _token.balanceOf(reserveAddr);
470: 
471:         nestedRecords.store(_nftId, address(_token), balanceReserveAfter - balanceReserveBefore, reserveAddr);//@audit can't underflow
472:     }

Unchecked block

As the initial _token.balanceOf(reserveAddr) is <= to the final _token.balanceOf(reserveAddr): line 471 can't underflow. Therefore, line 471 should be wrapped inside an unchecked block.

function _transferInputTokens()

482:     function _transferInputTokens(
...
494:         uint256 balanceBefore = _inputToken.balanceOf(address(this));
495:         if (_fromReserve) {
496:             require(
497:                 nestedRecords.getAssetHolding(_nftId, address(_inputToken)) >= _inputTokenAmount,
498:                 "NF: INSUFFICIENT_AMOUNT_IN"
499:             );
500:             // Get input from reserve
501:             reserve.withdraw(IERC20(_inputToken), _inputTokenAmount);
502:         } else {
503:             _inputToken.safeTransferFrom(_msgSender(), address(this), _inputTokenAmount);
504:         }
505:         return (_inputToken, _inputToken.balanceOf(address(this)) - balanceBefore); //@audit can't underflow
506:     }

Unchecked block

As the initial _inputToken.balanceOf(address(this)) is <= to the final _inputToken.balanceOf(address(this)): line 505 can't underflow. Therefore, it should be wrapped inside an unchecked block.

function _safeTransferWithFees()

566:     function _safeTransferWithFees(
567:         IERC20 _token,
568:         uint256 _amount,
569:         address _dest,
570:         uint256 _nftId
571:     ) private {
572:         uint256 feeAmount = _amount / 100; // 1% Fee
573:         _transferFeeWithRoyalty(feeAmount, _token, _nftId);
574:         _token.safeTransfer(_dest, _amount - feeAmount);//@audit can't underflow
575:     }

Unchecked block

As _amount - feeAmount is equivalent to _amount - (_amount / 100), the result can't underflow. Therefore, line 574 should be wrapped inside an unchecked block.

File: FeeSplitter.sol

function updateShareholder()

134:     function updateShareholder(uint256 _accountIndex, uint96 _weight) external onlyOwner {
135:         require(_accountIndex < shareholders.length, "FS: INVALID_ACCOUNT_INDEX");
136:         totalWeights = totalWeights + _weight - shareholders[_accountIndex].weight; //@audit cache
137:         require(totalWeights != 0, "FS: TOTAL_WEIGHTS_ZERO");
138:         shareholders[_accountIndex].weight = _weight;
139:         emit ShareholderUpdated(shareholders[_accountIndex].account, _weight);
140:     }

Cache totalWeights

It's possible to save around 1 SLOAD by caching totalWeights in memory, like this:

134:     function updateShareholder(uint256 _accountIndex, uint96 _weight) external onlyOwner {
135:         require(_accountIndex < shareholders.length, "FS: INVALID_ACCOUNT_INDEX");
136:         uint256 _totalWeights = totalWeights + _weight - shareholders[_accountIndex].weight;  //@audit +MSTORE
137:         require(_totalWeights != 0, "FS: TOTAL_WEIGHTS_ZERO"); //@audit +MLOAD -SLOAD
138:         totalWeights = _totalWeights; //@audit +MLOAD
139:         shareholders[_accountIndex].weight = _weight;
140:         emit ShareholderUpdated(shareholders[_accountIndex].account, _weight);
141:     }

function sendFees()

175:     function sendFees(IERC20 _token, uint256 _amount) external nonReentrant {
176:         uint256 weights;
177:         unchecked {
178:             weights = totalWeights - royaltiesWeight;
179:         }
180: 
181:         uint256 balanceBeforeTransfer = _token.balanceOf(address(this));
182:         _token.safeTransferFrom(_msgSender(), address(this), _amount); 
183: 
184:         _sendFees(_token, _token.balanceOf(address(this)) - balanceBeforeTransfer, weights); //@audit can't underflow (see L181 and L182)
185:     }

Unchecked block

As the initial _token.balanceOf(address(this)) is <= to the final _token.balanceOf(address(this)): line 184 can't underflow. Therefore, it should be wrapped inside an unchecked block.

function sendFeesWithRoyalties()

191:     function sendFeesWithRoyalties(
...
198:         uint256 balanceBeforeTransfer = _token.balanceOf(address(this));
199:         _token.safeTransferFrom(_msgSender(), address(this), _amount);
200:         uint256 amountReceived = _token.balanceOf(address(this)) - balanceBeforeTransfer;  //@audit can't underflow
201: 
202:         uint256 royaltiesAmount = _computeShareCount(amountReceived, royaltiesWeight, totalWeights); //@audit totalWeights SLOAD 1
203: 
204:         _sendFees(_token, amountReceived, totalWeights);//@audit totalWeights SLOAD 2
...

Unchecked block

As the initial _token.balanceOf(address(this)) is <= to the final _token.balanceOf(address(this)): line 200 can't underflow. Therefore, it should be wrapped inside an unchecked block.

Cache totalWeights

Caching this in memory can save around 1 SLOAD

function getAmountDue()

216:     function getAmountDue(address _account, IERC20 _token) public view returns (uint256) {
217:         TokenRecords storage _tokenRecords = tokenRecords[address(_token)];
218:         if (_tokenRecords.totalShares == 0) return 0;//@audit _tokenRecords.totalShares SLOAD 1
219: 
220:         uint256 totalReceived = _tokenRecords.totalReleased + _token.balanceOf(address(this));
221:         return
222:             (totalReceived * _tokenRecords.shares[_account]) /
223:             _tokenRecords.totalShares - //@audit _tokenRecords.totalShares SLOAD 2
224:             _tokenRecords.released[_account];
225:     }

Cache _tokenRecords.totalShares

Caching this in memory can save around 1 SLOAD

function _addShareholder()

A private function used only once can get inlined

As this private function is only used once line 127 in function setShareholders(), it can get inlined to save some gas.

File: ZeroExOperator.sol

function performSwap()

21:     function performSwap(
...
28:         uint256 buyBalanceBeforePurchase = buyToken.balanceOf(address(this));
29:         uint256 sellBalanceBeforePurchase = sellToken.balanceOf(address(this));
30: 
31:         bool success = ExchangeHelpers.fillQuote(sellToken, operatorStorage.swapTarget(), swapCallData);
32:         require(success, "ZEO: SWAP_FAILED");
33: 
34:         uint256 amountBought = buyToken.balanceOf(address(this)) - buyBalanceBeforePurchase; //@audit can't underflow (see L28 and L31)
35:         uint256 amountSold = sellBalanceBeforePurchase - sellToken.balanceOf(address(this));//@audit can't underflow (see L29 and L31)
36:         require(amountBought != 0, "ZeroExOperator::performSwap: amountBought cant be zero"); //@audit-info move up 1 ? Will certainly cost more gas on happy path while saving some on sad path. Not a good trade-off
37:         require(amountSold != 0, "ZeroExOperator::performSwap: amountSold cant be zero");
...

Unchecked block (1)

As the initial buyToken.balanceOf(address(this)) is <= to the final buyToken.balanceOf(address(this)): line 34 can't underflow. Therefore, it should be wrapped inside an unchecked block.

Unchecked block (2)

As the initial sellToken.balanceOf(address(this)) is <= to the final sellToken.balanceOf(address(this)): line 35 can't underflow. Therefore, it should be wrapped inside an unchecked block.

File: INestedFactory.sol

Storage

Tightly pack struct BatchedInputOrders

struct BatchedInputOrders can be tightly packed to save 1 storage slot by changing the code from this:

    struct BatchedInputOrders {
        IERC20 inputToken;//@audit 20 byte
        uint256 amount; //@audit 32 byte
        Order[] orders; //@audit fully takes slots
        bool fromReserve; //@audit 1 byte
    }

to this:

    struct BatchedInputOrders {
        IERC20 inputToken;//@audit 20 byte
        bool fromReserve; //@audit 1 byte
        uint256 amount; //@audit 32 byte
        Order[] orders; //@audit fully takes slots
    }

Tightly pack struct BatchedOutputOrders

struct BatchedOutputOrders can be tightly packed to save 1 storage slot by changing the code from this:

    struct BatchedOutputOrders {
        IERC20 outputToken;//@audit 20 byte
        uint256[] amounts;//@audit 32 byte
        Order[] orders;//@audit fully takes slots
        bool toReserve;//@audit 1 byte
    }

to this:

    struct BatchedOutputOrders {
        IERC20 outputToken;//@audit 20 byte
        bool toReserve;//@audit 1 byte
        uint256[] amounts;//@audit 32 byte
        Order[] orders;//@audit fully takes slots
    }

Only use 1 struct

In my opinion, the structs here are used in an unintended way: it's not up to a struct to carry the input/output concept here.

It's possible to use only 1 struct for the whole logic, as such:

    struct BatchedOrders {
        IERC20 token;
        bool hasReserve;
        uint256[] amounts;
        Order[] orders;
    }

And then declare input and output variables with this, like: BatchedOrders[] _batchedInputOrders or BatchedOrders[] _batchedOutputOrders.

Same struct, different variables.

I suggest going from:

NestedFactory.sol:
  141:     function create(uint256 _originalTokenId, BatchedInputOrders[] calldata _batchedOrders)
  162:     function processInputOrders(uint256 _nftId, BatchedInputOrders[] calldata _batchedOrders)
  176:     function processOutputOrders(uint256 _nftId, BatchedOutputOrders[] calldata _batchedOrders)
  190:         BatchedInputOrders[] calldata _batchedInputOrders,
  191:         BatchedOutputOrders[] calldata _batchedOutputOrders
  193:         _checkMsgValue(_batchedInputOrders);
  194:         _processInputOrders(_nftId, _batchedInputOrders);
  195:         _processOutputOrders(_nftId, _batchedOutputOrders);
  268:     function _processInputOrders(uint256 _nftId, BatchedInputOrders[] calldata _batchedOrders) private {
  286:     function _processOutputOrders(uint256 _nftId, BatchedOutputOrders[] calldata _batchedOrders) private {
  313:         BatchedInputOrders calldata _batchedOrders,
  359:         BatchedOutputOrders calldata _batchedOrders,
  579:     function _checkMsgValue(BatchedInputOrders[] calldata _batchedOrders) private {

interfaces\INestedFactory.sol:
  106:     function create(uint256 _originalTokenId, BatchedInputOrders[] calldata _batchedOrders) external payable;
  111:     function processInputOrders(uint256 _nftId, BatchedInputOrders[] calldata _batchedOrders) external payable;
  116:     function processOutputOrders(uint256 _nftId, BatchedOutputOrders[] calldata _batchedOrders) external;
  120:     /// @param _batchedInputOrders The input orders to execute (first)
  121:     /// @param _batchedOutputOrders The output orders to execute (after)
  124:         BatchedInputOrders[] calldata _batchedInputOrders,
  125:         BatchedOutputOrders[] calldata _batchedOutputOrders

to

NestedFactory.sol:
  141:     function create(uint256 _originalTokenId, BatchedOrders[] calldata _batchedInputOrders)
  162:     function processInputOrders(uint256 _nftId, BatchedOrders[] calldata _batchedInputOrders)
  176:     function processOutputOrders(uint256 _nftId, BatchedOrders[] calldata _batchedOutputOrders)
  190:         BatchedOrders[] calldata _batchedInputOrders,
  191:         BatchedOrders[] calldata _batchedOutputOrders
  193:         _checkMsgValue(_batchedInputOrders);
  194:         _processInputOrders(_nftId, _batchedInputOrders);
  195:         _processOutputOrders(_nftId, _batchedOutputOrders);
  268:     function _processInputOrders(uint256 _nftId, BatchedOrders[] calldata _batchedInputOrders) private {
  286:     function _processOutputOrders(uint256 _nftId, BatchedOrders[] calldata _batchedOutputOrders) private {
  313:         BatchedOrders calldata _batchedInputOrders,
  359:         BatchedOrders calldata _batchedOutputOrders,
  579:     function _checkMsgValue(BatchedOrders[] calldata _batchedInputOrders) private {

interfaces\INestedFactory.sol:
  106:     function create(uint256 _originalTokenId, BatchedOrders[] calldata _batchedInputOrders) external payable;
  111:     function processInputOrders(uint256 _nftId, BatchedOrders[] calldata _batchedInputOrders) external payable;
  116:     function processOutputOrders(uint256 _nftId, BatchedOrders[] calldata _batchedOutputOrders) external;
  124:         BatchedOrders[] calldata _batchedInputOrders,
  125:         BatchedOrders[] calldata _batchedOutputOrders

General recommendations

Variables

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

abstracts\MixinOperatorResolver.sol:36:        for (uint256 i = 0; i < requiredOperators.length; i++) {
abstracts\MixinOperatorResolver.sol:55:        for (uint256 i = 0; i < requiredOperators.length; i++) {
FeeSplitter.sol:126:        for (uint256 i = 0; i < accountsLength; i++) {
FeeSplitter.sol:148:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:165:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:261:        for (uint256 i = 0; i < shareholders.length; i++) {
FeeSplitter.sol:280:        for (uint256 i = 0; i < shareholdersCache.length; i++) {
FeeSplitter.sol:318:        for (uint256 i = 0; i < shareholders.length; i++) {
NestedFactory.sol:103:        for (uint256 i = 0; i < operatorsCache.length; i++) {
NestedFactory.sol:113:        for (uint256 i = 0; i < operatorsLength; i++) {
NestedFactory.sol:153:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:213:        for (uint256 i = 0; i < tokensLength; i++) {
NestedFactory.sol:273:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:291:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:327:        for (uint256 i = 0; i < batchLength; i++) {
NestedFactory.sol:369:        for (uint256 i = 0; i < _batchedOrders.orders.length; i++) {
NestedFactory.sol:581:        for (uint256 i = 0; i < _batchedOrders.length; i++) {
NestedRecords.sol:71:            uint256 tokenIndex = 0;
NestedRecords.sol:196:        for (uint256 i = 0; i < tokensCount; i++) {
OperatorResolver.sol:40:        for (uint256 i = 0; i < namesLength; i++) {
OperatorResolver.sol:60:        for (uint256 i = 0; i < names.length; i++) {
OperatorResolver.sol:75:        for (uint256 i = 0; i < destinations.length; i++) {

I suggest removing explicit initializations for default values.

Comparisons

Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

Places I suggest adding a non-zero-value check:

FeeSplitter.sol:155:                _tokens[i].safeTransfer(_msgSender(), amount);
FeeSplitter.sol:167:            _tokens[i].safeTransfer(_msgSender(), amount);
FeeSplitter.sol:182:        _token.safeTransferFrom(_msgSender(), address(this), _amount);
FeeSplitter.sol:199:        _token.safeTransferFrom(_msgSender(), address(this), _amount);
NestedFactory.sol:134:        _token.safeTransfer(owner(), amount);
NestedFactory.sol:467:        _token.safeTransfer(reserveAddr, _amount);
NestedFactory.sol:503:            _inputToken.safeTransferFrom(_msgSender(), address(this), _inputTokenAmount);
NestedFactory.sol:558:            _token.safeTransfer(_dest, _amount);
NestedFactory.sol:574:        _token.safeTransfer(_dest, _amount - feeAmount);
NestedReserve.sol:23:        _token.safeTransfer(_recipient, _amount);
NestedReserve.sol:30:        _token.safeTransfer(msg.sender, _amount);

For-Loops

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

abstracts\MixinOperatorResolver.sol:36:        for (uint256 i = 0; i < requiredOperators.length; i++) {
abstracts\MixinOperatorResolver.sol:55:        for (uint256 i = 0; i < requiredOperators.length; i++) {
FeeSplitter.sol:126:        for (uint256 i = 0; i < accountsLength; i++) {
FeeSplitter.sol:148:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:165:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:261:        for (uint256 i = 0; i < shareholders.length; i++) {
FeeSplitter.sol:280:        for (uint256 i = 0; i < shareholdersCache.length; i++) {
FeeSplitter.sol:318:        for (uint256 i = 0; i < shareholders.length; i++) {
NestedFactory.sol:103:        for (uint256 i = 0; i < operatorsCache.length; i++) {
NestedFactory.sol:113:        for (uint256 i = 0; i < operatorsLength; i++) {
NestedFactory.sol:153:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:213:        for (uint256 i = 0; i < tokensLength; i++) {
NestedFactory.sol:273:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:291:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:327:        for (uint256 i = 0; i < batchLength; i++) {
NestedFactory.sol:369:        for (uint256 i = 0; i < _batchedOrders.orders.length; i++) {
NestedFactory.sol:581:        for (uint256 i = 0; i < _batchedOrders.length; i++) {
NestedRecords.sol:78:                tokenIndex++;
NestedRecords.sol:196:        for (uint256 i = 0; i < tokensCount; i++) {
OperatorResolver.sol:40:        for (uint256 i = 0; i < namesLength; i++) {
OperatorResolver.sol:60:        for (uint256 i = 0; i < names.length; i++) {
OperatorResolver.sol:75:        for (uint256 i = 0; i < destinations.length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

abstracts\MixinOperatorResolver.sol:36:        for (uint256 i = 0; i < requiredOperators.length; i++) {
abstracts\MixinOperatorResolver.sol:55:        for (uint256 i = 0; i < requiredOperators.length; i++) {
FeeSplitter.sol:126:        for (uint256 i = 0; i < accountsLength; i++) {
FeeSplitter.sol:148:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:165:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:261:        for (uint256 i = 0; i < shareholders.length; i++) {
FeeSplitter.sol:280:        for (uint256 i = 0; i < shareholdersCache.length; i++) {
FeeSplitter.sol:318:        for (uint256 i = 0; i < shareholders.length; i++) {
NestedFactory.sol:103:        for (uint256 i = 0; i < operatorsCache.length; i++) {
NestedFactory.sol:113:        for (uint256 i = 0; i < operatorsLength; i++) {
NestedFactory.sol:153:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:213:        for (uint256 i = 0; i < tokensLength; i++) {
NestedFactory.sol:273:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:291:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
NestedFactory.sol:327:        for (uint256 i = 0; i < batchLength; i++) {
NestedFactory.sol:369:        for (uint256 i = 0; i < _batchedOrders.orders.length; i++) {
NestedFactory.sol:581:        for (uint256 i = 0; i < _batchedOrders.length; i++) {
NestedRecords.sol:78:                tokenIndex++;
NestedRecords.sol:196:        for (uint256 i = 0; i < tokensCount; i++) {
OperatorResolver.sol:40:        for (uint256 i = 0; i < namesLength; i++) {
OperatorResolver.sol:60:        for (uint256 i = 0; i < names.length; i++) {
OperatorResolver.sol:75:        for (uint256 i = 0; i < destinations.length; i++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

abstracts\MixinOperatorResolver.sol:36:        for (uint256 i = 0; i < requiredOperators.length; i++) {
abstracts\MixinOperatorResolver.sol:55:        for (uint256 i = 0; i < requiredOperators.length; i++) {
FeeSplitter.sol:148:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:165:        for (uint256 i = 0; i < _tokens.length; i++) {
FeeSplitter.sol:261:        for (uint256 i = 0; i < shareholders.length; i++) {
FeeSplitter.sol:280:        for (uint256 i = 0; i < shareholdersCache.length; i++) {
FeeSplitter.sol:318:        for (uint256 i = 0; i < shareholders.length; i++) {
NestedFactory.sol:103:        for (uint256 i = 0; i < operatorsCache.length; i++) {
NestedFactory.sol:369:        for (uint256 i = 0; i < _batchedOrders.orders.length; i++) {
NestedFactory.sol:581:        for (uint256 i = 0; i < _batchedOrders.length; i++) {
OperatorResolver.sol:60:        for (uint256 i = 0; i < names.length; i++) {
OperatorResolver.sol:75:        for (uint256 i = 0; i < destinations.length; i++) {

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes are here:

abstracts\OwnableProxyDelegation.sol:56:        require(newOwner != address(0), "Ownable: new owner is the zero address");
operators\ZeroEx\ZeroExOperator.sol:36:        require(amountBought != 0, "ZeroExOperator::performSwap: amountBought cant be zero");
operators\ZeroEx\ZeroExOperator.sol:37:        require(amountSold != 0, "ZeroExOperator::performSwap: amountSold cant be zero");
NestedFactory.sol:444:            require(amounts[1] <= _amountToSpend, "NestedFactory::_safeSubmitOrder: Overspent"); 

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

abstracts\MixinOperatorResolver.sol:76:        require(_foundAddress.implementation != address(0), string(abi.encodePacked("MOR: MISSING_OPERATOR: ", name)));
abstracts\MixinOperatorResolver.sol:100:            require(tokens[0] == _outputToken, "OH: INVALID_OUTPUT_TOKEN");
abstracts\MixinOperatorResolver.sol:101:            require(tokens[1] == _inputToken, "OH: INVALID_OUTPUT_TOKEN");
abstracts\OwnableFactoryHandler.sol:21:        require(supportedFactories[msg.sender], "OFH: FORBIDDEN");
abstracts\OwnableFactoryHandler.sol:28:        require(_factory != address(0), "OFH: INVALID_ADDRESS");
abstracts\OwnableFactoryHandler.sol:36:        require(supportedFactories[_factory], "OFH: NOT_SUPPORTED");
abstracts\OwnableProxyDelegation.sol:25:        require(!initialized, "OFP: INITIALIZED");
abstracts\OwnableProxyDelegation.sol:26:        require(StorageSlot.getAddressSlot(_ADMIN_SLOT).value == msg.sender, "OFP: FORBIDDEN");
abstracts\OwnableProxyDelegation.sol:40:        require(owner() == _msgSender(), "Ownable: caller is not the owner");
abstracts\OwnableProxyDelegation.sol:56:        require(newOwner != address(0), "Ownable: new owner is the zero address");
operators\Flat\FlatOperator.sol:18:        require(amount != 0, "FO: INVALID_AMOUNT");
operators\ZeroEx\ZeroExOperator.sol:32:        require(success, "ZEO: SWAP_FAILED");
operators\ZeroEx\ZeroExOperator.sol:36:        require(amountBought != 0, "ZeroExOperator::performSwap: amountBought cant be zero");
operators\ZeroEx\ZeroExOperator.sol:37:        require(amountSold != 0, "ZeroExOperator::performSwap: amountSold cant be zero");
FeeSplitter.sol:94:        require(_weth != address(0), "FS: INVALID_ADDRESS");
FeeSplitter.sol:103:        require(msg.sender == weth, "FS: ETH_SENDER_NOT_WETH");
FeeSplitter.sol:111:        require(_weight != 0, "FS: WEIGHT_ZERO");
FeeSplitter.sol:123:        require(accountsLength != 0 && accountsLength == _weights.length, "FS: INPUTS_LENGTH_MUST_MATCH");
FeeSplitter.sol:135:        require(_accountIndex < shareholders.length, "FS: INVALID_ACCOUNT_INDEX");
FeeSplitter.sol:137:        require(totalWeights != 0, "FS: TOTAL_WEIGHTS_ZERO");
FeeSplitter.sol:153:                require(success, "FS: ETH_TRANFER_ERROR");
FeeSplitter.sol:196:        require(_royaltiesTarget != address(0), "FS: INVALID_ROYALTIES_TARGET");
FeeSplitter.sol:306:        require(amountToRelease != 0, "FS: NO_PAYMENT_DUE");
FeeSplitter.sol:316:        require(_weight != 0, "FS: ZERO_WEIGHT");
FeeSplitter.sol:317:        require(_account != address(0), "FS: INVALID_ADDRESS");
FeeSplitter.sol:319:            require(shareholders[i].account != _account, "FS: ALREADY_SHAREHOLDER");
NestedAsset.sol:34:        require(_address == ownerOf(_tokenId), "NA: FORBIDDEN_NOT_OWNER");
NestedAsset.sol:44:        require(_exists(_tokenId), "URI query for nonexistent token");
NestedAsset.sol:78:        require(_exists(_replicatedTokenId) && tokenId != _replicatedTokenId, "NA: INVALID_REPLICATED_TOKEN_ID");
NestedAsset.sol:111:        require(bytes(tokenURI(_tokenId)).length == 0, "NA: TOKEN_URI_IMMUTABLE");
NestedFactory.sol:54:        require(
NestedFactory.sol:78:        require(nestedAsset.ownerOf(_nftId) == _msgSender(), "NF: CALLER_NOT_OWNER");
NestedFactory.sol:86:        require(block.timestamp > nestedRecords.getLockTimestamp(_nftId), "NF: LOCKED_NFT");
NestedFactory.sol:101:        require(operator != bytes32(""), "NF: INVALID_OPERATOR_NAME");
NestedFactory.sol:104:            require(operatorsCache[i] != operator, "NF: EXISTENT_OPERATOR");
NestedFactory.sol:126:        require(address(_feeSplitter) != address(0), "NF: INVALID_FEE_SPLITTER_ADDRESS");
NestedFactory.sol:148:        require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");
NestedFactory.sol:207:        require(_orders.length != 0, "NF: INVALID_ORDERS");
NestedFactory.sol:208:        require(tokensLength == _orders.length, "NF: INPUTS_LENGTH_MUST_MATCH");
NestedFactory.sol:209:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:243:        require(assetTokensLength > _tokenIndex, "NF: INVALID_TOKEN_INDEX");
NestedFactory.sol:245:        require(assetTokensLength > 1, "NF: UNALLOWED_EMPTY_PORTFOLIO");
NestedFactory.sol:246:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:270:        require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");
NestedFactory.sol:271:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:288:        require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");
NestedFactory.sol:289:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:317:        require(batchLength != 0, "NF: INVALID_ORDERS");
NestedFactory.sol:337:        require(amountSpent <= _inputTokenAmount - feesAmount, "NF: OVERSPENT");
NestedFactory.sol:363:        require(batchLength != 0, "NF: INVALID_ORDERS");
NestedFactory.sol:364:        require(_batchedOrders.amounts.length == batchLength, "NF: INPUTS_LENGTH_MUST_MATCH");
NestedFactory.sol:385:            require(amountSpent <= _inputTokenAmount, "NF: OVERSPENT");
NestedFactory.sol:418:        require(success, "NF: OPERATOR_CALL_FAILED");
NestedFactory.sol:444:            require(amounts[1] <= _amountToSpend, "NestedFactory::_safeSubmitOrder: Overspent");
NestedFactory.sol:489:            require(address(this).balance >= _inputTokenAmount, "NF: INVALID_AMOUNT_IN");
NestedFactory.sol:496:            require(
NestedFactory.sol:556:            require(success, "NF: ETH_TRANSFER_ERROR");
NestedFactory.sol:586:        require(msg.value == ethNeeded, "NF: WRONG_MSG_VALUE");
NestedRecords.sol:53:        require(_maxHoldingsCount != 0, "NRC: INVALID_MAX_HOLDINGS");
NestedRecords.sol:92:        require(records[_nftId].holdings[token] != 0, "NRC: HOLDING_INACTIVE");
NestedRecords.sol:119:            require(records[_nftId].reserve == _reserve, "NRC: RESERVE_MISMATCH");
NestedRecords.sol:123:        require(records[_nftId].tokens.length < maxHoldingsCount, "NRC: TOO_MANY_TOKENS");
NestedRecords.sol:124:        require(
NestedRecords.sol:140:        require(_timestamp > records[_nftId].lockTimestamp, "NRC: LOCK_PERIOD_CANT_DECREASE");
NestedReserve.sol:22:        require(_recipient != address(0), "NRS: INVALID_ADDRESS");
OperatorResolver.sol:27:        require(_foundOperator.implementation != address(0), reason);
OperatorResolver.sol:39:        require(namesLength == destinations.length, "OR: INPUTS_LENGTH_MUST_MATCH");
OperatorResolver.sol:57:        require(names.length == operatorsToImport.length, "OR: INPUTS_LENGTH_MUST_MATCH");

I suggest replacing revert strings with custom errors.