QA Report

[code423n4]

Does not validate the input fee parameter

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

Code instances:

    NestedBuybacker.setFeeSplitter (_feeSplitter)
    NestedFactory.setEntryFees (_entryFees)
    UpdateFees.updateFees (exitFees)
    NestedFactory.setExitFees (_exitFees)
    UpdateFees.updateFees (entryFees)
    NestedFactory.setFeeSplitter (_feeSplitter)

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated.
(see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instance:

    Deprecated safeApprove in DummyRouter.sol line 45: _token.approve(_spender, _amount);

Dangerous usage of tx.origin

Use of tx.origin for authorization may be abused by a MITM malicious contract forwarding calls from the legitimate user who interacts with it. Use msg.sender instead.

Code instance:

    TokenTransferProxy.sol, 41: require(from == tx.origin || from.isContract(), "Invalid from address");

Missing commenting

    The following functions are missing commenting as describe below:

Code instances:

    BeefyZapBiswapLPVaultOperator.sol, _getOptimalSwapAmount (private), parameters reserveA, reserveB, router not commented
    NestedAsset.sol, setIsRevealed (external), parameter _isRevealed not commented
    FeeSplitter.sol, _releaseToken (private), @return is missing
    MixinOperatorResolver.sol, isResolverCached (external), @return is missing
    FeeSplitter.sol, _releaseToken (private), parameters _account, _token not commented
    FeeSplitter.sol, _addShareholder (private), parameters _account, _weight not commented
    BeefyZapUniswapLPVaultOperator.sol, _getOptimalSwapAmount (private), parameters reserveA, reserveB, router not commented
    NestedAsset.sol, _baseURI (internal), @return is missing

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedBuybacker.sol#L77
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedFactory.sol#L159
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedAsset.sol#L136

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedReserve.sol#L21
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedFactory.sol#L631
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/mocks/DummyRouter.sol#L29
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/mocks/WETHMock.sol#L48
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/Withdrawer.sol#L27

Never used parameters

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.

Code instances:

    StakingLPVaultHelpers.sol: function _addLiquidityAndDepositETH parameter eth isn't used. (_addLiquidityAndDepositETH is internal)
    StakingLPVaultHelpers.sol: function _addLiquidityAndDepositETH parameter amount isn't used. (_addLiquidityAndDepositETH is internal)
    TestableOperatorCaller.sol: function performSwap parameter own isn't used. (performSwap is external)

Require with empty message

The following requires are with empty messages.
This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instance:

    Solidity file: WETHMock.sol, In line 46 with Empty Require message.

Not verified input

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.

Code instances:

    NestedRecords.sol.updateHoldingAmount _token
    BeefyZapUniswapLPVaultOperator.sol.withdraw vault
    YearnCurveVaultOperator.sol.withdraw256 vault
    NestedBuybacker.sol.setNestedReserve _nstReserve

Solidity compiler versions mismatch

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Code instance:

Init function calls an owner function

    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. 

Code instances:

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

Not verified owner

    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.

Code instances:

    OwnableProxyDelegation.sol.initialize ownerAddr
    OwnableProxyDelegation.sol.transferOwnership newOwner
    NestedAsset.sol.mint _owner
    NestedAsset.sol.burn _owner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    CurveHelpers.sol, getAmounts4Coins
    TimelockControllerEmergency.sol, isOperation
    TimelockControllerEmergency.sol, isOperationReady

Two Steps Verification before Transferring Ownership

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

Code instance:

    OwnableProxyDelegation.sol

Missing non reentrancy modifier

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..

Code instances:

    Withdrawer.sol, receive is missing a reentrancy modifier
    NestedFactory.sol, unlockTokens is missing a reentrancy modifier

In the following public update functions no value is returned

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: code-423n4/2021-10-badgerdao-findings#85).

Code instances:

    ZeroExStorage.sol, updatesSwapTarget
    NestedFactory.sol, updateLockTimestamp
    NestedRecords.sol, updateHoldingAmount
    FeeSplitter.sol, updateShareholder
    TimelockControllerEmergency.sol, updateDelay
    NestedRecords.sol, updateLockTimestamp
    UpdateFees.sol, updateFees

Never used parameters

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.

Code instances:

    StakingLPVaultHelpers.sol: function _addLiquidityAndDepositETH parameter eth isn't used. (_addLiquidityAndDepositETH is internal)
    StakingLPVaultHelpers.sol: function _addLiquidityAndDepositETH parameter amount isn't used. (_addLiquidityAndDepositETH is internal)
    TestableOperatorCaller.sol: function performSwap parameter own isn't used. (performSwap is external)

Dangerous usage of tx.origin

Use of tx.origin for authorization may be abused by a MITM malicious contract forwarding calls from the legitimate user who interacts with it. Use msg.sender instead.

Code instance:

    TokenTransferProxy.sol, 41: require(from == tx.origin || from.isContract(), "Invalid from address");

Missing commenting

    The following functions are missing commenting as describe below:

Code instances:

    BeefyZapBiswapLPVaultOperator.sol, _getOptimalSwapAmount (private), parameters reserveA, reserveB, router not commented
    NestedAsset.sol, setIsRevealed (external), parameter _isRevealed not commented
    FeeSplitter.sol, _releaseToken (private), @return is missing
    MixinOperatorResolver.sol, isResolverCached (external), @return is missing
    FeeSplitter.sol, _releaseToken (private), parameters _account, _token not commented
    FeeSplitter.sol, _addShareholder (private), parameters _account, _weight not commented
    BeefyZapUniswapLPVaultOperator.sol, _getOptimalSwapAmount (private), parameters reserveA, reserveB, router not commented
    NestedAsset.sol, _baseURI (internal), @return is missing