Nested Finance contest - pauliax's results

This issue has been created to upgrade a QA report submission to a medium severity finding. From pauliax:

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.

• 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).

• function transfer in NestedReserve is never used and can only be called by the factory (onlyFactory), so consider removing it because I think the factory uses a withdraw function from the Reserve.

• Currently never used:

  function setReserve onlyFactory

You can remove it to save some gas, or leave it if it was intended for future use with other factories.

• functions that add or remove operators or shareholders iterate over the whole array, so you can consider using EnumerableSet to store them: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol

• Could just use msg.sender and do not call an owner() function here:

    function unlockTokens(IERC20 _token) external override onlyOwner {
        ...
        _token.safeTransfer(owner(), amount);

• There are several functions that call _checkMsgValue. This function is quite expensive as it iterates over all the _batchedOrders and is only relevant when the inputToken is ETH. Later the callers will have to iterate over all the _batchedOrders again anyway, so I think this function should be refactored to significantly reduce gas. My suggestion: because processInputOrders and processInputAndOutputOrders both call _processInputOrders, the logic from _checkMsgValue could be moved to _processInputOrders. function create then can be refactored to re-use _processInputOrders. I see 2 discrepancies here: _fromReserve is always false when _submitInOrders is called from create (could be solved if _processInputOrders takes extra parameter), and _processInputOrders has this extra line:

  require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

but this could be solved if you first mint the NFT and then invoke _processInputOrders from create.

• function withdraw calls nestedRecords twice:

 uint256 assetTokensLength = nestedRecords.getAssetTokensLength(_nftId);
 ...
  address token = nestedRecords.getAssetTokens(_nftId)[_tokenIndex];

I think it could just substitute these links by first fetching all the tokens, and then calculating the length itself instead of making 2 external calls for pretty much the same data.

• Could use 'unchecked' maths here, as underflow is not possible:

   if (_amountToSpend > amounts[1]) {
      IERC20(_inputToken).safeTransfer(_msgSender(), _amountToSpend - amounts[1]);
    }