QA Report

[code423n4]

• Function releaseTokens uses weth, not eth when comparing against a native asset. if the token address is weth, it unwraps and sends the native asset to the user:

  if (address(_tokens[i]) == weth)
  IWETH(weth).withdraw(amount);
  (bool success, ) = _msgSender().call{ value: amount }("");
  require(success, "FS: ETH_TRANFER_ERROR");

  Releasing weth token should be left as a valid option if the user prefers wrapped ERC20 eth, and I think for this native purpose there is a not used storage variable named ETH:

  address private constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

  Based on my assumptions, the intention was:

  if (address(_tokens[i]) == ETH)
  ...

  or if you do not want to implement this change, then at least remove this unused variable to save some gas. However, the issue is small, because the user can always retrieve weth by using another function named releaseTokensNoETH.

• This was mentioned in the Red4Sec audit (NFSC09), but it wasn't fixed here: OwnableProxyDelegation is Context, but still uses msg.sender, not _msgSender():

  require(StorageSlot.getAddressSlot(_ADMIN_SLOT).value == msg.sender, "OFP: FORBIDDEN");

• function rebuildCache() in MixinOperatorResolver does not delete removed operators from operatorCache. resolverOperatorsRequired return current active operators, so it will not contain removed operators, e.g. operator was removed by calling removeOperator in the factory, then rebuildCache is called, and the cache will still contain this removed operator, and it will be possible to callOperator on this operator.

• Consider introducing an upper limit for _timestamp in updateLockTimestamp, e.g. max 1 year from current block timestamp, otherwise it may be possible to accidentally lock the token forever.

• if removeFactory has this check:

  require(supportedFactories[_factory], "OFH: NOT_SUPPORTED"); 

  then I think addFactory should have an analogous check:

  require(!supportedFactories[_factory], "OFH: ALREADY_SUPPORTED"); 

• The revert message is a bit missleading here:

  require(assetTokensLength > 1, "NF: UNALLOWED_EMPTY_PORTFOLIO");

• NestedFactory has a function unlockTokens that lets admin rescue any ERC20 token. Consider also adding support for rescuing the native asset (e.g. ETH).

[maximebrugel]

« Function releaseTokens uses weth, not eth (…) » (Duplicated)

https://github.com/code-423n4/2022-02-nested-findings/issues/15

« OwnableProxyDelegation is Context, but still uses msg.sender, not _msgSender() (…) » (Acknowledged)

No meta-transaction support for this admin function.

« Function rebuildCache() in MixinOperatorResolver does not delete removed operators from operatorCache (…) » ( Duplicated)

https://github.com/code-423n4/2022-02-nested-findings/issues/18

« Consider introducing an upper limit for _timestamp in updateLockTimestamp (…) » (Acknowledged)

We are not sure about an upper limit to set.

« (…) addFactory should have an analogous check » (Disputed)

No need for a require as long as supportedFactories[_factory] = true does not disrupt the protocol state.

« The revert message is a bit misleading here » (Disputed)

I don’t really know what is misleading. You can’t withdraw the last token and keep an empty portfolio.

« adding support for rescuing the native asset » (Acknowledged)

We will fix this issue by adding a require in the receive function. Also, the user can't send more ETH than needed with the _checkMsgValue function.

[harleythedogC4]

My personal judgements:

1. "Function releaseTokens uses weth". This is a gas optimization. Will keep it in mind when scoring #67. For here, Invalid.
2. "OwnableProxyDelegation is Context". Valid and very-low-critical.
3. "Function rebuildCache() in MixinOperatorResolver does not delete removed operators from operatorCache". This has been upgraded to medium severity in #77. Will not contribute to QA score.
4. "Consider introducing an upper limit for _timestamp in updateLockTimestamp". I think this is a good idea. Valid and low-critical.
5. "addFactory should have an analogous check". Just a consistency suggestion, valid and non-critical.
6. "The revert message is a bit missleading here". Warden doesn't explain enough why it is misleading. Invalid.
7. "Consider also adding support for rescuing the native asset". Valid and low-critical.

[harleythedogC4]

Now, here is the methodology I used for calculating a score for each QA report. I first assigned each submission to be either non-critical (1 point), very-low-critical (5 points) or low-critical (10 points), depending on how severe/useful the issue is. The score of a QA report is the sum of these points, divided by the maximum number of points achieved by a QA report. This maximum number was 26 points, achieved by https://github.com/code-423n4/2022-02-nested-findings/issues/66.

The number of points achieved by this report is 26 points. Thus the final score of this QA report is (26/26)*100 = 100.