Swivel v3 contest - rbserver's results

[L-01] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers.

VaultTracker\VaultTracker.sol
  60: yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
  62: yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
  65: uint256 interest = (yield * vlt.notional) / 1e26;
  94: yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
  97: yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
  100: uint256 interest = (yield * vlt.notional) / 1e26;
  124: yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
  127: yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
  130: uint256 interest = (yield * vlt.notional) / 1e26;
  167: yield = ((maturityRate * 1e26) / from.exchangeRate) - 1e26;
  169: yield = ((exchangeRate * 1e26) / from.exchangeRate) - 1e26;
  172: uint256 interest = (yield * from.notional) / 1e26;
  186: yield = ((maturityRate * 1e26) / to.exchangeRate) - 1e26;
  188: yield = ((exchangeRate * 1e26) / to.exchangeRate) - 1e26;
  191: uint256 newVaultInterest = (yield * to.notional) / 1e26;
  224: yield = ((maturityRate * 1e26) / sVault.exchangeRate) - 1e26;
  226: yield = ((exchangeRate * 1e26) / sVault.exchangeRate) - 1e26;
  228: uint256 interest = (yield * sVault.notional) / 1e26;

[L-02] UNRESOLVED TODO COMMENTS

Comments regarding todos indicate that there are unresolved action items for implementation, which need to be addressed before protocol deployment.

Swivel\Swivel.sol
  33: address public aaveAddr; // TODO immutable?
  120: // TODO cheaper to assign amount here or keep the ADD?
  157: // TODO assign amount or keep the ADD?
  192: // TODO assign amount or keep the ADD?
  221: // TODO assign amount or keep ADD?
  286: // TODO assign amount or keep the ADD?
  317: // TODO assign amount or keep the ADD?
  347: // TODO assign amount or keep the ADD?
  382: // TODO assign amount or keep the ADD?
  707: // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
  708: if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
  716: // TODO explain the Aave deposit args
  721: // TODO explain the 0 (primary account)
  740: // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
  741: if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
  748: // TODO explain the withdraw args
  752: // TODO explain the 0

[N-01] REDUNDANT NAMED RETURNS

When a function has unused named returns and used return statement(s), these named returns become redundant. To improve readability and maintainability, named returns need to be used and return statement(s) need to be removed or vice versa.

Marketplace\MarketPlace.sol
  148: function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken) returns (uint256 underlyingAmount) {

Creator\ZcToken.sol
  43: function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
  52: function convertToPrincipal(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
  70: function previewRedeem(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
  79: function maxWithdraw(address owner) external override view returns (uint256 maxUnderlyingAmount){
  88: function previewWithdraw(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
  98: function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
  124: function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){

Tokens\ZcToken.sol
  43: function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
  52: function convertToPrincipal(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
  61: function maxRedeem(address owner) external override view returns (uint256 maxPrincipalAmount){
  70: function previewRedeem(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
  79: function maxWithdraw(address owner) external override view returns (uint256 maxUnderlyingAmount){
  88: function previewWithdraw(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
  98: function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
  124: function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){

[N-02] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs.

Creator\ZcToken.sol
  2,35: pragma solidity ^0.8.4;

Tokens\ZcToken.sol
  2,35: pragma solidity ^0.8.4;

[N-03] MISSING NATSPEC COMMENTS

NatSpec provides rich documentation for code. NatSpec comments are missing for the following code:

Marketplace\MarketPlace.sol
  341: modifier authorized(address a) {
  346: modifier unpaused(uint8 p) {

Creator\ZcToken.sol
  152: modifier onlyAdmin(address a) {

Tokens\ZcToken.sol
  152: modifier onlyAdmin(address a) {

[N-04] @NOTICE PLACEMENT IN NATSPEC COMMENTS

It is a convention to place @notice above @param in NatSpec comments, which is not the case in the following code:

Creator\Creator.sol
  52-53:
    /// @param m Address of the deployed marketPlace contract
    /// @notice We only allow this to be set once
  
Marketplace\MarketPlace.sol
  43-44:
    /// @param s Address of the deployed swivel contract
    /// @notice We only allow this to be set once

[G-01] STATE VARIABLES THAT NEED TO BE LOADED FOR MULTIPLE TIMES IN A FUNCTION CAN BE CACHED IN MEMORY

When state variables need to be loaded for multiple times in a function, caching them in memory can save gas. Please see @audit in the following code for state variables that can be cached.

Creator\Creator.sol
  // @audit marketPlace can be cached for the following code
  55-57:
    if (marketPlace != address(0)) {
      revert Exception(33, 0, 0, marketPlace, address(0)); 
    }

Creator\VaultTracker.sol
  // @audit maturityRate can be cached for the following code
  59-60:
    if (maturityRate > 0) {
      yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
  93-94:
    if (maturityRate > 0) { // Calculate marginal interest
      yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
  123-124:
    if (maturityRate > 0) { // Calculate marginal interest
      yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
  165-167:
    if (maturityRate > 0) {
      // calculate marginal interest
      yield = ((maturityRate * 1e26) / from.exchangeRate) - 1e26;
  222-224:
    if (maturityRate > 0) {
      // calculate marginal interest
      yield = ((maturityRate * 1e26) / sVault.exchangeRate) - 1e26;
	  
Creator\ZcToken.sol
Tokens\ZcToken.sol
  // @audit maturity, redeemer, and protocol can be cached for the following code
  44-47:
    if (block.timestamp < maturity) {
     return 0;
    }
    return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
  53-56:
    if (block.timestamp < maturity) {
     return 0;
    }
    return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
  71-74:
    if (block.timestamp < maturity) {
     return 0;
    }
    return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
  80-83:
    if (block.timestamp < maturity) {
     return 0;
    }
    return (balanceOf[owner] * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
  89-92:
    if (block.timestamp < maturity) {
      return 0;
    }
    return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
	
  // @audit maturity, redeemer, protocol, and underlying can be cached for the following code
  101-116:
    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);
  126-135:
    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);

Marketplace\MarketPlace.sol
  // @audit swivel can be cached for the following code
  71-77:
    if (swivel == address(0)) { revert Exception(21, 0, 0, address(0), address(0)); }

    address underAddr = Compounding.underlying(p, c);

    if (markets[p][underAddr][m].vaultTracker != address(0)) { revert Exception(22, 0, 0, address(0), address(0)); }

    (address zct, address tracker) = ICreator(creator).create(p, underAddr, m, c, swivel, n, s, IErc20(underAddr).decimals()) ;
  156-164:
    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);
	
Swivel\Swivel.sol
  // audit feeChange can be cached for the following code
  500-502:
    if (feeChange == 0) { revert Exception(16, 0, 0, address(0), address(0)); }

    if (block.timestamp < feeChange) { revert Exception(17, block.timestamp, feeChange, address(0), address(0)); }

[G-02] filled[hash] = amount CAN BE USED INSTEAD OF filled[hash] += a

For the following code, after running uint256 amount = a + filled[hash] , filled[hash] = amount is more gas-efficient than filled[hash] += a, where filled is a mapping in the state storage. This is because that filled[hash] += a needs another sload operation that costs more gas than an mload operation.

Swivel\Swivel.sol
  116-121:
  153-158:
  188-193:
  217-222:
  282-287:
  313-318:
  343-348:
  378-383:
    uint256 amount = a + filled[hash];
    ...
    // TODO cheaper to assign amount here or keep the ADD?
    filled[hash] += a;

[G-03] X = X + Y AND X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y and x = x - y costs less gas than x += y and x -= y.

Creator\VaultTracker.sol
VaultTracker\VaultTracker.sol
  67: vlt.redeemable += interest;
  68: vlt.notional += a;
  102: vlt.redeemable += interest;
  103: vlt.notional -= a;
  174: from.redeemable += interest;
  175: from.notional -= a;
  193: to.redeemable += newVaultInterest;
  194: to.notional += a;
  213: oVault.notional -= a;
  230: sVault.redeemable += interest;
  234: sVault.notional += a;
  
Creator\ZcToken.sol
Tokens\ZcToken.sol
  115: allowance[holder][msg.sender] -= previewAmount;
  134: allowance[holder][msg.sender] -= principalAmount;

[G-04] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++.

Swivel\Swivel.sol
  100: unchecked {i++;}
  269: unchecked {i++;}
  418: i++;
  511: x++;
  564: i++;

[G-05] CONSTANTS CAN BE PRIVATE IF POSSIBLE

For constants that are used only inside the contract, they can be private than public to cost less gas.

Swivel\Swivel.sol
  25: string constant public NAME = 'Swivel Finance';
  26: string constant public VERSION = '3.0.0';
  27: uint256 constant public HOLD = 3 days;
  35: uint16 constant public MIN_FEENOMINATOR = 33;

[G-06] UNUSED NAMED RETURNS COSTS UNNECESSARY DEPLOYMENT GAS

When a function has unused named returns, unnecessary deployment gas is burned.

Marketplace\MarketPlace.sol
  148: function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken) returns (uint256 underlyingAmount) {

Creator\ZcToken.sol
Tokens\ZcToken.sol
  43: function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
  52: function convertToPrincipal(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
  61: function maxRedeem(address owner) external override view returns (uint256 maxPrincipalAmount){
  70: function previewRedeem(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
  79: function maxWithdraw(address owner) external override view returns (uint256 maxUnderlyingAmount){
  88: function previewWithdraw(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
  98: function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
  124: function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){

[G-07] NEWER VERSION OF SOLIDITY CAN BE USED

The protocol can benefit from more gas-efficient features and fixes by using a newer version of Solidity. Changes for newer Solidity versions can be viewed here.

Creator\ZcToken.sol
Tokens\ZcToken.sol
  2: pragma solidity ^0.8.4;