Swivel v3 contest - sashik_eth's results

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

G01 - Caching storage variables

Since reading from memory is much cheaper than reading from storage, state variables that are called more than 1 SLOAD inside the function should be cached.

Marketplace/MarketPlace.sol:77  (address zct, address tracker) = ICreator(creator).create(p, underAddr, m, c, swivel, n, s, IErc20(underAddr).decimals()) ; // swivel 2nd SLOAD

Swivel/Swivel.sol:502 if (block.timestamp < feeChange) { revert Exception(17, block.timestamp, feeChange, address(0), address(0)); }  // feeChange 2nd SLOAD

G02 - Variable that could be immutables

It's possible to avoid storage access and save gas using immutable keyword for the following variables:

Swivel/Swivel.sol:33  address public aaveAddr;

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

G03 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

L363 could be unchecked since due to L362 fee <= premiumFilled

    Swivel/Swivel.sol:362 uint256 fee = premiumFilled / feenominators[3];
    Swivel/Swivel.sol:363 Safe.transfer(uToken, msg.sender, premiumFilled - fee);  

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

Lines with adding block.timestamp value to constant value could be unchecked since their overflow would appear only in the extremely far future:

Swivel/Swivel.sol:437 uint256 when = block.timestamp + HOLD;
Swivel/Swivel.sol:474 uint256 when = block.timestamp + HOLD;  
Swivel/Swivel.sol:523 uint256 when = block.timestamp + HOLD; 

G04 - Using prefix increment

Prefix increment is cheaper than postfix increment. Consider using ++i in next lines:

Swivel/Swivel.sol:100 unchecked {i++;} 

Swivel/Swivel.sol:269 unchecked {i++;}  

Swivel/Swivel.sol:418 i++;  

Swivel/Swivel.sol:511 x++;  

Swivel/Swivel.sol:564 i++;  

G05 - No needed computation

Line 121 could be filled[hash] = amount without repeated computation from line 116:

    Swivel/Swivel.sol:116    uint256 amount = a + filled[hash];
		...
    Swivel/Swivel.sol:121    filled[hash] += a; 

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L116-L121 Same here: https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L158 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L193 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L222 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L287 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L318 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L348 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L383