QA Report

[code423n4]

QA Report

Non-Critical Findings

Contract implementations should inherit their interface

Description

It's best practice for a contract to inherit from it's interface. This improves the contract's clarity and makes sure the contract implementation complies with the defined interface.

Findings

QuantConfig.sol:

contract QuantConfig is
    AccessControlUpgradeable,
    OwnableUpgradeable,
    IQuantConfig, // @audit-info add interface IQuantConfig
    ITimelockedConfig
{
  ...
}

Recommended mitigation steps

Inherit from the missing interface or contract.

---

Public functions that could be declared external

Description

Following functions should be declared external, as functions that are never called by the contract internally should be declared external (to save gas) as well as for code clarity reasons.

Findings

ChainlinkFixedTimeOracleManager.isValidOption()
ProviderOracleManager.isValidOption()
ChainlinkOracleManager.isValidOption()
PriceRegistry.hasSettlementPrice()

Recommended mitigation steps

Use the external attribute for functions never called from the contract.

---

Spelling mistakes

Description

Spelling mistakes found.

Findings

timelock/ConfigTimelockController.sol

L623: _isProtocoValueSetter -> _isProtocolValueSetter

Recommended mitigation steps

Fix spelling mistakes.

---

Redundant storage access - use variable assetOracle

Description

I know there are no rewards for gas optimizations, nevertheless I want to address this finding to prevent any future issues and confusions.

The current implementation of ProviderOracleManager.getAssetOracle() has a require statement which repeatedly accesses assetOracles[_asset] from storage:

function getAssetOracle(address _asset)
    public
    view
    override
    returns (address)
{
    address assetOracle = assetOracles[_asset];
    require(
        assetOracles[_asset] != address(0), // @audit repeated storage variable access
        "ProviderOracleManager: Oracle doesn't exist for that asset"
    );
    return assetOracle;
}

Findings

ProviderOracleManager.getAssetOracle()

Recommended mitigation steps

Change code to:

function getAssetOracle(address _asset)
    public
    view
    override
    returns (address)
{
    address assetOracle = assetOracles[_asset];
    require(
        assetOracle != address(0), // @audit use `assetOracle` instead
        "ProviderOracleManager: Oracle doesn't exist for that asset"
    );
    return assetOracle;
}

Low Risk

Missing documentation of function parameters

Description

Function parameters should be fully documented to provide clear instructions what parameters are used for.

Findings

QTokenStringUtils._qTokenName(): missing @param for _strikeAsset
OptionsUtils.getTargetQTokenAddress(): missing @param for _quantConfig
OptionsUtils.getTargetCollateralTokenId(): missing @param for _quantConfig

Recommended mitigation steps

Add @param documentation for all parameters.

---

ChainlinkOracleManager.isValidOption() always returns true for any asset

Description

The current implementation of ChainlinkOracleManager.isValidOption() always returns true, not matter which _asset is passed:

function isValidOption(
    address,
    uint256,
    uint256
)
    public
    view
    virtual
    override(ProviderOracleManager, IProviderOracleManager)
    returns (bool)
{
    return true; // @audit-info always returns `true`
}

There are no risks caused by to this implementation as there are checks in place to make sure there's a valid oracle price for the underlying qToken asset (e.g in PriceRegistry.getSettlementPriceWithDecimals()).

Nevertheless this function should check if the passed _asset is valid.

Findings

ChainlinkOracleManager.isValidOption()

Recommended mitigation steps

Change implementation to:

function isValidOption(
    address _asset,
    uint256,
    uint256
)
    public
    view
    virtual
    override(ProviderOracleManager, IProviderOracleManager)
    returns (bool)
{
    getAssetOracle(_asset); // @audit-info add this function call which has a `require` statement to assert `_asset` is supported by oracle

    return true;
}

and fix failing unit test in ChainlinkOracleManager.test.ts#L385

---

Shadowed variables

Description

Possible incorrect use of variables are at stake which may have bad side effects to the contract if implemented incorrectly.

The variable _timestamps does not pose any immediate risk to the contract as it's not used in the contract, incorrect usage of the variable is possible and can cause serious issues if the developer does not pay close attention.

The variable minDelay could cause issues as it's used in the contract. As the shadowed minDelay variable in the inherited contract TimelockController.sol can be changed (with updateDelay()), the variable with the same name in contract ConfigTimelockController does not change and is out of sync.

Findings

timelock/ConfigTimelockController.sol._timestamps
timelock/ConfigTimelockController.sol.minDelay

Recommended mitigation steps

timelock/ConfigTimelockController.sol._timestamps:

Remove variable declaration _timestamps:

mapping(bytes32 => uint256) private _timestamps;

timelock/ConfigTimelockController.sol.minDelay:

Remove variable declaration minDelay:

uint256 public minDelay;

Replace all occurrences of minDelay with getMinDelay().

[alcueca]

Score: 94 (including points from #60)