Swivel v3 contest - c3phas's results

QA

Natspec is incomplete

File: Creator.sol line 39

Missing @return

  /// @param d Decimals of the new market zcToken
  function create (
    uint8 p,
    address u,
    uint256 m,
    address c,
    address sw,
    string memory n,
    string memory s,
    uint8 d
  ) external authorized(marketPlace) returns (address, address) {

File: Creator.sol line 46-50

Missing @return

  /// @param a Address of a new admin
  function setAdmin(address a) external authorized(admin) returns (bool) {
    admin = a;
    return true;
  }

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File: ZcToken.sol line 2

pragma solidity ^0.8.4;

Unused named return

Using both named returns and a return statement isn’t necessary in a function.To improve code quality, consider using only one of those.

File: MarketPlace.sol line 148-168

  function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken) returns (uint256 underlyingAmount) {
    Market memory market = markets[p][u][m];
    // if the market has not matured, mature it...
    if (market.maturityRate == 0) {
      if (!matureMarket(p, u, m)) { revert Exception(30, 0, 0, address(0), address(0)); }
      if (!IZcToken(market.zcToken).burn(f, a)) { revert Exception(29, 0, 0, address(0), address(0)); }
      ISwivel(swivel).authRedeem(p, u, market.cTokenAddr, t, a);
      return (a);
    } else {

      if (!IZcToken(market.zcToken).burn(f, a)) { revert Exception(29, 0, 0, address(0), address(0)); }
      uint256 amount = calculateReturn(p, u, m, a);
      ISwivel(swivel).authRedeem(p, u, market.cTokenAddr, t, amount);
      return (amount);
    }
  }

File: ZcToken.sol line 43-48

    function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
    }

File: ZcToken.sol line 52-57

    function convertToPrincipal(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
    }

File: ZcToken.sol line 61-66

    function maxRedeem(address owner) external override view returns (uint256 maxPrincipalAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (balanceOf[owner]);
    }

File: ZcToken.sol line 70-75 File: ZcToken.sol line 79-84 File: ZcToken.sol line 88-93

Open TODOs

File: Swivel.sol line 33

  address public aaveAddr; // TODO immutable?

File: Swivel.sol line 120

    // TODO cheaper to assign amount here or keep the ADD?

File: Swivel.sol line 157

    // TODO assign amount or keep the ADD?

File: Swivel.sol line 192

    // TODO assign amount or keep the ADD?

File: Swivel.sol line 221

    // TODO assign amount or keep ADD?

File: Swivel.sol line 707-708

    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?

File: Swivel.sol line 286 File: Swivel.sol line 317 File: Swivel.sol line 347 File: Swivel.sol line 716 File: Swivel.sol line 721 File: Swivel.sol line 748

Missing checks for address(0x0) when assigning values to address state variables

File: VaultTracker.sol line 35

    cTokenAddr = c;

FINDINGS

Use immutable on variables that are only set in the constructor and never after

File: Swivel.sol line 33

  address public aaveAddr; // TODO immutable?

The above address is set in the constructor and never set again in the contract

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource

File:ZcToken.sol line 115

            allowance[holder][msg.sender] -= previewAmount;

The above line cannot underflow due to the check on line 112 which ensures that allowance[holder][msg.sender] is greater than previewAmount before performing the arithmetic operation

The above can be modified to:

unchecked {
        allowance[holder][msg.sender] -= previewAmount;
}

File:ZcToken.sol line 134

            allowance[holder][msg.sender] -= principalAmount;  

The above line cannot underflow due to the check on line 133 which ensures that allowance[holder][msg.sender] is greater than principalAmount before performing the arithmetic operation

Cache storage values in memory to minimize SLOADs

The code can be optimized by minimizing the number of SLOADs. SLOADs are expensive 100 gas compared to MLOADs/MSTOREs(3gas) Storage value should get cached in memory

NB: Some functions have been truncated where necessary to just show affected parts of the code

vaultTracker.sol.addNotional(): maturityRate should be cached in memory

File: VaultTracker.sol line 59,60

  function addNotional(address o, uint256 a) external authorized(admin) returns (bool) {
    uint256 exchangeRate = Compounding.exchangeRate(protocol, cTokenAddr);

    Vault memory vlt = vaults[o];
    if (vlt.notional > 0) {
      uint256 yield;

      // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
      // otherwise, calculate marginal exchange rate between current and previous exchange rate.
      if (maturityRate > 0) { // Calculate marginal interest
        yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
      } else {
        yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
      }

    return true;
  }

In the above function maturityRate should be cached in memory to minimize the number of SLOADs

SLOAD 1: line 59 SLOAD 2: line 60

vaultTracker.sol.addNotional(): maturityRate should be cached in memory

File: VaultTracker.sol line 93,94

  function removeNotional(address o, uint256 a) external authorized(admin) returns (bool) {

    // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
    // otherwise, calculate marginal exchange rate between current and previous exchange rate.
    uint256 yield;
    if (maturityRate > 0) { // Calculate marginal interest
      yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
    } else {
      // calculate marginal interest
      yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
    }

    return true;
  }

In the above function maturityRate should be cached in memory to minimize the number of SLOADs

SLOAD 1: line 93 SLOAD 2: line 94

vaultTracker.sol.redeemInterest(): maturityRate should be cached in memory

File: VaultTracker.sol line 123,124

  function redeemInterest(address o) external authorized(admin) returns (uint256) {

    // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
    // otherwise, calculate marginal exchange rate between current and previous exchange rate.
    uint256 yield;
    if (maturityRate > 0) { // Calculate marginal interest
      yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
    } else {
      // calculate marginal interest
      yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
    }

    // return adds marginal interest to previously accrued redeemable interest
    return (redeemable + interest);
  }

In the above function maturityRate should be cached in memory to minimize the number of SLOADs

SLOAD 1: line 123 SLOAD 2: line 124

vaultTracker.sol.transferNotionalFrom(): maturityRate should be cached in memory

File: VaultTracker.sol line 165,167,184,186

  function transferNotionalFrom(address f, address t, uint256 a) external authorized(admin) returns (bool) {
    if (f == t) { revert Exception(32, 0, 0, f, t); }
    uint256 yield;
    if (maturityRate > 0) { 
      // calculate marginal interest
      yield = ((maturityRate * 1e26) / from.exchangeRate) - 1e26;
    } else {
      yield = ((exchangeRate * 1e26) / from.exchangeRate) - 1e26;
    }
		...
    vaults[f] = from;
		...
    // transfer notional to address "t", calculate interest if necessary
    if (to.notional > 0) {
      // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
      // otherwise, calculate marginal exchange rate between current and previous exchange rate.
      if (maturityRate > 0) { 
        // calculate marginal interest
        yield = ((maturityRate * 1e26) / to.exchangeRate) - 1e26;
      } else {
        yield = ((exchangeRate * 1e26) / to.exchangeRate) - 1e26;
      }

    return true;
  }

In the above function maturityRate should be cached in memory to minimize the number of SLOADs(4 SLOADS)

SLOAD 1: line 165 SLOAD 2: line 167 SLOAD 3: line 184 SLOAD 4: line 186

vaultTracker.sol.transferNotionalFee(): maturityRate should be cached in memory ()

File: VaultTracker.sol line 222,224

  function transferNotionalFee(address f, uint256 a) external authorized(admin) returns(bool) {
    Vault memory oVault = vaults[f];

    uint256 yield;
    if (sVault.exchangeRate != exchangeRate) {
      // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
      // otherwise, calculate marginal exchange rate between current and previous exchange rate.
      if (maturityRate > 0) { 
        // calculate marginal interest
          yield = ((maturityRate * 1e26) / sVault.exchangeRate) - 1e26;
      } else {
          yield = ((exchangeRate * 1e26) / sVault.exchangeRate) - 1e26;
      }

    return true;
  }

In the above function maturityRate should be cached in memory to minimize the number of SLOADs(4 SLOADS)

SLOAD 1: line 222 SLOAD 2: line 224

MarketPlace.sol.redeemZcToken(): maturityRate should be cached in memory

File: MarketPlace.sol line 180,188

  function redeemZcToken(uint8 p, address u, uint256 m, address t, uint256 a) external authorized(swivel) unpaused(p) returns (uint256) {
    Market memory market = markets[p][u][m];
   // if the market has not matured, mature it and redeem exactly the amount
    if (market.maturityRate == 0) {
      if (!matureMarket(p, u, m)) { revert Exception(30, 0, 0, address(0), address(0)); }
    }
    if (!IZcToken(market.zcToken).burn(t, a)) { revert Exception(29, 0, 0, address(0), address(0)); }
    emit RedeemZcToken(p, u, m, t, a);
    if (market.maturityRate == 0) {
      return a;
    } else { 
      // if the market was already mature the return should include the amount + marginal floating interest generated on Compound since maturity
      return calculateReturn(p, u, m, a);
    }
  }

In the above function maturityRate should be cached in memory to minimize the number of SLOADs

SLOAD 1: line 180 SLOAD 2: line 188

ZcToken.sol.convertToUnderlying(): redeemer should be cached in memory

File: ZcToken.sol line 47

    function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
    }

In the above function redeemer should be cached in memory to minimize the number of SLOADs

It is being read twice from memory in the return statement

ZcToken.sol.convertToUnderlying(): redeemer should be cached in memory ()

File: ZcToken.sol line 56

        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

File: Swivel.sol line 100

      unchecked {i++;}

The above should be modified to:

      unchecked {++i;}

Other instances File: Swivel.sol line 269

      unchecked {i++;}

File: Swivel.sol line 417-419

      unchecked {
        i++;
      }

File: Swivel.sol line 510-512

      unchecked {
        x++;
      }

File: Swivel.sol line 563-565

      unchecked {
        i++;
      }

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

File: Swivel.sol line 25

  string constant public NAME = 'Swivel Finance';

File: Swivel.sol line 26

  string constant public VERSION = '3.0.0';

File: Swivel.sol line 27

  uint256 constant public HOLD = 3 days;

File: Swivel.sol line 35

  uint16 constant public MIN_FEENOMINATOR = 33;

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: VaultTracker.sol line 60 1e26

        yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;

File: VaultTracker.sol line 62 1e26

        yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;

File: VaultTracker.sol line 65 1e26

      uint256 interest = (yield * vlt.notional) / 1e26;

Other instances File: VaultTracker.sol line 94 File: VaultTracker.sol line 97 File: VaultTracker.sol line 100 File: VaultTracker.sol line 124 File: VaultTracker.sol line 127 File: VaultTracker.sol line 130