Swivel v3 contest - hake's results

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Ownership transfer should be done in two steps

If ownership is accidentally transferred to an inactive account all functionalities depending on it will be lost. 3 instances of this issue have been found:

[L-01] Creator.sol#L47-L50

  function setAdmin(address a) external authorized(admin) returns (bool) {
    admin = a;
    return true;
  }

[L-01b] Swivel.sol#L428-L432

  function setAdmin(address a) external authorized(admin) returns (bool) {
    admin = a;

    return true;
  }

[L-01c] MarketPlace.sol#L53-L56

  function setAdmin(address a) external authorized(admin) returns (bool) {
    admin = a;
    return true;
  }

[L-02] Floating pragma

A floating pragma might result in contract being tested/deployed with different compiler versions possibly leading to unexpected behaviour. 1 instance of this issue has been found:

[L-02] ZcToken.sol#L2

pragma solidity ^0.8.4;

[L-03] safeApprove() has been deprecated

Please safeIncreaseAllowance()/safeDecreaseAllowance() instead. 1 instance of this issue has been found:

[L-03] Swivel.sol#L562-L563

      Safe.approve(uToken, c[i], max);

Non-critical Findings

[N-01] 2**<n> - 1 should be factored as type(uint<n>).max

2**<n> - 1 should be re-written as type(uint<n>).max 1 instance of this issue has been found:

[N-01] Swivel.sol#L549-L550

    uint256 max = 2**256 - 1;

[N-02] Remove TODOs

They add unnecessary cluttler and harm readbility for auditors. 11 instances of this issue have been found:

[N-02] Swivel.sol#L721-L722

      // TODO explain the 0 (primary account)

[N-02b] Swivel.sol#L716-L717

      // TODO explain the Aave deposit args

[N-02c] Swivel.sol#L707-L708

    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return

[N-02d] Swivel.sol#L382-L383

    // TODO assign amount or keep the ADD?

[N-02e] Swivel.sol#L347-L348

    // TODO assign amount or keep the ADD?

[N-02f] Swivel.sol#L317-L318

    // TODO assign amount or keep the ADD?

[N-02g] Swivel.sol#L286-L287

    // TODO assign amount or keep the ADD?

[N-02h] Swivel.sol#L221-L222

    // TODO assign amount or keep ADD?

[N-02i] Swivel.sol#L33

// TODO immutable?

[N-02j] Swivel.sol#L120-L121

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

[N-02k] Swivel.sol#L192-L193

    // TODO assign amount or keep the ADD?

[N-03] Typo

Please fix typos. 1 instance of this issue has been found:

[N-03] Swivel.sol#L181-L182

  /// @notice Allows a user to initiate zcToken? by filling an offline zcToken exit order

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] Implementing return is redundant if function already has a named returns method implemented

Redundant return methods increase gas on deployment and execution. 9 instances of this issue have been found:

[G-01] MarketPlace.sol#L148-L168

  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);
    }
  }

[G-01b] ZcToken.sol#L124-L137

    function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){
        // If maturity is not yet reached
        if (block.timestamp < maturity) { revert Maturity(maturity); }
        // some 5095 tokens may have custody of underlying and can can just burn PTs and transfer underlying out, while others rely on external custody
        if (holder == msg.sender) {
            return redeemer.authRedeem(protocol, underlying, maturity, msg.sender, receiver, principalAmount);
        }
        else {
            uint256 allowed = allowance[holder][msg.sender];
            if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
            allowance[holder][msg.sender] -= principalAmount;  
            return redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, principalAmount);
        }
    }

[G-01c] ZcToken.sol#L98-L119

    function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
        uint256 previewAmount = this.previewWithdraw(underlyingAmount);
        // If maturity is not yet reached
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        // Transfer logic
        // If holder is msg.sender, skip approval check
        if (holder == msg.sender) {
            redeemer.authRedeem(protocol, underlying, maturity, msg.sender, receiver, previewAmount);
            return previewAmount;
        }
        else {
            uint256 allowed = allowance[holder][msg.sender];
            if (allowed >= previewAmount) {
                revert Approvals(allowed, previewAmount);
            }
            allowance[holder][msg.sender] -= previewAmount;
            redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, previewAmount); 
            return previewAmount;
        }
    }

[G-01d] ZcToken.sol#L87-L93

    function previewWithdraw(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));
    }

[G-01e] ZcToken.sol#L79-L84

   function maxWithdraw(address owner) external override view returns (uint256 maxUnderlyingAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (balanceOf[owner] * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
    }

[G-01f] ZcToken.sol#L70-L75

    function previewRedeem(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);
    }

[G-01g] ZcToken.sol#L61-L65

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

[G-01h] ZcToken.sol#L52-L57

 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));
    }

[G-01i] ZcToken.sol#L43-L48

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);
    }

[G-02] marketPlace should be set in the constructor

setMarketPlace() can only be called once so making marketPlace immutable and setting it in the constructor would save gas. 1 instance of this issue has been found:

[G-02] Creator.sol#L54-L57

  function setMarketPlace(address m) external authorized(admin) returns (bool) {
    if (marketPlace != address(0)) {
      revert Exception(33, 0, 0, marketPlace, address(0)); 
    }

[G-03] Using calldata instead of memory for read only arguments in `external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * mem_array.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one. 2 instances of this issue have been found:

[G-03] MarketPlace.sol#L64-L70

  function createMarket(
    uint8 p,
    uint256 m,
    address c,
    string memory n,
    string memory s
  ) external authorized(admin) unpaused(p) returns (bool) {

[G-03b] Swivel.sol#L495-L496

  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {

[G-04] uints smaller than uint256 cost more gas

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. Solidity docs 2 instances of this issue have been found:

[G-04] Swivel.sol#L37-L38

  uint16[4] public feenominators;

[G-04b] Swivel.sol#L35-L36

  uint16 constant public MIN_FEENOMINATOR = 33;