QA Report #61

code423n4 · 2022-06-18T11:34:05Z

Overview

Risk Rating	Number of issues
Low Risk	6
Non-Critical Risk	4

Table of Contents

Low Risk Issues
Non-Critical Issues

Low Risk Issues

1. Known vulnerabilities exist in currently used `@openzeppelin/contracts` version

As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/[email protected] here:

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/package.json#L65

        "@openzeppelin/contracts": "^4.3.2",

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)

2. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

operators/Yearn/YearnCurveVaultOperator.sol:23:    address public immutable eth;
operators/Yearn/YearnCurveVaultOperator.sol:26:    IWETH private immutable weth;
operators/Yearn/YearnCurveVaultOperator.sol:29:    Withdrawer private immutable withdrawer;
utils/NestedAssetBatcher.sol:19:    INestedAsset public immutable nestedAsset;
utils/NestedAssetBatcher.sol:20:    INestedRecords public immutable nestedRecords;
Withdrawer.sol:14:    IWETH public immutable weth;

3. `OwnableProxyDelegation.initialize()` is front-runnable in the solution

I suggest adding some access control or atomically initializing the contract:

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

File: OwnableProxyDelegation.sol
24:     function initialize(address ownerAddr) external {
25:         require(ownerAddr != address(0), "OPD: INVALID_ADDRESS");
26:         require(!initialized, "OPD: INITIALIZED");
27:         require(StorageSlot.getAddressSlot(_ADMIN_SLOT).value == msg.sender, "OPD: FORBIDDEN");
28: 
29:         _setOwner(ownerAddr);
30: 
31:         initialized = true;
32:     }

4. Use a `constant` instead of duplicating the same string

abstracts/OwnableProxyDelegation.sol:25:        require(ownerAddr != address(0), "OPD: INVALID_ADDRESS");
abstracts/OwnableProxyDelegation.sol:57:        require(newOwner != address(0), "OPD: INVALID_ADDRESS");

governance/TimelockControllerEmergency.sol:229:        require(targets.length == values.length, "TimelockController: length mismatch");
governance/TimelockControllerEmergency.sol:230:        require(targets.length == datas.length, "TimelockController: length mismatch");
governance/TimelockControllerEmergency.sol:319:        require(targets.length == values.length, "TimelockController: length mismatch");
governance/TimelockControllerEmergency.sol:320:        require(targets.length == datas.length, "TimelockController: length mismatch");

governance/TimelockControllerEmergency.sol:334:        require(isOperationReady(id), "TimelockController: operation is not ready");
governance/TimelockControllerEmergency.sol:342:        require(isOperationReady(id), "TimelockController: operation is not ready");

libraries/CurveHelpers/CurveHelpers.sol:28:        revert("CH: INVALID_INPUT_TOKEN");
libraries/CurveHelpers/CurveHelpers.sol:48:        revert("CH: INVALID_INPUT_TOKEN");
libraries/CurveHelpers/CurveHelpers.sol:68:        revert("CH: INVALID_INPUT_TOKEN");

libraries/StakingLPVaultHelpers.sol:108:        require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");
libraries/StakingLPVaultHelpers.sol:138:        require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");

operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:52:        require(amountToDeposit != 0, "BLVO: INVALID_AMOUNT");
operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:97:        require(amount != 0, "BLVO: INVALID_AMOUNT");

operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:54:        require(router != address(0), "BLVO: INVALID_VAULT");
operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:99:        require(router != address(0), "BLVO: INVALID_VAULT");
operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:187:        require(pair.factory() == biswapRouter.factory(), "BLVO: INVALID_VAULT");

operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:97:        require(amount != 0, "BLVO: INVALID_AMOUNT");
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:52:        require(amountToDeposit != 0, "BLVO: INVALID_AMOUNT");

operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:271:        require(reserveA > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");
operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:272:        require(reserveB > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");

operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:54:        require(router != address(0), "BLVO: INVALID_VAULT");
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:99:        require(router != address(0), "BLVO: INVALID_VAULT");

operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:269:        require(reserveA > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:270:        require(reserveB > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");

operators/Beefy/BeefyVaultOperator.sol:41:        require(amount != 0, "BVO: INVALID_AMOUNT");
operators/Beefy/BeefyVaultOperator.sol:83:        require(amount != 0, "BVO: INVALID_AMOUNT");
operators/Beefy/BeefyVaultOperator.sol:96:        require(tokenAmount != 0, "BVO: INVALID_AMOUNT");

operators/Beefy/BeefyVaultOperator.sol:43:        require(address(token) != address(0), "BVO: INVALID_VAULT");
operators/Beefy/BeefyVaultOperator.sol:85:        require(address(token) != address(0), "BVO: INVALID_VAULT");

operators/Yearn/YearnCurveVaultOperator.sol:70:        require(amount != 0, "YCVO: INVALID_AMOUNT");
operators/Yearn/YearnCurveVaultOperator.sol:121:        require(amount != 0, "YCVO: INVALID_AMOUNT");
operators/Yearn/YearnCurveVaultOperator.sol:164:        require(amount != 0, "YCVO: INVALID_AMOUNT");
operators/Yearn/YearnCurveVaultOperator.sol:212:        require(amount != 0, "YCVO: INVALID_AMOUNT");
operators/Yearn/YearnCurveVaultOperator.sol:260:        require(amount != 0, "YCVO: INVALID_AMOUNT");

operators/Yearn/YearnCurveVaultOperator.sol:73:        require(pool != address(0), "YCVO: INVALID_VAULT");
operators/Yearn/YearnCurveVaultOperator.sol:123:        require(pool != address(0), "YCVO: INVALID_VAULT");
operators/Yearn/YearnCurveVaultOperator.sol:167:        require(pool != address(0), "YCVO: INVALID_VAULT");
operators/Yearn/YearnCurveVaultOperator.sol:215:        require(pool != address(0), "YCVO: INVALID_VAULT");
operators/Yearn/YearnCurveVaultOperator.sol:263:        require(pool != address(0), "YCVO: INVALID_VAULT");

NestedFactory.sol:160:        require(_entryFees != 0, "NF: ZERO_FEES");
NestedFactory.sol:168:        require(_exitFees != 0, "NF: ZERO_FEES");

NestedFactory.sol:161:        require(_entryFees <= 10000, "NF: FEES_OVERFLOW");
NestedFactory.sol:169:        require(_exitFees <= 10000, "NF: FEES_OVERFLOW");

NestedFactory.sol:191:        require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");
NestedFactory.sol:312:        require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");
NestedFactory.sol:330:        require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");

NestedFactory.sol:250:        require(_orders.length != 0, "NF: INVALID_ORDERS");
NestedFactory.sol:359:        require(batchLength != 0, "NF: INVALID_ORDERS");
NestedFactory.sol:406:        require(batchLength != 0, "NF: INVALID_ORDERS");

NestedFactory.sol:251:        require(tokensLength == _orders.length, "NF: INPUTS_LENGTH_MUST_MATCH");
NestedFactory.sol:407:        require(_batchedOrders.amounts.length == batchLength, "NF: INPUTS_LENGTH_MUST_MATCH");

NestedFactory.sol:252:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:289:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:313:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");
NestedFactory.sol:331:        require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

NestedFactory.sol:379:        require(amountSpent <= _inputTokenAmount - feesAmount, "NF: OVERSPENT");
NestedFactory.sol:428:            require(amountSpent <= _inputTokenAmount, "NF: OVERSPENT");
NestedFactory.sol:495:            require(amounts[1] <= _amountToSpend, "NF: OVERSPENT");

OperatorResolver.sol:39:        require(namesLength == destinations.length, "OR: INPUTS_LENGTH_MUST_MATCH");
OperatorResolver.sol:57:        require(names.length == operatorsToImport.length, "OR: INPUTS_LENGTH_MUST_MATCH");

5. Funds can be locked

There isn't a withdraw mechanism and several payable methods are implemented:

BeefyZapBiswapLPVaultOperator.sol:

operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:51:    ) external payable returns (uint256[] memory amounts, address[] memory tokens) {

YearnCurveVaultOperator.sol:

operators/Yearn/YearnCurveVaultOperator.sol:69:    ) external payable returns (uint256[] memory amounts, address[] memory tokens) {
operators/Yearn/YearnCurveVaultOperator.sol:120:    ) external payable returns (uint256[] memory amounts, address[] memory tokens) {
operators/Yearn/YearnCurveVaultOperator.sol:163:    ) external payable returns (uint256[] memory amounts, address[] memory tokens) {
operators/Yearn/YearnCurveVaultOperator.sol:211:    ) external payable returns (uint256[] memory amounts, address[] memory tokens) {
operators/Yearn/YearnCurveVaultOperator.sol:259:    ) external payable returns (uint256[] memory amounts, address[] memory tokens) {

6. A magic number should be documented and explained. Use a `constant` instead

Similar issue in the past: here

1:

operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:240:            1,
operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:251:            1,
operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:252:            1,
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:240:            1,
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:251:            1,
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:252:            1,

10000:

NestedFactory.sol:378:        feesAmount = (amountSpent * entryFees) / 10000; // Entry Fees
NestedFactory.sol:443:            feesAmount = (amountBought * (_toReserve ? entryFees : exitFees)) / 10000;

I suggest using constant variables as this would make the code more maintainable and readable while costing nothing gas-wise (constants are replaced by their value at compile-time).

Non-Critical Issues

1. It's better to emit after all processing is done

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L374-L378 :

contracts/governance/TimelockControllerEmergency.sol:
  374      function updateDelay(uint256 newDelay) external virtual {
  375          require(msg.sender == address(this), "TimelockController: caller must be timelock");
  376:         emit MinDelayChange(_minDelay, newDelay);
  377          _minDelay = newDelay;
  378      }

2. Typos

datas vs data

abstracts/MixinOperatorResolver.sol:81:    /// @dev Build the calldata (with safe datas) and call the Operator

setted vs set

- abstracts/OwnableProxyDelegation.sol:17:    /// @dev True if the owner is setted
+ abstracts/OwnableProxyDelegation.sol:17:    /// @dev True if the owner is set

liquitiy vs liquidity

libraries/StakingLPVaultHelpers.sol:21:    /// @param pool The Curve pool to add liquitiy in
libraries/StakingLPVaultHelpers.sol:52:    /// @param pool The Curve pool to add liquitiy in
libraries/StakingLPVaultHelpers.sol:85:    /// @param pool The Curve pool to remove liquitiy from
libraries/StakingLPVaultHelpers.sol:115:    /// @param pool The Curve pool to remove liquitiy from

WITHDRAWED vs WITHDREW or WITHDRAWN

operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:108:        require(vaultAmount == amount, "BLVO: INVALID_AMOUNT_WITHDRAWED");
operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:108:        require(vaultAmount == amount, "BLVO: INVALID_AMOUNT_WITHDRAWED");
operators/Beefy/BeefyVaultOperator.sol:95:        require(vaultAmount == amount, "BVO: INVALID_AMOUNT_WITHDRAWED");
NestedFactory.sol:51:    /// @dev Fees when funds are withdrawed
NestedFactory.sol:639:    /// @return The withdrawed amount from the reserve

dont vs don't

NestedFactory.sol:477:    /// @dev Call the operator to submit the order but dont stop if the call to the operator fail.

transfered vs transferred

NestedFactory.sol:534:    /// @return Token transfered (in case of ETH)

3. Adding a `return` statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

contracts/governance/TimelockControllerEmergency.sol:
  119:     function isOperation(bytes32 id) public view virtual returns (bool pending) {
  126:     function isOperationPending(bytes32 id) public view virtual returns (bool pending) {
  133:     function isOperationReady(bytes32 id) public view virtual returns (bool ready) {
  141:     function isOperationDone(bytes32 id) public view virtual returns (bool done) {
  149:     function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) {
  158:     function getMinDelay() public view virtual returns (uint256 duration) {

contracts/libraries/CurveHelpers/CurveHelpers.sol:
  21:     ) internal view returns (uint256[2] memory amounts) {
  41:     ) internal view returns (uint256[3] memory amounts) {
  61:     ) internal view returns (uint256[4] memory amounts) {
  85:     ) internal returns (bool success) {

4. `public` functions not called by the contract should be declared `external` instead

governance/OwnerProxy.sol:16:    function execute(address _target, bytes memory _data) public payable onlyOwner returns (bytes memory response) {

The text was updated successfully, but these errors were encountered:

Yashiru · 2022-06-22T15:49:41Z

It's better to emit after all processing is done (Confirmed)

Quality assurance confirmed

obatirou · 2022-06-23T15:25:33Z

3. OwnableProxyDelegation.initialize() is front-runnable in the solution (disputed)

Already surfaced in previous audit

obatirou · 2022-06-24T08:17:23Z

5. Funds can be locked (disputed)

They are not supposed to be called outside of a delegateCall context

obatirou · 2022-06-24T09:37:20Z

1. Known vulnerabilities exist in currently used @openzeppelin/contracts version (duplicate)

Duplicate in QA report #73

obatirou · 2022-06-24T09:40:12Z

4. public functions not called by the contract should be declared external instead (duplicate)

Duplicate in QA report #76

Yashiru · 2022-06-24T13:19:55Z

6. A magic number should be documented and explained. Use a constant instead (Duplicated)

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

obatirou · 2022-06-24T13:30:45Z

3. Adding a return statement when the function defines a named return variable, is redundant (confirmed)

Confirmed

Missed occurrences found in QA report #64

TimelockControllerEmergency.sol, hashOperation
TimelockControllerEmergency.sol, hashOperationBatch

Yashiru · 2022-06-24T14:11:10Z

2. Typos (Duplicated)

Duplicated of #45 at Typos

Yashiru · 2022-06-24T14:48:45Z

2. Missing address(0) checks (Confirmed)

Quality assurance confirmed.

Missing occurrences:

TimelockControllerEmergency.sol#L295 found in Missing zero address check could lead to an executeEmergency() callable by anyone #17
ZeroExOperator.sol#L16 found in QA Report #30
BeefyVaultOperator.sol#L37 found in QA Report #30
BeefyVaultOperator.sol#L18 found in QA Report #40
BeefyZapBiswapLPVaultOperator.sol#L28 found in QA Report #40
NestedFactory.sol#L257 found in QA Report #40
OwnableProxyDelegation.sol#L65 found in QA Report #76

jack-the-pug · 2022-07-31T15:03:35Z

L-1: Known vulnerabilities exist in currently used @openzeppelin/contracts version

Valid, upgrading is suggested.

L-2: Missing address(0) checks

Non-critical.

L-3: OwnableProxyDelegation.initialize() is front-runnable in the solution

Valid and surfaced in previous audit.

L-4: Use a constant instead of duplicating the same string

Non-critical. Prefer not making changes.

L-5: Funds can be locked

Invalid.

L-6: A magic number should be documented and explained. Use a constant instead

Valid, best practices, make changes when you see fit.

N-1: It's better to emit after all processing is done

The emit is done earlier to save gas, no?

N-2: Typos

Valid.

N-3: Adding a return statement when the function defines a named return variable, is redundant

Non-critical. Prefer not making changes.

N-4: public functions not called by the contract should be declared external instead

Valid, but no need to make changes.

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 18, 2022

code423n4 added a commit that referenced this issue Jun 18, 2022

Meera issue #61

cad9b98

obatirou self-assigned this Jun 21, 2022

This was referenced Jun 24, 2022

QA Report #64

Closed

QA Report #76

Open

Yashiru mentioned this issue Jun 24, 2022

QA Report #45

Open

This was referenced Jun 24, 2022

QA Report #11

Open

QA Report #3

Open

QA Report #13

Closed

Missing zero address check could lead to an executeEmergency() callable by anyone #17

Closed

QA Report #30

Closed

QA Report #40

Open

QA Report #84

Open

Yashiru added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jun 27, 2022

jack-the-pug added the valid label Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #61

QA Report #61

code423n4 commented Jun 18, 2022

Yashiru commented Jun 22, 2022

obatirou commented Jun 23, 2022

obatirou commented Jun 24, 2022

obatirou commented Jun 24, 2022 •

edited

Loading

obatirou commented Jun 24, 2022

Yashiru commented Jun 24, 2022

obatirou commented Jun 24, 2022

Yashiru commented Jun 24, 2022

Yashiru commented Jun 24, 2022 •

edited

Loading

jack-the-pug commented Jul 31, 2022

QA Report #61

QA Report #61

Comments

code423n4 commented Jun 18, 2022

Low Risk Issues

1. Known vulnerabilities exist in currently used @openzeppelin/contracts version

2. Missing address(0) checks

3. OwnableProxyDelegation.initialize() is front-runnable in the solution

4. Use a constant instead of duplicating the same string

5. Funds can be locked

6. A magic number should be documented and explained. Use a constant instead

Non-Critical Issues

1. It's better to emit after all processing is done

2. Typos

3. Adding a return statement when the function defines a named return variable, is redundant

4. public functions not called by the contract should be declared external instead

Yashiru commented Jun 22, 2022

It's better to emit after all processing is done (Confirmed)

obatirou commented Jun 23, 2022

3. OwnableProxyDelegation.initialize() is front-runnable in the solution (disputed)

obatirou commented Jun 24, 2022

5. Funds can be locked (disputed)

obatirou commented Jun 24, 2022 • edited Loading

1. Known vulnerabilities exist in currently used @openzeppelin/contracts version (duplicate)

obatirou commented Jun 24, 2022

4. public functions not called by the contract should be declared external instead (duplicate)

Yashiru commented Jun 24, 2022

6. A magic number should be documented and explained. Use a constant instead (Duplicated)

obatirou commented Jun 24, 2022

3. Adding a return statement when the function defines a named return variable, is redundant (confirmed)

Yashiru commented Jun 24, 2022

2. Typos (Duplicated)

Yashiru commented Jun 24, 2022 • edited Loading

2. Missing address(0) checks (Confirmed)

jack-the-pug commented Jul 31, 2022

L-1: Known vulnerabilities exist in currently used @openzeppelin/contracts version

L-2: Missing address(0) checks

L-3: OwnableProxyDelegation.initialize() is front-runnable in the solution

L-4: Use a constant instead of duplicating the same string

L-5: Funds can be locked

L-6: A magic number should be documented and explained. Use a constant instead

N-1: It's better to emit after all processing is done

N-2: Typos

N-3: Adding a return statement when the function defines a named return variable, is redundant

N-4: public functions not called by the contract should be declared external instead

1. Known vulnerabilities exist in currently used `@openzeppelin/contracts` version

3. `OwnableProxyDelegation.initialize()` is front-runnable in the solution

4. Use a `constant` instead of duplicating the same string

6. A magic number should be documented and explained. Use a `constant` instead

3. Adding a `return` statement when the function defines a named return variable, is redundant

4. `public` functions not called by the contract should be declared `external` instead

obatirou commented Jun 24, 2022 •

edited

Loading

Yashiru commented Jun 24, 2022 •

edited

Loading