Gas Optimizations

[code423n4]

Optimization

1. (wfCash) Unnecessary safeTrasnfer on WETH

in wfCashLogic._sendTokensToReceiver, it uses IERC20(address(WETH)).safeTransfer on WETH. But safeERC20 library is only intended to take a care of non standard ERC20 behaviour, in case of WETH, it doesn’t need to be used.

Same in _mintInternal which it calls safeTrasnferFrom on weth.

2. (wfCash) _withdrawCashToAccount doesn’t need to check balance to get tokensTransfered

The following is how _withdarwCashToAccount is programmed: check balanceBefore first, pass it into _sendTokenToReceiver, which will check the balance again and transfer the difference.

function _withdrawCashToAccount(
      uint16 currencyId,
      address receiver,
      uint88 assetInternalCashClaim,
      bool toUnderlying
  ) private returns (uint256 tokensTransferred) {
      (IERC20 token, bool isETH) = getToken(toUnderlying);
      uint256 balanceBefore = isETH ? address(this).balance : token.balanceOf(address(this));

      NotionalV2.withdraw(currencyId, assetInternalCashClaim, toUnderlying);

      tokensTransferred = _sendTokensToReceiver(token, receiver, isETH, balanceBefore);
  }

This is not optimal for _withdrawCashToAccount, because the amount will already be returned by NotionalV2.withdraw. It is better not to use the _sendTokensToReceiver function to save 2 hot SLOAD and external calls. Consider doing 1 of the following:

1. refactor _sendTokensToReceiver to either take the amount directly or calculate the difference, with a flag
2. don’t use _sendTokenToReceiver for this internal function and do it manually:

function _withdrawCashToAccount(
      uint16 currencyId,
      address receiver,
      uint88 assetInternalCashClaim,
      bool toUnderlying
  ) private returns (uint256 tokensTransferred) {
      (IERC20 token, bool isETH) = getToken(toUnderlying);

      tokensTransferred = NotionalV2.withdraw(currencyId, assetInternalCashClaim, toUnderlying);

            if (isETH) {
          WETH.deposit{value: tokensTransferred}();
          IERC20(address(WETH)).transfer(receiver, tokensTransferred);
      } else {
          token.safeTransfer(receiver, tokensTransferred);
      }
  }

3.(wfCash) Amount of JUMP can be reduced by using function _burn directly.

in wfCashLogic.burn, it calls the OZ implementation burn, which then calls an overridden function _burn. It can be optimised by calling _burn directly, this should’t hurt readability, and considering it’s a path that all redeem action will go through, it’s worth the extra 2 parameters

function redeem(uint256 amount, RedeemOpts memory opts) public override {
    bytes memory data = abi.encode(opts);
    _burn(_msgSender(), amount, data, "");
        // instead of OZ.burn => this._burn
  }

4.(wfCash) convertToShares should use cached variable supply instead of totalSupply

In converToShares function, the total supply is already stored in variable supply , there is no need to query again. I believe this is a small typo

/** @dev See {IERC4626-convertToShares} */
  function convertToShares(uint256 assets) public view override returns (uint256 shares) {
      uint256 supply = totalSupply();
      if (supply == 0) {
          // Scales assets by the value of a single unit of fCash
          uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
          return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
      }
      // return (assets * totalSupply) / totalAssets(); <= current implementation
      return (assets * totalSupply) / totalAssets(); // <= do this
  }

5. (wfCash) Use unchecked in DateTime library

In DateTime.getMarketIndex, i is checked whether it will overflow everytime i++ is calculated.

better skip the check for i:

for (uint256 i = 1; i <= maxMarketIndex;) {
    uint256 marketMaturity = tRef + DateTime.getTradedMarket(i);
    // If market matches then is not idiosyncratic
    if (marketMaturity == maturity) return (i, false);
    // Returns the market that is immediately greater than the maturity
    if (marketMaturity > maturity) return (i, true);
        unchecked { i ++ }
}

In function DateTime.getBitNumFromMaturity, all of the overflow check is already done prior to calculation, the whole function can be wrapped with uncheck to save gas.

Same apply to getReferenceTime and getTimeUTC0

6. (wfCash & TradeModule) Use != 0 instead of >0.

in DateTime.getMarketIndex , and 2 other functions in wfCashBase and wfCashERC4626, the code is comparing number with >0, it’s more efficient to use ≠ 0 if it’s not optimised by via-ir.

// in DateTime.getMarketIndex & wfCashBase.initialize
require(cashGroup.maxMarketIndex != 0, "Invalid currency");

// in _getMaturedValue
require(underlyingExternal != 0, "Must Settle");

in NotionalTradeModule._redeemMaturedPositions the same trick can be applied here:

for(uint256 i = 0; i < positionsLength; i++) {
            // Check that the given position is an equity position
            if(positions[i].unit > 0) { // this line can be changed to != 0
                address component = positions[i].component;
                if(_isWrappedFCash(component)) {
                    ...
                }
            }
        }

7. (TradeModule) Use returned variable from fCash.getToken to determine if receiveToken is eth.

The function currently determines if receiveToken is ETH by comparing the address as follow:

if(fCashPosition.hasMatured()) {
    (IERC20 receiveToken, ) = fCashPosition.getToken(toUnderlying);
    if(address(receiveToken) == ETH_ADDRESS) {
        receiveToken = weth;
    }
    uint256 fCashBalance = fCashPosition.balanceOf(address(_setToken));
    _redeemFCashPosition(_setToken, fCashPosition, receiveToken, fCashBalance, 0);
}

But the bool isETH can be used directly to remove this comparison.

if(fCashPosition.hasMatured()) {
      (IERC20 receiveToken, bool isEth) = fCashPosition.getToken(toUnderlying);
      if(isEth) {
          receiveToken = weth;
      }
  }

This saves average of 6 gas 😂

8. (TradeModule) Optimise view function getFCashPosition

The current _getFCashPositions function takes 2 loops to return array of fCash positions, but in each loop, the call isWrappedFCash is duplicated. This costs a lot of gas to use because it involve external calls.

it can be optimised with a local uint variable, recording which i has been checked as component.

function _getFCashPositions(ISetToken _setToken)
    internal
    view
    returns(address[] memory fCashPositions)
    {
        ISetToken.Position[] memory positions = _setToken.getPositions();
        uint positionsLength = positions.length;
        uint numFCashPositions;

                // add this variable;
        uint256 isFCashBitMap;

        for(uint256 i = 0; i < positionsLength; i++) {
            // Check that the given position is an equity position
            if(positions[i].unit > 0) {
                address component = positions[i].component;
                if(_isWrappedFCash(component)) {
                    numFCashPositions++;

                    // use the ith bit to indicate if positions[i] is fCash
                    isFCashBitMap += (uint256(1) << i);
                }
            }
        }

        fCashPositions = new address[](numFCashPositions);

        uint j;
        for(uint256 i = 0; i < positionsLength; i++) {
            // if the last bit after >> i is 1, it means that it's fCash
            if ((isFCashBitMap >> i) %2 != 0) {
                fCashPositions[j] = positions[i].component;
                j++;
            }
        }

[jeffywu]

This is a well written report.