Tribe Turbo contest - robee's results

Title: Missing fee parameter validation Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    TurboClerk.setCustomFeePercentageForSafe (newFeePercentage)
    TurboClerk.setDefaultFeePercentage (newDefaultFeePercentage)
    TurboClerk.setCustomFeePercentageForCollateral (newFeePercentage)

Title: Require with empty message Severity: Low Risk

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: TurboGibber.sol, In line 100 with Empty Require message.
    Solidity file: TurboSavior.sol, In line 106 with Empty Require message.
    Solidity file: TurboGibber.sol, In line 81 with Empty Require message.

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    TurboRouter.sol.withdraw to
    TurboRouter.sol.createSafeAndDeposit to
    TurboSafe.sol.gib to
    TurboRouter.sol.redeem to
    TurboRouter.sol.deposit to
    TurboRouter.sol.mint to
    TurboSafe.sol.sweep to
    TurboGibber.sol.impound to
    TurboMaster.sol.sweep to
    TurboGibber.sol.impoundAll to
    TurboRouter.sol.sweep to
    TurboRouter.sol.createSafeAndDepositAndBoost to

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    TurboBooster.sol: function canSafeBoostVault parameter safe isn't used. (canSafeBoostVault is external)
    TurboBooster.sol: function canSafeBoostVault parameter feiAmount isn't used. (canSafeBoostVault is external)

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    TurboMaster.sol.constructor _owner
    TurboClerk.sol.constructor _owner
    TurboBooster.sol.constructor _owner
    TurboSavior.sol.constructor _owner
    TurboGibber.sol.constructor _owner
    TurboSafe.sol.constructor _owner

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    TurboSavior.sol, setMinDebtPercentageForSaving is missing a reentrancy modifier

Title: Require with not comprehensive message Severity: Low Risk

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: TurboSafe.sol, In line 173 with Require message: NOT_FEI

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base
    

Deprecated safeApprove in TurboSafe.sol line 91: fei.safeApprove(address(feiTurboCToken), type(uint256).max);

Deprecated safeApprove in TurboGibber.sol line 54: fei.safeApprove(address(feiTurboCToken), type(uint256).max);

Deprecated safeApprove in TurboSafe.sol line 88: asset.safeApprove(address(assetTurboCToken), type(uint256).max);

Deprecated safeApprove in TurboSafe.sol line 193: fei.safeApprove(address(vault), feiAmount);

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:
        
        Instead a < b / c use a * c < b. 
    
In all of the big and trusted contracts this rule is maintained.

    TurboSavior.sol, 129, require( borrowLimit != 0 && debtValue.divWadUp(borrowLimit) >= minDebtPercentageForSaving, "DEBT_PERCENT_TOO_LOW" ); 

Title: Unsafe Cast Severity: Medium Risk

use openzeppilin's safeCast in:

    TurboSafe.afterDeposit : unsafe cast int(assetAmount)

Title: Must approve 0 first Severity: Low/Med Risk

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

approve without approving 0 first TurboSafe.sol, 193, fei.safeApprove(address(vault), feiAmount);

approve without approving 0 first TurboSafe.sol, 88, asset.safeApprove(address(assetTurboCToken), type(uint256).max);

approve without approving 0 first TurboSafe.sol, 91, fei.safeApprove(address(feiTurboCToken), type(uint256).max);

approve without approving 0 first TurboGibber.sol, 54, fei.safeApprove(address(feiTurboCToken), type(uint256).max);

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    TurboSafe.sol, afterDeposit
    TurboSafe.sol, beforeWithdraw

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    TurboRouter.constructor (name)

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    TurboMaster.sol, line 12, import {TurboGibber} from "./modules/TurboGibber.sol";

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    TurboClerk.sol.constructor
    TurboBooster.sol.constructor

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    TurboSafe.sol, afterDeposit
    TurboSafe.sol, beforeWithdraw

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    TurboRouter.sol, withdraw
    TurboRouter.sol, redeem
    TurboRouter.sol, deposit
    TurboRouter.sol, boost
    TurboRouter.sol, createSafeAndDepositAndBoost
    TurboRouter.sol, mint
    TurboSafe.sol, totalAssets

Title: Unnecessary payable Severity: GAS

The following functions are payable but msg.value isn't used - therefore the function payable state modifier isn't necessary. Payable functions are more gas expensive than others, and it's danger the users if they send ETH by mistake.

    TurboRouter.sol, withdraw is payable but doesn't use msg.value
    TurboRouter.sol, redeem is payable but doesn't use msg.value
    TurboRouter.sol, mint is payable but doesn't use msg.value
    TurboRouter.sol, deposit is payable but doesn't use msg.value