Merge pull request #744 from bosonprotocol/v2_3_0_findings

Remediate findings CHF-01M PHF-01M PIH-02M

contracts/interfaces/handlers/IBosonPauseHandler.sol

@@ -19,7 +19,6 @@ interface IBosonPauseHandler is IBosonPauseEvents {
      *
      * Reverts if:
      * - Caller does not have PAUSER role
-     * - A region is specified more than once
      *
      * @param _regions - an array of regions to pause. See: {BosonTypes.PausableRegion}
      */
@@ -33,7 +32,6 @@ interface IBosonPauseHandler is IBosonPauseEvents {
      * Reverts if:
      * - Caller does not have PAUSER role
      * - Protocol is not currently paused
-     * - A region is specified more than once
      *
      * @param _regions - an array of regions to pause. See: {BosonTypes.PausableRegion}
      */

contracts/protocol/facets/ConfigHandlerFacet.sol

@@ -43,8 +43,8 @@ contract ConfigHandlerFacet is IBosonConfigHandler, ProtocolBase {
         setBuyerEscalationDepositPercentage(_fees.buyerEscalationDepositPercentage);
         setMaxTotalOfferFeePercentage(_limits.maxTotalOfferFeePercentage);
         setMaxRoyaltyPecentage(_limits.maxRoyaltyPecentage);
-        setMinResolutionPeriod(_limits.minResolutionPeriod);
         setMaxResolutionPeriod(_limits.maxResolutionPeriod);
+        setMinResolutionPeriod(_limits.minResolutionPeriod);
         setMinDisputePeriod(_limits.minDisputePeriod);
 
         // Initialize protocol counters
@@ -428,7 +428,13 @@ contract ConfigHandlerFacet is IBosonConfigHandler, ProtocolBase {
         // Make sure _maxResolutionPeriod is greater than 0
         checkNonZero(_minResolutionPeriod);
 
-        protocolLimits().minResolutionPeriod = _minResolutionPeriod;
+        // cache protocol limits
+        ProtocolLib.ProtocolLimits storage limits = protocolLimits();
+
+        // Make sure _minResolutionPeriod is less than _maxResolutionPeriod
+        require(_minResolutionPeriod <= limits.maxResolutionPeriod, INVALID_RESOLUTION_PERIOD);
+
+        limits.minResolutionPeriod = _minResolutionPeriod;
         emit MinResolutionPeriodChanged(_minResolutionPeriod, msgSender());
     }
 
@@ -456,7 +462,13 @@ contract ConfigHandlerFacet is IBosonConfigHandler, ProtocolBase {
         // Make sure _maxResolutionPeriod is greater than 0
         checkNonZero(_maxResolutionPeriod);
 
-        protocolLimits().maxResolutionPeriod = _maxResolutionPeriod;
+        // cache protocol limits
+        ProtocolLib.ProtocolLimits storage limits = protocolLimits();
+
+        // Make sure _maxResolutionPeriod is greater than _minResolutionPeriod
+        require(_maxResolutionPeriod >= limits.minResolutionPeriod, INVALID_RESOLUTION_PERIOD);
+
+        limits.maxResolutionPeriod = _maxResolutionPeriod;
         emit MaxResolutionPeriodChanged(_maxResolutionPeriod, msgSender());
     }
 

contracts/protocol/facets/PauseHandlerFacet.sol

@@ -30,7 +30,6 @@ contract PauseHandlerFacet is ProtocolBase, IBosonPauseHandler {
      *
      * Reverts if:
      * - Caller does not have PAUSER role
-     * - A region is specified more than once
      *
      * @param _regions - an array of regions to pause. See: {BosonTypes.PausableRegion}
      */
@@ -49,7 +48,6 @@ contract PauseHandlerFacet is ProtocolBase, IBosonPauseHandler {
      * Reverts if:
      * - Caller does not have PAUSER role
      * - Protocol is not currently paused
-     * - A region is specified more than once
      */
     function unpause(BosonTypes.PausableRegion[] calldata _regions) external onlyRole(PAUSER) nonReentrant {
         // Cache protocol status for reference
@@ -105,9 +103,6 @@ contract PauseHandlerFacet is ProtocolBase, IBosonPauseHandler {
      *
      * Toggle all regions if none are specified.
      *
-     * Reverts if:
-     * - A region is specified more than once
-     *
      * @param _regions - an array of regions to pause/unpause. See: {BosonTypes.PausableRegion}
      * @param _pause - a boolean indicating whether to pause (true) or unpause (false)
      */

contracts/protocol/facets/ProtocolInitializationHandlerFacet.sol

@@ -48,7 +48,7 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
      * - For upgrade to v2.2.0:
      *   - If versions is set already
      *   - If _initializationData cannot be decoded to uin256
-     *   - If _initializationData is represents value 0
+     *   - If _initializationData is represents value
      *
      * @param _version - version of the protocol
      * @param _addresses - array of facet addresses to call initialize methods
@@ -143,10 +143,11 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
      * Reverts if:
      *  - Current version is not 2.2.1
      *  - There are already twins. This version adds a new mapping for twins which make it incompatible with previous versions.
-     *  - minResolutionPeriond is not present in _initializationData parameter
+     *  - minResolutionPeriod is not present in _initializationData parameter
      *  - length of seller creators does not match the length of seller ids
      *  - if some of seller creators is zero address
      *  - if some of seller ids does not bellong to a seller
+     *  - if minResolutionPeriod is greater than maxResolutionPeriod
      *
      * @param _initializationData - data representing uint256 _minResolutionPeriod, uint256[] memory sellerIds, address[] memory sellerCreators
      */
@@ -161,9 +162,15 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
             (uint256, uint256[], address[])
         );
 
+        // cache protocol limits
+        ProtocolLib.ProtocolLimits storage limits = protocolLimits();
+
+        // make sure _minResolutionPeriod is less than maxResolutionPeriod
+        require(limits.maxResolutionPeriod >= _minResolutionPeriod, INVALID_RESOLUTION_PERIOD);
+
         // Initialize limits.maxPremintedVouchers (configHandlerFacet initializer)
         require(_minResolutionPeriod != 0, VALUE_ZERO_NOT_ALLOWED);
-        protocolLimits().minResolutionPeriod = _minResolutionPeriod;
+        limits.minResolutionPeriod = _minResolutionPeriod;
         emit MinResolutionPeriodChanged(_minResolutionPeriod, msgSender());
 
         // Initialize sellerCreators

test/protocol/ConfigHandlerTest.js

@@ -1,7 +1,6 @@
 const { ethers } = require("hardhat");
 const { getSigners, getContractAt, ZeroAddress, parseUnits } = ethers;
 const { expect } = require("chai");
-
 const Role = require("../../scripts/domain/Role");
 const { getInterfaceIds } = require("../../scripts/config/supported-interfaces.js");
 const { RevertReasons } = require("../../scripts/config/revert-reasons.js");
@@ -718,6 +717,55 @@ describe("IBosonConfigHandler", function () {
         });
       });
 
+      context("👉 setMinResolutionPeriod()", async function () {
+        let minResolutionPeriod;
+        beforeEach(async function () {
+          // set new value
+          minResolutionPeriod = oneWeek;
+        });
+
+        it("should emit a MinResolutionPeriodChanged event", async function () {
+          // Set new resolution period
+          await expect(configHandler.connect(deployer).setMinResolutionPeriod(minResolutionPeriod))
+            .to.emit(configHandler, "MinResolutionPeriodChanged")
+            .withArgs(minResolutionPeriod, await deployer.getAddress());
+        });
+
+        it("should update state", async function () {
+          // Set new resolution period
+          await configHandler.connect(deployer).setMinResolutionPeriod(minResolutionPeriod);
+
+          // Verify that new value is stored
+          expect(await configHandler.connect(rando).getMinResolutionPeriod()).to.equal(minResolutionPeriod);
+        });
+
+        context("💔 Revert Reasons", async function () {
+          it("caller is not the admin", async function () {
+            // Attempt to set new value, expecting revert
+            await expect(configHandler.connect(rando).setMinResolutionPeriod(minResolutionPeriod)).to.revertedWith(
+              RevertReasons.ACCESS_DENIED
+            );
+          });
+
+          it("minResolutionPeriod is zero", async function () {
+            minResolutionPeriod = 0;
+            await expect(configHandler.connect(deployer).setMinResolutionPeriod(minResolutionPeriod)).to.revertedWith(
+              RevertReasons.VALUE_ZERO_NOT_ALLOWED
+            );
+          });
+
+          it("minResolutionPeriod is greater than maxResolutionPeriod", async function () {
+            const maxResolutionPeriod = oneMonth;
+            await configHandler.connect(deployer).setMaxResolutionPeriod(maxResolutionPeriod);
+
+            minResolutionPeriod = maxResolutionPeriod + 1n;
+            await expect(configHandler.connect(deployer).setMinResolutionPeriod(minResolutionPeriod)).to.revertedWith(
+              RevertReasons.INVALID_RESOLUTION_PERIOD
+            );
+          });
+        });
+      });
+
       context("👉 setMaxResolutionPeriod()", async function () {
         let maxResolutionPeriod;
         beforeEach(async function () {
@@ -754,6 +802,16 @@ describe("IBosonConfigHandler", function () {
               RevertReasons.VALUE_ZERO_NOT_ALLOWED
             );
           });
+
+          it("maxResolutionPeriod is less than minResolutionPeriod", async function () {
+            const minResolutionPeriod = oneWeek;
+            await configHandler.connect(deployer).setMinResolutionPeriod(minResolutionPeriod);
+
+            const maxResolutionPeriod = minResolutionPeriod - 1n;
+            await expect(configHandler.connect(deployer).setMaxResolutionPeriod(maxResolutionPeriod)).to.revertedWith(
+              RevertReasons.INVALID_RESOLUTION_PERIOD
+            );
+          });
         });
       });
 

test/protocol/OfferHandlerTest.js

@@ -737,6 +737,7 @@ describe("IBosonOfferHandler", function () {
         });
 
         it("Resolution period is greater than protocol max resolution period", async function () {
+          await configHandler.setMinResolutionPeriod(oneDay - 1n);
           // Set max resolution period to 1 day
           await configHandler.setMaxResolutionPeriod(oneDay); // 24 hours
 

test/protocol/ProtocolInitializationHandlerTest.js

@@ -18,7 +18,7 @@ const { mockTwin, mockSeller, mockAuthToken, mockVoucherInitValues } = require("
 const { deployProtocolDiamond } = require("../../scripts/util/deploy-protocol-diamond.js");
 const { deployAndCutFacets, deployProtocolFacets } = require("../../scripts/util/deploy-protocol-handler-facets");
 const { getInterfaceIds, interfaceImplementers } = require("../../scripts/config/supported-interfaces");
-const { maxPriorityFeePerGas, oneWeek } = require("../util/constants");
+const { maxPriorityFeePerGas, oneWeek, oneMonth } = require("../util/constants");
 
 const { getFees } = require("../../scripts/util/utils");
 const { getFacetAddCut, getFacetReplaceCut } = require("../../scripts/util/diamond-utils");
@@ -831,6 +831,27 @@ describe("ProtocolInitializationHandler", async function () {
         ).to.be.revertedWith(RevertReasons.VALUE_ZERO_NOT_ALLOWED);
       });
 
+      it("Min resolution period is greater than max resolution period", async function () {
+        version = "2.3.0";
+        console.log("oneMonth", oneMonth);
+        await configHandler.connect(deployer).setMaxResolutionPeriod(oneMonth);
+        minResolutionPeriod = oneMonth + 1n;
+        console.log("minResolutionPeriod", minResolutionPeriod);
+        initializationData = abiCoder.encode(["uint256", "uint256[]", "address[]"], [minResolutionPeriod, [], []]);
+        calldataProtocolInitialization = deployedProtocolInitializationHandlerFacet.interface.encodeFunctionData(
+          "initialize",
+          [encodeBytes32String(version), [], [], true, initializationData, [], []]
+        );
+
+        await expect(
+          diamondCutFacet.diamondCut(
+            [facetCut],
+            deployedProtocolInitializationHandlerFacetAddress,
+            calldataProtocolInitialization
+          )
+        ).to.be.revertedWith(RevertReasons.INVALID_RESOLUTION_PERIOD);
+      });
+
       it("sellerIds and sellerCreators length mismatch", async function () {
         version = "2.3.0";
         initializationData = abiCoder.encode(["uint256", "uint256[]", "address[]"], [minResolutionPeriod, [1], []]);