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.constructor (_feeSplitter)
    NestedFactory.setFeeSplitter (_feeSplitter)
    NestedBuybacker.constructor (_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);

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
    CurveHelpers.sol, getAmounts2Coins
    CurveHelpers.sol, getAmounts3Coins
    CurveHelpers.sol, removeLiquidityOneCoin
    TimelockControllerEmergency.sol, getMinDelay
    TimelockControllerEmergency.sol, isOperationPending
    TimelockControllerEmergency.sol, getTimestamp
    TimelockControllerEmergency.sol, hashOperation
    TimelockControllerEmergency.sol, hashOperationBatch
    TimelockControllerEmergency.sol, isOperationDone

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

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
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedFactory.sol#L177

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

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
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/mocks/DummyRouter.sol#L41
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedRecords.sol#L52
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedRecords.sol#L157
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedBuybacker.sol#L61
    https://github.com/code-423n4/2022-06-nested/tree/main/contracts/NestedAsset.sol#L122

Must approve 0 first

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.

Code instance:

approve without approving 0 first DummyRouter.sol, 45, _token.approve(_spender, _amount);

Unbounded loop on array that can only grow can lead to DoS

A malicious attacker that is also a protocol owner can push unlimitedly to an array, that some function loop over this array.
If increasing the array size enough, calling the function that does a loop over the array will always revert since there is a gas limit.
This is a Med Risk issue since it can lead to DoS with a reasonable chance of having untrusted owner or even an owner that did a mistake in good faith.

Code instances:

    FeeSplitter.sol (L258): Unbounded loop on the array shareholders that can be publicly pushed by ['_addShareholder'] and can't be pulled
    FeeSplitter.sol (L315): Unbounded loop on the array shareholders that can be publicly pushed by ['_addShareholder'] and can't be pulled

Potential DoS

the balance of outputToken is checked to be exactly a specified value that is not declared in this specific function.
Therefore, a malicious user can transfer to the contract address tiny amount of tokens and the user transactions will always revert.

Code instances:

    Potential DoS in BeefyZapUniswapLPVaultOperator.sol, 108
    Potential DoS in BeefyZapBiswapLPVaultOperator.sol, 108
    Potential DoS in BeefyVaultOperator.sol, 95
    Potential DoS in BeefyVaultOperator.sol, 55

Override function but with different argument location

IYearnVault.sol.withdraw inherent IStakingVault.sol.withdraw but the parameters does not match
https://github.com/code-423n4/2022-06-nested/tree/main/contracts/interfaces/external/IStakingVault/IStakingVault.sol#L8

approve return value is ignored

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean.
Calling these functions with the correct EIP20 function signatures will always revert.
Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value.
We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens.
The list of occurrences in format (solidity file, line number, actual line)

Code instance:

DummyRouter.sol, 45, _token.approve(_spender, _amount);

transfer return value of a general ERC20 is ignored

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value
   Another popular possibility is to add a whiteList.
   Those are the appearances (solidity file, line number, actual line):

Code instances:

    DeflationaryMockERC20.sol, 29 (transferFrom), super.transferFrom(sender, recipient, (amount * 5) / 10);
    DummyRouter.sol, 29 (dummyswapToken), IERC20(_inputToken).transferFrom(msg.sender, address(this), _amount);
    DummyRouter.sol, 30 (dummyswapToken), _outputToken.transfer(msg.sender, _amount);
    AugustusSwapper.sol, 22 (dummyswapToken), IERC20(_outputToken).transfer(msg.sender, _amount);

[obatirou]

Require with empty message (disputed)

Code instances out of scope

Solidity compiler versions mismatch (disputed)

All contracts in scope are in 0.8.14

[obatirou]

Dangerous usage of tx.origin (disputed)

Code instances out of scope

safeApprove of openZeppelin is deprecated (disputed)

Code instance out of scope

Must approve 0 first (disputed)

Code instance out of scope

[obatirou]

approve return value is ignored (disputed)

Code instances out of scope

transfer return value of a general ERC20 is ignored (disputed)

Code instances out of scope

[obatirou]

Unbounded loop on array that can only grow can lead to DoS (disputed)

In readme + previous audit

[obatirou]

Potential DoS (disputed).

All interaction happens in one transaction
An attacker cannot send funds between withdraw/deposit and validation

[obatirou]

Override function but with different argument location. (disputed).

Overloading
Withdraw are different yes

[Yashiru]

Does not validate the input fee parameter (Disputed)

• NestedBuybacker.setFeeSplitter(_feeSplitter) => _feeSplitter is checked here
• NestedFactory.setEntryFees(_entryFees) => _entryFees is checked here
• UpdateFees.updateFees(entryFees) => entryFees is checked here
• NestedFactory.setExitFees(_exitFees) => _exitFees is checked here
• UpdateFees.updateFees(exitFees) => exitFees is checked here
• NestedFactory.constructor(_feeSplitter) => _feeSplitter is is checked in this require at this line
• NestedFactory.setFeeSplitter(_feeSplitter) => _feeSplitter is checked here
• NestedBuybacker.constructor(_feeSplitter) => _feeSplitter is checked here

[obatirou]

Never used parameters (disputed).

The parameters are used in the code

[obatirou]

public update functions no value is returned (disputed).

Some are out of scope and for the others, in the function signature, there is no returns hence it does not returns anything

[obatirou]

Missing non reentrancy modifier (disputed)

unlockTokens is onlyOwner
receiver of withdrawer: why would we need a modifier non reentrant ?

[Yashiru]

Not verified owner (Disputed)

• OwnableProxyDelegation.sol.initialize ownerAddr => ownerAddr is checked here
• OwnableProxyDelegation.sol.transferOwnership newOwner => newOwner is checked here
• NestedAsset.sol.mint _owner => mint() is always called by the NestedFactory, we never have address(0) here
• NestedAsset.sol.burn _owner => burn() is always called by the NestedFactory, we never have address(0) here

[obatirou]

Two Steps Verification before Transferring Ownership (disputed)

Already surfaced in precedent audit and readme

[obatirou]

Add a timelock (disputed)

Some are out of scope
For the others: there is a timelock, see ownership documentation in readme

[obatirou]

Named return issue (duplicated)

Duplicate point 3 of qa report #61

[obatirou]

Missing commenting (duplicate)

#84 (comment)

[jack-the-pug]

I'll just invalidate this QA report as there are so many unchecked bot results