Merge pull request #338 from VenusProtocol/VEN-1887

[VEN-1887]: Quantstamp audit fix for comptroller diamond proxy

contracts/Comptroller/Diamond/Diamond.sol

@@ -6,6 +6,11 @@ pragma experimental ABIEncoderV2;
 import { IDiamondCut } from "./interfaces/IDiamondCut.sol";
 import { Unitroller, ComptrollerV12Storage } from "../Unitroller.sol";
 
+/**
+ * @title Diamond
+ * @author Venus
+ * @notice This contract contains functions related to facets
+ */
 contract Diamond is IDiamondCut, ComptrollerV12Storage {
     /// @notice Emitted when functions are added, replaced or removed to facets
     event DiamondCut(IDiamondCut.FacetCut[] _diamondCut);
@@ -25,7 +30,8 @@ contract Diamond is IDiamondCut, ComptrollerV12Storage {
     }
 
     /**
-     * @notice To add function selectors to the facets' mapping
+     * @notice To add function selectors to the facet's mapping
+     * @dev Allows the contract admin to add function selectors
      * @param diamondCut_ IDiamondCut contains facets address, action and function selectors
      */
     function diamondCut(IDiamondCut.FacetCut[] memory diamondCut_) public {

contracts/Comptroller/Diamond/facets/FacetBase.sol

@@ -8,6 +8,11 @@ import { ComptrollerV12Storage } from "../../../Comptroller/ComptrollerStorage.s
 import { IAccessControlManager } from "../../../Governance/IAccessControlManager.sol";
 import { SafeBEP20, IBEP20 } from "../../../Utils/SafeBEP20.sol";
 
+/**
+ * @title FacetBase
+ * @author Venus
+ * @notice This facet contract contains functions related to access and checks
+ */
 contract FacetBase is ComptrollerV12Storage, ExponentialNoError, ComptrollerErrorReporter {
     /// @notice Emitted when an account enters a market
     event MarketEntered(VToken indexed vToken, address indexed account);

contracts/Comptroller/Diamond/facets/MarketFacet.sol

@@ -6,7 +6,10 @@ import { IMarketFacet } from "../interfaces/IMarketFacet.sol";
 import { FacetBase, VToken } from "./FacetBase.sol";
 
 /**
+ * @title MarketFacet
+ * @author Venus
  * @dev This facet contains all the methods related to the market's management in the pool
+ * @notice This facet contract contains functions regarding markets
  */
 contract MarketFacet is IMarketFacet, FacetBase {
     /// @notice Emitted when an admin supports a market
@@ -167,7 +170,7 @@ contract MarketFacet is IMarketFacet, FacetBase {
 
     /**
      * @notice Add the market to the markets mapping and set it as listed
-     * @dev Admin function to set isListed and add support for the market
+     * @dev Allows a privileged role to add and list markets to the Comptroller
      * @param vToken The address of the market (token) to list
      * @return uint256 0=success, otherwise a failure. (See enum Error for details)
      */

contracts/Comptroller/Diamond/facets/PolicyFacet.sol

@@ -7,7 +7,10 @@ import { IPolicyFacet } from "../interfaces/IPolicyFacet.sol";
 import { XVSRewardsHelper, VToken } from "./XVSRewardsHelper.sol";
 
 /**
+ * @title PolicyFacet
+ * @author Venus
  * @dev This facet contains all the hooks used while transferring the assets
+ * @notice This facet contract contains all the external pre-hook functions related to vToken
  */
 contract PolicyFacet is IPolicyFacet, XVSRewardsHelper {
     /// @notice Emitted when a new borrow-side XVS speed is calculated for a market
@@ -417,6 +420,7 @@ contract PolicyFacet is IPolicyFacet, XVSRewardsHelper {
     // setter functionality
     /**
      * @notice Set XVS speed for a single market
+     * @dev Allows the contract admin to set XVS speed for a market
      * @param vTokens The market whose XVS speed to update
      * @param supplySpeeds New XVS speed for supply
      * @param borrowSpeeds New XVS speed for borrow

contracts/Comptroller/Diamond/facets/RewardFacet.sol

@@ -8,7 +8,10 @@ import { SafeBEP20, IBEP20 } from "../../../Utils/SafeBEP20.sol";
 import { VBep20Interface } from "../../../Tokens/VTokens/VTokenInterfaces.sol";
 
 /**
+ * @title RewardFacet
+ * @author Venus
  * @dev This facet contains all the methods related to the reward functionality
+ * @notice This facet contract provides the external functions related to all claims and rewards of the protocol
  */
 contract RewardFacet is IRewardFacet, XVSRewardsHelper {
     /// @notice Emitted when Venus is granted by admin
@@ -110,7 +113,8 @@ contract RewardFacet is IRewardFacet, XVSRewardsHelper {
 
     /**
      * @notice Transfer XVS to the recipient
-     * @dev Note: If there is not enough XVS, we do not perform the transfer all
+     * @dev Allows the contract admin to transfer XVS to any recipient based on the recipient's shortfall
+     *      Note: If there is not enough XVS, we do not perform the transfer all
      * @param recipient The address of the recipient to transfer XVS to
      * @param amount The amount of XVS to (possibly) transfer
      */

contracts/Comptroller/Diamond/facets/SetterFacet.sol

@@ -9,7 +9,10 @@ import { VAIControllerInterface } from "../../../Tokens/VAI/VAIControllerInterfa
 import { FacetBase, VToken } from "./FacetBase.sol";
 
 /**
+ * @title SetterFacet
+ * @author Venus
  * @dev This facet contains all the setters for the states
+ * @notice This facet contract contains all the configurational setter functions
  */
 contract SetterFacet is ISetterFacet, FacetBase {
     /// @notice Emitted when close factor is changed by admin
@@ -95,7 +98,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Sets a new price oracle for the comptroller
-     * @dev Admin function to set a new price oracle
+     * @dev Allows the contract admin to set a new price oracle used by the Comptroller
      * @return uint256 0=success, otherwise a failure (see ErrorReporter.sol for details)
      */
     function _setPriceOracle(
@@ -119,7 +122,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Sets the closeFactor used when liquidating borrows
-     * @dev Admin function to set closeFactor
+     * @dev Allows the contract admin to set the closeFactor used to liquidate borrows
      * @param newCloseFactorMantissa New close factor, scaled by 1e18
      * @return uint256 0=success, otherwise will revert
      */
@@ -129,8 +132,18 @@ contract SetterFacet is ISetterFacet, FacetBase {
         // Check caller is admin
         ensureAdmin();
 
-        uint256 oldCloseFactorMantissa = closeFactorMantissa;
+        Exp memory newCloseFactorExp = Exp({ mantissa: newCloseFactorMantissa });
+
+        //-- Check close factor <= 0.9
+        Exp memory highLimit = Exp({ mantissa: closeFactorMaxMantissa });
+        //-- Check close factor >= 0.05
+        Exp memory lowLimit = Exp({ mantissa: closeFactorMinMantissa });
 
+        if (lessThanExp(highLimit, newCloseFactorExp) || greaterThanExp(lowLimit, newCloseFactorExp)) {
+            return fail(Error.INVALID_CLOSE_FACTOR, FailureInfo.SET_CLOSE_FACTOR_VALIDATION);
+        }
+
+        uint256 oldCloseFactorMantissa = closeFactorMantissa;
         closeFactorMantissa = newCloseFactorMantissa;
         emit NewCloseFactor(oldCloseFactorMantissa, newCloseFactorMantissa);
 
@@ -139,7 +152,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Sets the address of the access control of this contract
-     * @dev Admin function to set the access control address
+     * @dev Allows the contract admin to set the address of access control of this contract
      * @param newAccessControlAddress New address for the access control
      * @return uint256 0=success, otherwise will revert
      */
@@ -160,7 +173,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Sets the collateralFactor for a market
-     * @dev Restricted function to set per-market collateralFactor
+     * @dev Allows a privileged role to set the collateralFactorMantissa
      * @param vToken The market to set the factor on
      * @param newCollateralFactorMantissa The new collateral factor, scaled by 1e18
      * @return uint256 0=success, otherwise a failure. (See ErrorReporter for details)
@@ -206,7 +219,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Sets liquidationIncentive
-     * @dev Admin function to set liquidationIncentive
+     * @dev Allows a privileged role to set the liquidationIncentiveMantissa
      * @param newLiquidationIncentiveMantissa New liquidationIncentive scaled by 1e18
      * @return uint256 0=success, otherwise a failure. (See ErrorReporter for details)
      */
@@ -230,21 +243,23 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Update the address of the liquidator contract
-     * @dev Admin function to set liquidator contract
+     * @dev Allows the contract admin to update the address of liquidator contract
      * @param newLiquidatorContract_ The new address of the liquidator contract
      */
     function _setLiquidatorContract(
         address newLiquidatorContract_
     ) external compareAddress(liquidatorContract, newLiquidatorContract_) {
         // Check caller is admin
         ensureAdmin();
+        ensureNonzeroAddress(newLiquidatorContract_);
         address oldLiquidatorContract = liquidatorContract;
         liquidatorContract = newLiquidatorContract_;
         emit NewLiquidatorContract(oldLiquidatorContract, newLiquidatorContract_);
     }
 
     /**
      * @notice Admin function to change the Pause Guardian
+     * @dev Allows the contract admin to change the Pause Guardian
      * @param newPauseGuardian The address of the new Pause Guardian
      * @return uint256 0=success, otherwise a failure. (See enum Error for details)
      */
@@ -267,7 +282,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Set the given borrow caps for the given vToken market Borrowing that brings total borrows to or above borrow cap will revert
-     * @dev Access is controled by ACM. A borrow cap of 0 corresponds to unlimited borrowing
+     * @dev Allows a privileged role to set the borrowing cap for a vToken market. A borrow cap of 0 corresponds to unlimited borrowing
      * @param vTokens The addresses of the markets (tokens) to change the borrow caps for
      * @param newBorrowCaps The new borrow cap values in underlying to be set. A value of 0 corresponds to unlimited borrowing
      */
@@ -287,7 +302,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Set the given supply caps for the given vToken market Supply that brings total Supply to or above supply cap will revert
-     * @dev Admin function to set the supply cap A supply cap of 0 corresponds to Minting NotAllowed
+     * @dev Allows a privileged role to set the supply cap for a vToken. A supply cap of 0 corresponds to Minting NotAllowed
      * @param vTokens The addresses of the markets (tokens) to change the supply caps for
      * @param newSupplyCaps The new supply cap values in underlying to be set. A value of 0 corresponds to Minting NotAllowed
      */
@@ -307,6 +322,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Set whole protocol pause/unpause state
+     * @dev Allows a privileged role to pause/unpause protocol
      * @param state The new state (true=paused, false=unpaused)
      * @return bool The updated state of the protocol
      */
@@ -320,6 +336,7 @@ contract SetterFacet is ISetterFacet, FacetBase {
 
     /**
      * @notice Pause/unpause certain actions
+     * @dev Allows a privileged role to pause/unpause the protocol action state
      * @param markets_ Markets to pause/unpause the actions on
      * @param actions_ List of action ids to pause/unpause
      * @param paused_ The new paused state (true=paused, false=unpaused)

contracts/Comptroller/Diamond/facets/XVSRewardsHelper.sol

@@ -5,7 +5,10 @@ pragma solidity 0.5.16;
 import { FacetBase, VToken } from "./FacetBase.sol";
 
 /**
+ * @title XVSRewardsHelper
+ * @author Venus
  * @dev This contract contains internal functions used in RewardFacet and PolicyFacet
+ * @notice This facet contract contains the shared functions used by the RewardFacet and PolicyFacet
  */
 contract XVSRewardsHelper is FacetBase {
     /// @notice Emitted when XVS is distributed to a borrower

tests/hardhat/Comptroller/Diamond/comptrollerTest.ts

@@ -201,6 +201,13 @@ describe("Comptroller", () => {
         "old address is same as new address",
       );
     });
+
+    it("should revert on zero address", async () => {
+      await comptroller._setLiquidatorContract(accounts[0].address);
+      await expect(comptroller._setLiquidatorContract(constants.AddressZero)).to.be.revertedWith(
+        "can't be zero address",
+      );
+    });
   });
 
   describe("_setPauseGuardian", () => {
@@ -328,6 +335,7 @@ describe("Comptroller", () => {
 
   describe("_setCloseFactor", () => {
     let comptroller: ComptrollerMock;
+    let unitroller: Unitroller;
 
     beforeEach(async () => {
       ({ comptroller } = await loadFixture(deploySimpleComptroller));
@@ -340,6 +348,15 @@ describe("Comptroller", () => {
     it("should revert on same values", async () => {
       await expect(comptroller._setCloseFactor(0)).to.be.revertedWith("old value is same as new value");
     });
+
+    it("fails if factor is set out of range", async () => {
+      expect(await comptroller._setCloseFactor(convertToUnit(1, 18)))
+        .to.emit(unitroller, "Failure")
+        .withArgs(
+          ComptrollerErrorReporter.Error.INVALID_CLOSE_FACTOR,
+          ComptrollerErrorReporter.FailureInfo.SET_CLOSE_FACTOR_VALIDATION,
+        );
+    });
   });
 
   describe("_setCollateralFactor", () => {