From df08df4332e781c571a59b5c136438e0dd5961ab Mon Sep 17 00:00:00 2001 From: Debugger022 <104391977+Debugger022@users.noreply.github.com> Date: Fri, 8 Sep 2023 16:14:49 +0530 Subject: [PATCH] [VEN-1887]: Quantstamp audit fix for comptroller diamond proxy (#328) * fix: ven-1887 ven-08 * fix: ven-1887 ven-12 * fix: ven-1887 ven-04 * fix: lint * fix: replaced And operator with OR while checking cutoff * refactor: netspec comments * Revert "fix: ven-1887 ven-12" This reverts commit 0e24ded96aeea3ac7aaac56daa9b0e9fab228452. * fix: lint --- contracts/Comptroller/Diamond/Diamond.sol | 8 ++++- .../Comptroller/Diamond/facets/FacetBase.sol | 5 +++ .../Diamond/facets/MarketFacet.sol | 5 ++- .../Diamond/facets/PolicyFacet.sol | 4 +++ .../Diamond/facets/RewardFacet.sol | 6 +++- .../Diamond/facets/SetterFacet.sol | 35 ++++++++++++++----- .../Diamond/facets/XVSRewardsHelper.sol | 3 ++ .../Comptroller/Diamond/comptrollerTest.ts | 17 +++++++++ 8 files changed, 71 insertions(+), 12 deletions(-) diff --git a/contracts/Comptroller/Diamond/Diamond.sol b/contracts/Comptroller/Diamond/Diamond.sol index 86d991267..21ceef7c3 100644 --- a/contracts/Comptroller/Diamond/Diamond.sol +++ b/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 { diff --git a/contracts/Comptroller/Diamond/facets/FacetBase.sol b/contracts/Comptroller/Diamond/facets/FacetBase.sol index 44891e625..66336a7cb 100644 --- a/contracts/Comptroller/Diamond/facets/FacetBase.sol +++ b/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); diff --git a/contracts/Comptroller/Diamond/facets/MarketFacet.sol b/contracts/Comptroller/Diamond/facets/MarketFacet.sol index ceacb7930..4209cafb5 100644 --- a/contracts/Comptroller/Diamond/facets/MarketFacet.sol +++ b/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) */ diff --git a/contracts/Comptroller/Diamond/facets/PolicyFacet.sol b/contracts/Comptroller/Diamond/facets/PolicyFacet.sol index 5a304308b..2b0e3d50e 100644 --- a/contracts/Comptroller/Diamond/facets/PolicyFacet.sol +++ b/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 diff --git a/contracts/Comptroller/Diamond/facets/RewardFacet.sol b/contracts/Comptroller/Diamond/facets/RewardFacet.sol index e059e16a7..6e2888a7b 100644 --- a/contracts/Comptroller/Diamond/facets/RewardFacet.sol +++ b/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 */ diff --git a/contracts/Comptroller/Diamond/facets/SetterFacet.sol b/contracts/Comptroller/Diamond/facets/SetterFacet.sol index 241230bec..561b75240 100644 --- a/contracts/Comptroller/Diamond/facets/SetterFacet.sol +++ b/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,7 +243,7 @@ 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( @@ -238,6 +251,7 @@ contract SetterFacet is ISetterFacet, FacetBase { ) external compareAddress(liquidatorContract, newLiquidatorContract_) { // Check caller is admin ensureAdmin(); + ensureNonzeroAddress(newLiquidatorContract_); address oldLiquidatorContract = liquidatorContract; liquidatorContract = newLiquidatorContract_; emit NewLiquidatorContract(oldLiquidatorContract, newLiquidatorContract_); @@ -245,6 +259,7 @@ contract SetterFacet is ISetterFacet, FacetBase { /** * @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) diff --git a/contracts/Comptroller/Diamond/facets/XVSRewardsHelper.sol b/contracts/Comptroller/Diamond/facets/XVSRewardsHelper.sol index 82fb819ec..f2318eba8 100644 --- a/contracts/Comptroller/Diamond/facets/XVSRewardsHelper.sol +++ b/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 diff --git a/tests/hardhat/Comptroller/Diamond/comptrollerTest.ts b/tests/hardhat/Comptroller/Diamond/comptrollerTest.ts index 15560e882..511860552 100644 --- a/tests/hardhat/Comptroller/Diamond/comptrollerTest.ts +++ b/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", () => {