QA Report

[code423n4]

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);

[Joeysantoro]

Fixing multiplication precision: https://github.com/fei-protocol/tribe-turbo/pull/62

Ack others but not fix.

Unsafe Cast issue disputed, there is no casting in that function

[GalloDaSballo]

Really dislike the formatting as it honestly shows this report is basically from automated software

There's the usual basic findings here, would rate 4 however because of formatting, presentation and ultimately because this feels like a soulless copy paste I'll downgrade to 3/10