QA Report

[code423n4]

L01 - withdraw and redeem function would not work if msg.sender != holder

Both functions in case when msg.sender != holder checks allowance amount in wrong way - in case if allowance >= previewAmount they revert transaction instead of checking for allowance < previewAmount, this case would be reverted in next line due to check on underflow allowance[holder][msg.sender] -= previewAmount This lead to inability to use them using "allowance" functionality.

    function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
        uint256 previewAmount = this.previewWithdraw(underlyingAmount);
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        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;
        }
    }

    function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){
        if (block.timestamp < maturity) { revert Maturity(maturity); }
        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);
        }
    }

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/ZcToken.sol#L98-L143

Recommendation: Update allowance check to if (allowed < principalAmount) { revert Approvals(allowed, principalAmount); } Also due to check logic next line could be unchecked for gas saving: allowance[holder][msg.sender] -= principalAmount;

L02 - Lack of event emitting after sensitive actions

Contracts do not emit relevant events after setting sensitive variables. Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity in following functions:

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

  /// @param m Address of the deployed marketPlace contract
  /// @notice We only allow this to be set once
  function setMarketPlace(address m) external authorized(admin) returns (bool) {  missing event 
    if (marketPlace != address(0)) {
      revert Exception(33, 0, 0, marketPlace, address(0)); 
    }

    marketPlace = m;
    return true;
  }

https://github.com/code-423n4/2022-07-swivel/blob/main/Creator/Creator.sol#L47-L57

  /// @param s Address of the deployed swivel contract
  /// @notice We only allow this to be set once
  function setSwivel(address s) external authorized(admin) returns (bool) { 
    if (swivel != address(0)) { revert Exception(20, 0, 0, swivel, address(0));  }

    swivel = s;
    return true;
  }

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

https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L45-L56

  /// @notice Matures the vault
  /// @param c The current cToken exchange rate
  function matureVault(uint256 c) external authorized(admin) returns (bool) { 
    maturityRate = c;
    return true;
  }

https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L143-L146

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

    return true;
  }

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L428-L432

L03 - Missing input validation of array lengths

Few functions fails to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior. https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L82-L104

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L244-L273

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L407-L423

N01 - Open TODOs

Swivel/Swivel.sol:33  address public aaveAddr; // TODO immutable?  

Swivel/Swivel.sol:120 // TODO cheaper to assign amount here or keep the ADD? 

Swivel/Swivel.sol:157 // TODO assign amount or keep the ADD? 

Swivel/Swivel.sol:192 // TODO assign amount or keep the ADD? 

Swivel/Swivel.sol:221 // TODO assign amount or keep ADD?  

Swivel/Swivel.sol:317 // TODO assign amount or keep the ADD?

Swivel/Swivel.sol:347 // TODO assign amount or keep the ADD?  

Swivel/Swivel.sol:382 // TODO assign amount or keep the ADD?  

Swivel/Swivel.sol:708 if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here? 

Swivel/Swivel.sol:741 if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here? 

Swivel/Swivel.sol:748 // TODO explain the withdraw args

Swivel/Swivel.sol:752 // TODO explain the 0 

N02 - Typos

Swivel/Swivel.sol:36 /// @dev holds the fee demoninators for [zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit]

Swivel/Swivel.sol:677 // NOTE: for swivel reddem there is no transfer out as there is in redeemVaultInterest

Swivel/Swivel.sol:682 /// @notice Varifies the validity of an order and it's signature.

Swivel/Swivel.sol:747 // Aave v2 docs state that withraw returns uint256

N03 - Interface and implementation function declaration differs

Function has name authRedeemZcToken: https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L620 While interface decleres it authRedeem https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/Interfaces.sol#L52

[robrobbins]

the zctoken comparisons were addressed in another ticket