Nested Finance contest - simon135's results

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

1. REQUIRE INSTEAD OF &&

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas. instances include : NestedFactory.sol:69 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/BeefyVaultOperator.sol#L54 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L61-L62 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L63-L65 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Paraswap/ParaswapOperator.sol#L16

2. save gas by using msg.sender instead of msgSender() function

msg.sender is global variable and msgSender() function needs th get that variable making it cost more gas. instances: NestedFactory.sol:104,200,207

3. Mark functions as payable when users can't mistakenly send ETH

impact

Functions marked as payable are 24 gas cheaper than their counterpart (in non-payable functions, Solidity adds an extra check to ensure msg.value is zero). When users can't mistakenly send ETH to a function (as an example, when there's an onlyOwner modifier or alike), it is safe to mark it as payable

proof of concept

Functions with onlyOwner modifier that aren't payable yet: instances include: NestedFactory.sol 126: function addOperator(bytes32 operator) external override onlyOwner { 139: function removeOperator(bytes32 operator) external override onlyOwner { 158: function setFeeSplitter(FeeSplitter _feeSplitter) external override onlyOwner { 165: function setEntryFees(uint256 _entryFees) external override onlyOwner { 173: function setExitFees(uint256 _exitFees) external override onlyOwner { 181: function unlockTokens(IERC20 _token) external override onlyOwner { OpertorResolver.sol 56: ) external override onlyOwner { 74: function rebuildCaches(MixinOperatorResolver[] calldata destinations) public onlyOwner { https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/BeefyVaultStorage.sol#L34 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/operators/Beefy/BeefyVaultStorage.sol#L24

recommended mitigation steps

Mark as payable the functions that have a "safe for users" modifier

4. 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. instances include: NestedFactory.sol : 130: for (uint256 i = 0; i < operatorsCache.length; i++) { 142: for (uint256 i = 0; i < operatorsLength; i++) { 202: for (uint256 i = 0; i < batchedOrdersLength; i++) { 262: for (uint256 i = 0; i < tokensLength; i++) { 321: for (uint256 i = 0; i < batchedOrdersLength; i++) { 339: for (uint256 i = 0; i < batchedOrdersLength; i++) { 375: for (uint256 i = 0; i < batchLength; i++) { 418: for (uint256 i = 0; i < batchLength; i++) { OpertorResolver.sol: 40: for (uint256 i = 0; i < namesLength; i++) { 60: for (uint256 i = 0; i < names.length; i++) { 75: for (uint256 i = 0; i < destinations.length; i++) { MixinOperatorResolver.sol: 37: for (uint256 i = 0; i < requiredOperators.length; i++) { 56: for (uint256 i = 0; i < requiredOperators.length; i++) { TimelockControllerEmergency.sol: 84,89,234,324

5. 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. 1 byte for character instanaces: https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/abstracts/MixinOperatorResolver.sol#L77 https://github.com/code-423n4/2022-06-nested/blob/0dc44d779eaca8f40b7526aabdd81a098dcebf25/contracts/libraries/StakingLPVaultHelpers.sol#L138 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/governance/scripts/OperatorScripts.sol#L19-L20 https://github.com/code-423n4/2022-06-nested/blob/b253ed80f67d1bb2a04e1702f5796fd96a7c521e/contracts/governance/scripts/OperatorScripts.sol#L54