QA Report

[code423n4]

Low Risk Findings Overview

	Finding	Instances
[L-01]	Ownership transfer should be done in two steps	3
[L-02]	Floating pragma	1
[L-03]	safeApprove() has been deprecated	1

Non-critical Findings Overview

	Finding	Instances
[N-01]	2**<n> - 1 should be factored as type(uint<n>).max	1
[N-02]	Remove TODOs	11
[N-03]	Typo	1

QA overview per contract

Contract	Total Instances	Total Findings	Low Findings	Low Instances	NC Findings	NC Instances
Swivel.sol	15	5	2	2	3	13
Creator.sol	1	1	1	1	0	0
MarketPlace.sol	1	1	1	1	0	0
ZcToken.sol	1	1	1	1	0	0

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 - 1 should be re-written as type(uint).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

[robrobbins]

the soulmate safe that we use does not share the change to ...increaseAllowance over approve

https://github.com/transmissions11/solmate/blob/v7/src/utils/SafeTransferLib.sol