QA Report

[code423n4]

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    TimelockController.sol, isOperation
    TimelockController.sol, isOperationPending
    TimelockController.sol, hashOperation
    TimelockController.sol, hashOperationBatch
    TimelockController.sol, getMinDelay

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.

    OptionsFactory.sol.isQToken _qToken
    EIP712MetaTransaction.sol._verify user
    QTokenStringUtils.sol._qTokenSymbol _strikeAsset
    OptionsUtils.sol.getTargetQTokenAddress _oracle

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.

    CollateralToken.sol.metaSetApprovalForAll owner
    Controller.sol._collateralTokenApproval _owner
    Controller.sol._qTokenPermit _owner
    CollateralToken.sol.burnCollateralTokenBatch owner
    CollateralToken.sol.burnCollateralToken owner

Title: Missing commenting Severity: Low Risk

    The following functions are missing commenting as describe below:

    ConfigTimelockController.sol, schedule (public), parameters target, value, data, predecessor, salt, delay not commented
    EIP712MetaTransaction.sol, _verify (internal), @return is missing
    ConfigTimelockController.sol, _getProtocolValueDelay (internal), parameter protocolValueType not commented

Title: Duplicates in array Severity: Low Risk

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.

AssetsRegistry.addAssetWithOptionalERC20Methods pushed (_underlying)

AssetsRegistry.addAsset pushed (_underlying)

QuantConfig.setProtocolAddress pushed (_protocolAddress)

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    QuantConfig.sol

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    TimelockController.sol, updateDelay
    ChainlinkFixedTimeOracleManager.sol, setFixedTimeUpdate

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..

    Controller.sol, initialize is missing a reentrancy modifier

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    EIP712MetaTransaction.sol, initializeEIP712, 106
    Controller.sol, initialize, 134
    QuantConfig.sol, initialize, 148

Title: Two arrays length mismatch Severity: Low/Med Risk

The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior. Consider making this a medium risk please.

    ConfigTimelockController.sol, scheduleBatch ['targets', 'datas'] 

    CollateralToken.sol, mintCollateralTokenBatch ['ids', 'amounts'] 

    CollateralToken.sol, burnCollateralTokenBatch ['ids', 'amounts'] 

Title: Override function but with different argument location Severity: Low/Med Risk

    ChainlinkFixedTimeOracleManager.sol.constructor inherent ProviderOracleManager.sol.constructor but the parameters does not match
    ChainlinkOracleManager.sol.constructor inherent ProviderOracleManager.sol.constructor but the parameters does not match

Title: Unsafe Cast Severity: Medium Risk

use openzeppilin's safeCast in:

    ChainlinkOracleManager._binarySearchStep : unsafe cast uint64(_firstRoundProxy)
    ChainlinkOracleManager._setExpiryPriceInRegistryByRound : unsafe cast uint64(_roundIdAfterExpiry)
    ChainlinkOracleManager._binarySearchStep : unsafe cast uint64(_lastRoundProxy)

Title: Div by 0 Severity: Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    QuantMath.sol (L151) a might be 0)
    QuantMath.sol (L151) b might be 0)
    QTokenStringUtils.sol (L145) strikePriceScale might be 0)

[alcueca]

This QA report has numerous errors. Striking it as invalid.