Merge pull request #642 from VenusProtocol/feat/quantstamp-audit-mitigations

[VPD-226] Flash Loan Quantstamp Audit Mitigations

Author: Debugger022

contracts/Comptroller/ComptrollerInterface.sol

@@ -207,6 +207,8 @@ interface ComptrollerInterface {
             uint96 marketPoolId,
             bool isBorrowAllowed
         );
+
+    function isFlashLoanPaused() external view returns (bool);
 }
 
 interface IVAIVault {

contracts/Comptroller/ComptrollerStorage.sol

@@ -324,4 +324,7 @@ contract ComptrollerV17Storage is ComptrollerV16Storage {
 contract ComptrollerV18Storage is ComptrollerV17Storage {
     /// @notice Mapping of accounts authorized to execute flash loans
     mapping(address => bool) public authorizedFlashLoan;
+
+    /// @notice Whether flash loans are paused system-wide
+    bool public flashLoanPaused;
 }

contracts/Comptroller/Diamond/facets/FlashLoanFacet.sol

@@ -18,6 +18,9 @@ import { ReentrancyGuardTransient } from "../../../Utils/ReentrancyGuardTransien
  *      The contract supports protocol fee collection and integrates with the Venus lending protocol.
  */
 contract FlashLoanFacet is IFlashLoanFacet, FacetBase, ReentrancyGuardTransient {
+    /// @notice Maximum number of assets that can be flash loaned in a single transaction
+    uint256 public constant MAX_FLASHLOAN_ASSETS = 200;
+
     /// @notice Emitted when the flash loan is successfully executed
     event FlashLoanExecuted(address indexed receiver, VToken[] assets, uint256[] amounts);
 
@@ -39,7 +42,9 @@ contract FlashLoanFacet is IFlashLoanFacet, FacetBase, ReentrancyGuardTransient
      * @param underlyingAmounts The amounts of each underlying assets to be loaned.
      * @param param The bytes passed in the executeOperation call.
      * @custom:error FlashLoanNotEnabled is thrown if the flash loan is not enabled for the asset.
+     * @custom:error FlashLoanPausedSystemWide is thrown if flash loans are paused system-wide.
      * @custom:error InvalidAmount is thrown if the requested amount is zero.
+     * @custom:error TooManyAssetsRequested is thrown if the number of requested assets exceeds the maximum limit.
      * @custom:error NoAssetsRequested is thrown if no assets are requested for the flash loan.
      * @custom:error InvalidFlashLoanParams is thrown if the flash loan params are invalid.
      * @custom:error MarketNotListed is thrown if the specified vToken market is not listed.
@@ -54,13 +59,23 @@ contract FlashLoanFacet is IFlashLoanFacet, FacetBase, ReentrancyGuardTransient
         uint256[] memory underlyingAmounts,
         bytes memory param
     ) external nonReentrant {
+        if (flashLoanPaused) {
+            revert FlashLoanPausedSystemWide();
+        }
+
+        ensureNonzeroAddress(onBehalf);
+
         uint256 len = vTokens.length;
         Market storage market;
 
         // vTokens array must not be empty
         if (len == 0) {
             revert NoAssetsRequested();
         }
+        // Add maximum array length check to prevent gas limit issues
+        if (len > MAX_FLASHLOAN_ASSETS) {
+            revert TooManyAssetsRequested(len, MAX_FLASHLOAN_ASSETS);
+        }
 
         // All arrays must have the same length and not be zero
         if (len != underlyingAmounts.length) {

contracts/Comptroller/Diamond/facets/SetterFacet.sol

@@ -125,6 +125,9 @@ contract SetterFacet is ISetterFacet, FacetBase {
     /// @notice Emitted when pool Fallback status is updated
     event PoolFallbackStatusUpdated(uint96 indexed poolId, bool oldStatus, bool newStatus);
 
+    /// @notice Emitted when flash loan pause status changes
+    event FlashLoanPauseChanged(bool oldPaused, bool newPaused);
+
     /**
      * @notice Compare two addresses to ensure they are different
      * @param oldAddress The original address to compare
@@ -640,6 +643,24 @@ contract SetterFacet is ISetterFacet, FacetBase {
         emit IsAccountFlashLoanWhitelisted(account, isWhiteListed);
     }
 
+    /**
+     * @notice Pause or unpause flash loans system-wide
+     * @param paused True to pause flash loans, false to unpause
+     * @custom:access Only Governance
+     * @custom:event Emits FlashLoanPauseChanged event
+     */
+    function setFlashLoanPaused(bool paused) external {
+        ensureAllowed("setFlashLoanPaused(bool)");
+
+        // Check if value is actually changing
+        if (flashLoanPaused == paused) {
+            return; // No change needed
+        }
+
+        emit FlashLoanPauseChanged(flashLoanPaused, paused);
+        flashLoanPaused = paused;
+    }
+
     /**
      * @notice Updates the label for a specific pool (excluding the Core Pool)
      * @param poolId ID of the pool to update

contracts/Comptroller/Diamond/interfaces/ISetterFacet.sol

@@ -103,4 +103,6 @@ interface ISetterFacet {
     function setPoolLabel(uint96 poolId, string calldata newLabel) external;
 
     function setAllowCorePoolFallback(uint96 poolId, bool allowFallback) external;
+
+    function setFlashLoanPaused(bool paused) external;
 }

contracts/Utils/ErrorReporter.sol

@@ -76,6 +76,12 @@ contract ComptrollerErrorReporter {
     /// @notice Thrown when the vToken market is not listed
     error MarketNotListed(address vToken);
 
+    /// @notice Thrown when flash loans are paused system-wide
+    error FlashLoanPausedSystemWide();
+
+    /// @notice Thrown when too many assets are requested in a single flash loan
+    error TooManyAssetsRequested(uint256 requested, uint256 maximum);
+
     enum Error {
         NO_ERROR,
         UNAUTHORIZED,

contracts/test/ComptrollerMock.sol

@@ -4,11 +4,12 @@ import "../Comptroller/Diamond/facets/MarketFacet.sol";
 import "../Comptroller/Diamond/facets/PolicyFacet.sol";
 import "../Comptroller/Diamond/facets/RewardFacet.sol";
 import "../Comptroller/Diamond/facets/SetterFacet.sol";
+import "../Comptroller/Diamond/facets/FlashLoanFacet.sol";
 import "../Comptroller/Unitroller.sol";
 
 // This contract contains all methods of Comptroller implementation in different facets at one place for testing purpose
 // This contract does not have diamond functionality(i.e delegate call to facets methods)
-contract ComptrollerMock is MarketFacet, PolicyFacet, RewardFacet, SetterFacet {
+contract ComptrollerMock is MarketFacet, PolicyFacet, RewardFacet, SetterFacet, FlashLoanFacet {
     constructor() {
         admin = msg.sender;
     }

tests/hardhat/Comptroller/Diamond/flashLoan.ts

@@ -62,6 +62,7 @@ const flashLoanTestFixture = async (): Promise<FlashLoanContractsFixture> => {
   const result = await deployDiamond("");
   const unitroller = result.unitroller;
   const comptroller = await ethers.getContractAt("ComptrollerMock", unitroller.address);
+
   const comptrollerLens = await ComptrollerLensFactory.deploy();
   await comptroller._setAccessControl(accessControlManager.address);
   await comptroller._setComptrollerLens(comptrollerLens.address);
@@ -185,6 +186,26 @@ describe("FlashLoan", async () => {
       await mockReceiverContract.deployed();
     });
 
+    it("Should revert flash loan when paused system-wide", async () => {
+      // Pause flash loans system-wide
+      await comptroller.setFlashLoanPaused(true);
+
+      // Verify pause status
+      expect(await comptroller.flashLoanPaused()).to.be.true;
+
+      // Should revert when paused
+      await expect(
+        mockReceiverContract
+          .connect(alice)
+          .requestFlashLoan(
+            [vTokenA.address, vTokenB.address],
+            [flashLoanAmount1, flashLoanAmount2],
+            mockReceiverContract.address,
+            "0x",
+          ),
+      ).to.be.revertedWithCustomError(comptroller, "FlashLoanPausedSystemWide");
+    });
+
     it("Should revert if flashLoan is not enabled", async () => {
       expect(await vTokenA.isFlashLoanEnabled()).to.be.false;
       expect(await vTokenB.isFlashLoanEnabled()).to.be.false;
@@ -210,6 +231,33 @@ describe("FlashLoan", async () => {
       );
     });
 
+    it("Should revert when onBehalf is address zero", async () => {
+      await expect(
+        comptroller.executeFlashLoan(
+          ethers.constants.AddressZero, // ❌ Zero address
+          mockReceiverContract.address,
+          [vTokenA.address],
+          [flashLoanAmount1],
+          "0x",
+        ),
+      ).to.be.revertedWith("can't be zero address");
+    });
+
+    it("Should revert when too many assets are requested", async () => {
+      await vTokenA.setFlashLoanEnabled(true);
+      // Create array with more assets than MAX_FLASHLOAN_ASSETS
+      const tooManyAssets = new Array(201).fill(vTokenA.address); // Assuming MAX is 200
+      const tooManyAmounts = new Array(201).fill(flashLoanAmount1);
+
+      await comptroller.setWhiteListFlashLoanAccount(alice.address, true);
+
+      await expect(
+        comptroller.executeFlashLoan(alice.address, mockReceiverContract.address, tooManyAssets, tooManyAmounts, "0x"),
+      )
+        .to.be.revertedWithCustomError(comptroller, "TooManyAssetsRequested")
+        .withArgs(201, 200);
+    });
+
     it("Should revert if the market is not listed", async () => {
       await vTokenC.setFlashLoanEnabled(true);
       expect(await vTokenC.isFlashLoanEnabled()).to.be.true;
@@ -314,6 +362,32 @@ describe("FlashLoan", async () => {
       ).to.be.revertedWithCustomError(comptroller, "ExecuteFlashLoanFailed");
     });
 
+    it("Should emit FlashLoanPauseChanged event when pause status changes", async () => {
+      // Test pausing
+      await expect(comptroller.setFlashLoanPaused(true))
+        .to.emit(comptroller, "FlashLoanPauseChanged")
+        .withArgs(false, true);
+
+      // Test unpausing
+      await expect(comptroller.setFlashLoanPaused(false))
+        .to.emit(comptroller, "FlashLoanPauseChanged")
+        .withArgs(true, false);
+    });
+
+    it("Should not emit event when setting same pause status", async () => {
+      // Flash loans are not paused by default
+      expect(await comptroller.flashLoanPaused()).to.be.false;
+
+      // Setting to false again should not emit event (no change)
+      await expect(comptroller.setFlashLoanPaused(false)).to.not.emit(comptroller, "FlashLoanPauseChanged");
+
+      // Pause first
+      await comptroller.setFlashLoanPaused(true);
+
+      // Setting to true again should not emit event (no change)
+      await expect(comptroller.setFlashLoanPaused(true)).to.not.emit(comptroller, "FlashLoanPauseChanged");
+    });
+
     it("User has not supplied in venus - Should not create debt position if receiver repays full amount + fee", async () => {
       await vTokenA.setFlashLoanEnabled(true);
       await vTokenB.setFlashLoanEnabled(true);