QA Report

[code423n4]

owner can sweep all funds

description

the owner can withdraw all tokens from NestedFactory.sol with no timelock

To give more trust to users: this function should be put behind a timelock.

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L175-L179

use of magic numbers

description

constants should be declared rather than use magic numbers

findings

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L264 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L378 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L443

fees could be zero

description

this could happen when exitFees has not been initialized yet by calling setExitFees(), exitFees will be zero which makes amountFees zero

for small amounts of amountBought, amountFees could round down to zero due to division by 10000

findings

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L264

fees can be set with no time lock

description

fees can be set to 100% with no time lock

a malicious admin can front run transactions by setting fee to 100% to steal funds

recommend adding a timelock to this function

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L167-L172

No Transfer Ownership Pattern

description

Recommend considering implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

findings

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L56-L59

missing checks for zero address

description

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

findings

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L48-L50

[obatirou]

owner can sweep all funds (disputed)

There is a timelock, see ownership documentation in readme

[Yashiru]

Use of magic numbers (Duplicated)

Duplicated of #76 at 5. constants should be defined rather than using magic numbers

[Yashiru]

Missing checks for zero address (Duplicated)

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