QA Report

[code423n4]

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    NestedFactory.constructor (_feeSplitter)
    NestedFactory.setFeeSplitter (_feeSplitter)

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended due ti undefined behaviors as a result of it.

Title: Init function calls an owner function Severity: Low Risk

Init function that calls an onlyOwner function is problematic since sometimes the initializer or the one applies 
the constructor isn't necessary the owner of the protocol. And if a contract does it then you might get a situation
that all the onlyOwner functions are blocked since only the factory contract may use them but isn't necessary 
support it. 

    FeeSplitter.sol.constructor - calls setRoyaltiesWeight
    FeeSplitter.sol.constructor - calls setShareholders

Title: Not verified owner Severity: Low Risk

owner param should be validated to make sure the owner address is not address(0).
Otherwise if not given the right input all only owner accessible functions will be unaccessible.

    OwnableProxyDelegation.sol.transferOwnership newOwner
    OwnableProxyDelegation.sol.initialize ownerAddr
    NestedAsset.sol.burn _owner
    NestedAsset.sol.mint _owner
    NestedAsset.sol.backfillTokenURI _owner
    NestedAsset.sol.mintWithMetadata _owner
    OwnableProxyDelegation.sol._setOwner newOwner

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    OwnableProxyDelegation.sol

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    FeeSplitter.sol, updateShareholder is missing a reentrancy modifier
    NestedFactory.sol, receive is missing a reentrancy modifier
    FeeSplitter.sol, receive is missing a reentrancy modifier
    NestedFactory.sol, removeOperator is missing a reentrancy modifier
    NestedFactory.sol, addOperator is missing a reentrancy modifier
    NestedFactory.sol, setFeeSplitter is missing a reentrancy modifier
    FeeSplitter.sol, setShareholders is missing a reentrancy modifier
    NestedFactory.sol, unlockTokens is missing a reentrancy modifier
    FeeSplitter.sol, setRoyaltiesWeight is missing a reentrancy modifier
    NestedFactory.sol, updateLockTimestamp is missing a reentrancy modifier

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    ZeroExStorage.sol, updatesSwapTarget
    NestedRecords.sol, updateHoldingAmount
    NestedFactory.sol, updateLockTimestamp
    NestedRecords.sol, updateLockTimestamp
    FeeSplitter.sol, updateShareholder

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    MockERC20.sol: function constructor parameter _symbol isn't used. (constructor is default)
    NestedAsset.sol: function backfillTokenURI parameter _owner isn't used. (backfillTokenURI is external)
    TestableMixingOperatorResolver.sol: function constructor parameter _resolver isn't used. (constructor is default)
    DeflationaryMockERC20.sol: function constructor parameter _symbol isn't used. (constructor is default)
    DeflationaryMockERC20.sol: function constructor parameter _name isn't used. (constructor is default)
    TestableOperatorCaller.sol: function performSwap parameter own isn't used. (performSwap is external)
    MockERC20.sol: function constructor parameter _name isn't used. (constructor is default)

Title: Missing commenting Severity: Low Risk

The following functions are missing commenting as describe below:

    FeeSplitter.sol, _addShareholder (private), parameters _account, _weight not commented
    NestedRecords.sol, getAssetTokens (public), @return is missing
    FeeSplitter.sol, _releaseToken (private), @return is missing
    FeeSplitter.sol, _computeShareCount (private), parameters _amount, _weight, _totalWeights not commented
    NestedRecords.sol, getAssetHolding (public), @return is missing
    FeeSplitter.sol, _releaseToken (private), parameters _account, _token not commented
    FeeSplitter.sol, _computeShareCount (private), @return is missing

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    NestedFactory.withdraw
    NestedReserve.withdraw

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    FeeSplitter.sol._addShares _token
    FeeSplitter.sol._addShares _account
    DeflationaryMockERC20.sol.transferFrom recipient
    OwnableFactoryHandler.sol.removeFactory _factory

[maximebrugel]

"Does not validate the input fee parameter" (Disputed)

• In NestedFactory.constructor, the _feeSplitter param is checked:

  require(
  address(_nestedAsset) != address(0) &&
      address(_nestedRecords) != address(0) &&
      address(_reserve) != address(0) && 
      address(_feeSplitter) != address(0) && // Checked
      address(_weth) != address(0) &&
      _operatorResolver != address(0),
  "NF: INVALID_ADDRESS"

• In NestedFactory.setFeeSplitter, the _feeSplitter param is checked:

  require(address(_feeSplitter) != address(0), "NF: INVALID_FEE_SPLITTER_ADDRESS");

"Solidity compiler versions mismatch" (Disputed)

All the files in the scope (excluding mocks) are in Solidity 0.8.11.

"Init function calls an owner function" (Disputed)

The FeeSplitter is not deployed by a smart contract.

"Not verified owner" (6 Disputed/ 1 Confirmed)

• OwnableProxyDelegation.sol.transferOwnership newOwner => (Disputed)

  require(newOwner != address(0), "Ownable: new owner is the zero address");

• OwnableProxyDelegation.sol.initialize ownerAddr => (Confirmed)
• _NestedAsset.sol.burn owner => (Disputed) NestedFactory is managing the parameter and it's always msg.sender.
• _NestedAsset.sol.mint owner => (Disputed) NestedFactory is managing the parameter and it's always msg.sender.
• _NestedAsset.sol.backfillTokenURI owner => (Disputed) NestedFactory is managing the parameter and it's always msg.sender.
• _NestedAsset.sol.mintWithMetadata owner => (Disputed) NestedFactory is managing the parameter and it's always msg.sender.
• _OwnableProxyDelegation.sol.setOwner newOwner => (Disputed) Must be checked by caller. Otherwise renounceOwnership doesn't work.

"Two Steps Verification before Transferring Ownership" (Disputed)

Already surfaced in the first audit : https://github.com/code-423n4/2021-11-nested-findings/issues/101

"Missing non reentrancy modifier" (Disputed)

No reentrancy on owner functions...

"Never used parameters" (Disputed)

_owner is used by the onlyTokenOwner modifier. The rest is out of scope.

"Missing commenting" (Disputed)

Not explaining why the comments are needed. Can be well explained in @notice or @dev.

"Anyone can withdraw others" (Disputed)

No. onlyTokenOwner and onlyFactory

"Not verified input" (Disputed)

• DeflationaryMockERC20.transferFrom is out of scope.
• FeeSplitter._addShares can't really happen for this function.
• OwnableFactoryHandler.removeFactory it's fine, will revert with "OFH: NOT_SUPPORTED".

[maximebrugel]

In conclusion, only confirmed for : "OwnableProxyDelegation.sol.initialize ownerAddr => (Confirmed)"

[harleythedogC4]

My personal judgements:

1. For the only confirmed finding - I will assign it Valid and very-low-critical.

[harleythedogC4]

Now, here is the methodology I used for calculating a score for each QA report. I first assigned each submission to be either non-critical (1 point), very-low-critical (5 points) or low-critical (10 points), depending on how severe/useful the issue is. The score of a QA report is the sum of these points, divided by the maximum number of points achieved by a QA report. This maximum number was 26 points, achieved by #66.

The number of points achieved by this report is 5 points. Thus the final score of this QA report is (5/26)*100 = 19.