QA Report

[code423n4]

Table of Contents:

• [L-01] approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()
• [L-02] Add constructor initializers
• [L-03] Missing address(0) checks
• [L-04] Add a timelock and event to critical functions
• [L-05] Fee in DemandMinerV2.setFeeConfig() should be upper-bounded
• [N-01] Unused named returns
• [N-02] Useless import: SafeMath
• [N-03] The visibility for constructor is ignored

[L-01] approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()

approve is subject to a known front-running attack. Consider using safeApprove instead:

core/contracts/liquidityMining/v2/PARMinerV2.sol:
   58:     _par.approve(address(_a.parallel().core()), uint256(-1));
  125:     collateralToken.approve(proxy, collateralToken.balanceOf(address(this)));

core/echidna/TInceptionVaultHealthy.sol:
  33:     _weth.approve(address(a), _adminDepositAmount);
  39:     _link.approve(address(v), _userDepositAmount);
  47:     _par.approve(address(_inceptionVaultsCore), _MAX_INT);

core/echidna/TInceptionVaultUnhealthy.sol:
  37:     _weth.approve(address(a), _adminDepositAmount);
  43:     _link.approve(address(v), _userDepositAmount);
  52:     _par.approve(address(_inceptionVaultsCore), _MAX_INT);

core/echidna/TInceptionVaultUnhealthyAssertion.sol:
  36:     _weth.approve(address(a), _adminDepositAmount);
  42:     _link.approve(address(v), _userDepositAmount);
  51:     _par.approve(address(_inceptionVaultsCore), _MAX_INT);

core/echidna/TInceptionVaultUnhealthyProperty.sol:
  35:     _weth.approve(address(a), _adminDepositAmount);
  41:     _link.approve(address(v), _userDepositAmount);
  50:     _par.approve(address(_inceptionVaultsCore), _MAX_INT);

supervaults/contracts/SuperVault.sol:
   97:     asset.approve(address(lendingPool), flashloanRepayAmount);
  149:     IERC20(toCollateral).approve(address(a.core()), depositAmount);
  199:     par.approve(address(a.core()), par.balanceOf(address(this)));
  273:     token.approve(address(a.core()), amount);
  289:     token.approve(address(a.core()), depositAmount);
  326:     token.approve(address(a.core()), 2**256 - 1);
  345:     token.approve(proxy, amount);

Keep in mind though that it would be actually better to replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance().

See this discussion: SafeERC20.safeApprove() Has unnecessary and unsecure added behavior

[L-02] Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

core/contracts/inception/AdminInceptionVault.sol:
  35:   function initialize(

core/contracts/inception/InceptionVaultsCore.sol:
  40:   function initialize(

core/contracts/inception/InceptionVaultsDataProvider.sol:
  30:   function initialize(IInceptionVaultsCore inceptionVaultsCore, IAddressProvider addressProvider)

core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol:
  29:   function initialize(

supervaults/contracts/SuperVault.sol:
  49:   function initialize(

[L-03] Missing address(0) checks

According to Slither:

AdminInceptionVault.initialize(address,IAddressProvider,IDebtNotifier,IWETH,IERC20,IInceptionVaultsCore)._owner (contracts/inception/AdminInceptionVault.sol#36) lacks a zero-check on :
  - owner = _owner (contracts/inception/AdminInceptionVault.sol#48)
InceptionVaultsCore.initialize(address,IInceptionVaultsCore.VaultConfig,IERC20,IAddressProvider,IAdminInceptionVault,IInceptionVaultsDataProvider,IInceptionVaultPriceFeed)._owner (contracts/inception/InceptionVaultsCore.sol#41) lacks a zero-check on :
  - owner = _owner (contracts/inception/InceptionVaultsCore.sol#56)
DemandMinerV2.setFeeCollector(address).feeCollector (contracts/liquidityMining/v2/DemandMinerV2.sol#46) lacks a zero-check on :
  - _feeCollector = feeCollector (contracts/liquidityMining/v2/DemandMinerV2.sol#47)
PARMinerV2.liquidate(uint256,uint256,uint256,bytes).router (contracts/liquidityMining/v2/PARMinerV2.sol#124) lacks a zero-check on :
  - router.call(dexTxData) (contracts/liquidityMining/v2/PARMinerV2.sol#126)

[L-04] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Manager making a frontrunning/sandwich attack on the fees).

Consider adding a timelock to:

File: DemandMinerV2.sol
56:   function setFeeConfig(FeeConfig memory newFeeConfig) external override onlyManager {
57:     _feeConfig = newFeeConfig;
58:     emit FeeConfigSet(newFeeConfig);
59:   }

[L-05] Fee in DemandMinerV2.setFeeConfig() should be upper-bounded

File: DemandMinerV2.sol
56:   function setFeeConfig(FeeConfig memory newFeeConfig) external override onlyManager {
57:     _feeConfig = newFeeConfig;
58:     emit FeeConfigSet(newFeeConfig);
59:   }

[N-01] Unused named returns

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol:
  73:   function getAssetPrice() public view override returns (uint256 price) {

[N-02] Useless import: SafeMath

File: SuperVault.sol
6: import "@openzeppelin/contracts/utils/math/SafeMath.sol"; //@audit NC: useless import

[N-03] The visibility for constructor is ignored

File: SuperVaultFactory.sol
17:   constructor(address _base) public {

[m19]

Very clear and well structured QA report

[gzeoneth]

For L-01 I don't think there are front-running risk but the suggestion to use safeIncreaseAllowance is fine
Otherwise looks good and I think the severity of each issue is well labeled.