code423n4 commented 2 years ago

1 . unused imports ( its already imported)

Ierc20 is already imported in Inestedfactory.sol Feespliter.sol is already imported Inestedfactory.sol NestedReverse.sol is already imprted Insteadfactory.sol NestedFactory:6 NestedFactory:12 safeerc20 imports Ierc20 so you can take out Ierc20 when you import safeerc20.sol NestedFactory:13 IOperatorResolver.sol is already imported in MixinOperatorResolver.sol take out IOperatorResolver.sol from OperatorResolver.sol OperatorResolver:4 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L10 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L10 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Paraswap/ParaswapOperator.sol#L6 safeerc20.sol is already in exchangehelper.sol https://github.com/code-423n4/2022-06-nested/blob/0dc44d779eaca8f40b7526aabdd81a098dcebf25/contracts/libraries/StakingLPVaultHelpers.sol#L10

2. Change your imports

ex: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; instead do your imports like this import {Ierc20,safeer20} from "import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/NestedFactory.sol#L5 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L10

3. no check that address can be zero in array of address

no check that token of i can be zero address token = tokens[i]; instances: https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L257 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/BeefyVaultOperator.sol#L18 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L28

4. make success variable in call function a check in a require statement . instead of a if statment where if success is false it will just skip execution not revert.

make if (success) into: require(success) to make sure the call dosnt fail https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/NestedFactory.sol#L518

5. initialize function should have onlyowner modifer anyone can front run the initialize call and become owner.

If the initializer is not executed in the same transaction as the constructor, a malicious user can front-run the initialize() call, forcing the contract to be redeployed ex: function initialize(address ownerAddr) external { https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/abstracts/OwnableProxyDelegation.sol#L24

6. no check on success variable , if the call function fails

bool success variable should be checked with a require statement if not logic can brake and cause loss of funds https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/libraries/ExchangeHelpers.sol#L22

7.typos

instead of : liquitiy use : liquidity https://github.com/code-423n4/2022-06-nested/blob/0dc44d779eaca8f40b7526aabdd81a098dcebf25/contracts/libraries/StakingLPVaultHelpers.sol#L52 https://github.com/code-423n4/2022-06-nested/blob/0dc44d779eaca8f40b7526aabdd81a098dcebf25/contracts/libraries/StakingLPVaultHelpers.sol#L85 https://github.com/code-423n4/2022-06-nested/blob/0dc44d779eaca8f40b7526aabdd81a098dcebf25/contracts/libraries/StakingLPVaultHelpers.sol#L115

8. Event is missing indexed fields

each event should use three indexed fields if there are there or more fields https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/governance/TimelockControllerEmergency.sol#L50

obatirou commented 2 years ago

6. no check on success variable , if the call function fails (disputed)

Check is done in contract when using the library

Yashiru commented 2 years ago

2. Change your imports (Acknowledged)

Indeed we could do it this way, but it is only a development choice and we prefer to keep our imports as they are.

obatirou commented 2 years ago

4. make success variable in call function a check in a require statement . instead of a if statment where if success is false it will just skip execution not revert. (disputed)

This statement is confused The link point to _transferToReserveAndStore where there is no mention of success variable

Yashiru commented 2 years ago

5. initialize function should have onlyowner modifer anyone can front run the initialize call and become owner (Disputed)

Already surfaced in previous audit

obatirou commented 2 years ago

8. Event is missing indexed fields (duplicate)

Duplicate from https://github.com/code-423n4/2022-06-nested-findings/issues/11#issuecomment-1165618378

Yashiru commented 2 years ago

7.typos (Duplicated)

Duplicated of #45 at Typos

Yashiru commented 2 years ago

3. no check that address can be zero in array of address (Duplicated)

Duplicated of #61 at 2. Missing address(0) checks

obatirou commented 2 years ago