From 634c838adf3c3cb341c0b5d2ba6b71f212dd7b6e Mon Sep 17 00:00:00 2001 From: Klemen <64400885+zajck@users.noreply.github.com> Date: Tue, 9 Jan 2024 11:27:38 +0100 Subject: [PATCH 1/2] PDB-01S] Unutilized Contract Member (#875) * Remove unused variable from contracts * Update scripts --- contracts/protocol/bases/PriceDiscoveryBase.sol | 9 +-------- contracts/protocol/facets/PriceDiscoveryHandlerFacet.sol | 7 +------ .../protocol/facets/SequentialCommitHandlerFacet.sol | 7 +------ scripts/config/facet-deploy.js | 4 ++-- 4 files changed, 5 insertions(+), 22 deletions(-) diff --git a/contracts/protocol/bases/PriceDiscoveryBase.sol b/contracts/protocol/bases/PriceDiscoveryBase.sol index 6b3fbbd02..2b3f50356 100644 --- a/contracts/protocol/bases/PriceDiscoveryBase.sol +++ b/contracts/protocol/bases/PriceDiscoveryBase.sol @@ -21,24 +21,17 @@ contract PriceDiscoveryBase is ProtocolBase { using SafeERC20 for IERC20; IWrappedNative internal immutable wNative; - uint256 private immutable EXCHANGE_ID_2_2_0; // solhint-disable-line /** * @notice * For offers with native exchange token, it is expected the the price discovery contracts will * operate with wrapped native token. Set the address of the wrapped native token in the constructor. * - * After v2.2.0, token ids are derived from offerId and exchangeId. - * EXCHANGE_ID_2_2_0 is the first exchange id to use for 2.2.0. - * Set EXCHANGE_ID_2_2_0 in the constructor. - * * @param _wNative - the address of the wrapped native token - * @param _firstExchangeId2_2_0 - the first exchange id to use for 2.2.0 */ //solhint-disable-next-line - constructor(address _wNative, uint256 _firstExchangeId2_2_0) { + constructor(address _wNative) { wNative = IWrappedNative(_wNative); - EXCHANGE_ID_2_2_0 = _firstExchangeId2_2_0; } /** diff --git a/contracts/protocol/facets/PriceDiscoveryHandlerFacet.sol b/contracts/protocol/facets/PriceDiscoveryHandlerFacet.sol index a248e79d6..b4fc00eaa 100644 --- a/contracts/protocol/facets/PriceDiscoveryHandlerFacet.sol +++ b/contracts/protocol/facets/PriceDiscoveryHandlerFacet.sol @@ -29,15 +29,10 @@ contract PriceDiscoveryHandlerFacet is IBosonPriceDiscoveryHandler, PriceDiscove * For offers with native exchange token, it is expected the the price discovery contracts will * operate with wrapped native token. Set the address of the wrapped native token in the constructor. * - * After v2.2.0, token ids are derived from offerId and exchangeId. - * EXCHANGE_ID_2_2_0 is the first exchange id to use for 2.2.0. - * Set EXCHANGE_ID_2_2_0 in the constructor. - * * @param _wNative - the address of the wrapped native token - * @param _firstExchangeId2_2_0 - the first exchange id to use for 2.2.0 */ //solhint-disable-next-line - constructor(address _wNative, uint256 _firstExchangeId2_2_0) PriceDiscoveryBase(_wNative, _firstExchangeId2_2_0) {} + constructor(address _wNative) PriceDiscoveryBase(_wNative) {} /** * @notice Facet Initializer diff --git a/contracts/protocol/facets/SequentialCommitHandlerFacet.sol b/contracts/protocol/facets/SequentialCommitHandlerFacet.sol index c1b502389..6b4b6d047 100644 --- a/contracts/protocol/facets/SequentialCommitHandlerFacet.sol +++ b/contracts/protocol/facets/SequentialCommitHandlerFacet.sol @@ -24,15 +24,10 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis * For offers with native exchange token, it is expected the the price discovery contracts will * operate with wrapped native token. Set the address of the wrapped native token in the constructor. * - * After v2.2.0, token ids are derived from offerId and exchangeId. - * EXCHANGE_ID_2_2_0 is the first exchange id to use for 2.2.0. - * Set EXCHANGE_ID_2_2_0 in the constructor. - * * @param _wNative - the address of the wrapped native token - * @param _firstExchangeId2_2_0 - the first exchange id to use for 2.2.0 */ //solhint-disable-next-line - constructor(address _wNative, uint256 _firstExchangeId2_2_0) PriceDiscoveryBase(_wNative, _firstExchangeId2_2_0) {} + constructor(address _wNative) PriceDiscoveryBase(_wNative) {} /** * @notice Initializes facet. diff --git a/scripts/config/facet-deploy.js b/scripts/config/facet-deploy.js index 7426e80fe..5efa5b8b4 100644 --- a/scripts/config/facet-deploy.js +++ b/scripts/config/facet-deploy.js @@ -80,11 +80,11 @@ async function getFacets(config) { facetArgs["ExchangeHandlerFacet"] = { init: [], constructorArgs: [protocolConfig.EXCHANGE_ID_2_2_0[network]] }; facetArgs["SequentialCommitHandlerFacet"] = { init: [], - constructorArgs: [protocolConfig.WrappedNative[network], protocolConfig.EXCHANGE_ID_2_2_0[network]], + constructorArgs: [protocolConfig.WrappedNative[network]], }; facetArgs["PriceDiscoveryHandlerFacet"] = { init: [], - constructorArgs: [protocolConfig.WrappedNative[network], protocolConfig.EXCHANGE_ID_2_2_0[network]], + constructorArgs: [protocolConfig.WrappedNative[network]], }; // metaTransactionsHandlerFacet initializer arguments. From 3892e757d33861b4e7f1a9b3f710a3c4c14173b1 Mon Sep 17 00:00:00 2001 From: Klemen <64400885+zajck@users.noreply.github.com> Date: Tue, 9 Jan 2024 11:38:55 +0100 Subject: [PATCH 2/2] [FLB-01M] Potentially Incorrect Dispute Case Handling (#877) * Revert if invalid final state * Test revert reasons --- contracts/domain/BosonErrors.sol | 4 ++ contracts/mock/MockDisputeHandlerFacet.sol | 23 ++++++++++ contracts/mock/MockExchangeHandlerFacet.sol | 22 +++++++++ .../protocol/facets/DisputeHandlerFacet.sol | 3 ++ .../protocol/facets/ExchangeHandlerFacet.sol | 10 ++--- scripts/config/revert-reasons.js | 2 + test/protocol/DisputeHandlerTest.js | 45 ++++++++++++++++++- test/protocol/ExchangeHandlerTest.js | 41 +++++++++++++++++ 8 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 contracts/mock/MockDisputeHandlerFacet.sol diff --git a/contracts/domain/BosonErrors.sol b/contracts/domain/BosonErrors.sol index 4457ce6b2..9122d1a9e 100644 --- a/contracts/domain/BosonErrors.sol +++ b/contracts/domain/BosonErrors.sol @@ -214,6 +214,8 @@ interface BosonErrors { error ExchangeAlreadyExists(); // Range length is 0, is more than quantity available or it would cause an overflow error InvalidRangeLength(); + // Exchange is being finalized into an invalid state + error InvalidTargeExchangeState(); // Twin related // Twin does not exist @@ -300,6 +302,8 @@ interface BosonErrors { error InvalidDisputeTimeout(); // Absolute zero offers cannot be escalated error EscalationNotAllowed(); + // Dispute is being finalized into an invalid state + error InvalidTargeDisputeState(); // Config related // Percentage exceeds 100% diff --git a/contracts/mock/MockDisputeHandlerFacet.sol b/contracts/mock/MockDisputeHandlerFacet.sol new file mode 100644 index 000000000..00fc7068a --- /dev/null +++ b/contracts/mock/MockDisputeHandlerFacet.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity 0.8.22; + +import { DisputeHandlerFacet } from "../protocol/facets/DisputeHandlerFacet.sol"; + +/** + * @title TestDisputeHandlerFacet + * + * @notice Extended DisputeHandlerFacet with additional external functions for testing + */ +contract TestDisputeHandlerFacet is DisputeHandlerFacet { + /** + * @notice Test function to test invalid final dispute state + * + * @param _exchangeId - the id of the associated exchange + * @param _targetState - target final state + */ + function finalizeDispute(uint256 _exchangeId, DisputeState _targetState) external { + (, Exchange storage exchange) = fetchExchange(_exchangeId); + (, Dispute storage dispute, DisputeDates storage disputeDates) = fetchDispute(_exchangeId); + finalizeDispute(_exchangeId, exchange, dispute, disputeDates, _targetState, 1000); + } +} diff --git a/contracts/mock/MockExchangeHandlerFacet.sol b/contracts/mock/MockExchangeHandlerFacet.sol index 289746124..a4e508745 100644 --- a/contracts/mock/MockExchangeHandlerFacet.sol +++ b/contracts/mock/MockExchangeHandlerFacet.sol @@ -7,6 +7,7 @@ import { DisputeBase } from "../protocol/bases/DisputeBase.sol"; import { FundsLib } from "../protocol/libs/FundsLib.sol"; import "../domain/BosonConstants.sol"; import "@openzeppelin/contracts/utils/Address.sol"; +import { ExchangeHandlerFacet } from "../protocol/facets/ExchangeHandlerFacet.sol"; /** * @title MockExchangeHandlerFacet @@ -537,3 +538,24 @@ contract MockExchangeHandlerFacetWithDefect is MockExchangeHandlerFacet { emit VoucherCanceled2(exchange.offerId, _exchangeId, msgSender()); } } + +/** + * @title TestExchangeHandlerFacet + * + * @notice Extended ExchangeHandlerFacet with additional external functions for testing + */ +contract TestExchangeHandlerFacet is ExchangeHandlerFacet { + //solhint-disable-next-line + constructor(uint256 _firstExchangeId2_2_0) ExchangeHandlerFacet(_firstExchangeId2_2_0) {} + + /** + * @notice Test function to test invalid final exchange state + * + * @param _exchangeId - the id of the exchange to finalize + * @param _targetState - the target state to which the exchange should be transitioned + */ + function finalizeExchange(uint256 _exchangeId, ExchangeState _targetState) external { + (, Exchange storage exchange) = fetchExchange(_exchangeId); + finalizeExchange(exchange, _targetState); + } +} diff --git a/contracts/protocol/facets/DisputeHandlerFacet.sol b/contracts/protocol/facets/DisputeHandlerFacet.sol index 8a4ce2c3c..f84e2a15c 100644 --- a/contracts/protocol/facets/DisputeHandlerFacet.sol +++ b/contracts/protocol/facets/DisputeHandlerFacet.sol @@ -439,6 +439,9 @@ contract DisputeHandlerFacet is DisputeBase, IBosonDisputeHandler { DisputeState _targetState, uint256 _buyerPercent ) internal disputesNotPaused { + if (_targetState == DisputeState.Escalated || _targetState == DisputeState.Resolving) + revert InvalidTargeDisputeState(); + // update dispute and exchange _disputeDates.finalized = block.timestamp; _dispute.state = _targetState; diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index 4fa9a9f63..a1afba797 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -947,11 +947,11 @@ contract ExchangeHandlerFacet is DisputeBase, BuyerBase, IBosonExchangeHandler, */ function finalizeExchange(Exchange storage _exchange, ExchangeState _targetState) internal { // Make sure target state is a final state - require( - _targetState == ExchangeState.Completed || - _targetState == ExchangeState.Revoked || - _targetState == ExchangeState.Canceled - ); + if ( + _targetState != ExchangeState.Completed && + _targetState != ExchangeState.Revoked && + _targetState != ExchangeState.Canceled + ) revert InvalidTargeExchangeState(); // Set the exchange state to the target state _exchange.state = _targetState; diff --git a/scripts/config/revert-reasons.js b/scripts/config/revert-reasons.js index f8bb341a7..086a61bcd 100644 --- a/scripts/config/revert-reasons.js +++ b/scripts/config/revert-reasons.js @@ -134,6 +134,7 @@ exports.RevertReasons = { EXCHANGE_IS_NOT_IN_A_FINAL_STATE: "ExchangeIsNotInAFinalState", INVALID_RANGE_LENGTH: "InvalidRangeLength", EXCHANGE_ALREADY_EXISTS: "ExchangeAlreadyExists", + INVALID_TARGET_EXCHANGE_STATE: "InvalidTargeExchangeState", // Voucher related EXCHANGE_ID_IN_RESERVED_RANGE: "ExchangeIdInReservedRange", @@ -190,6 +191,7 @@ exports.RevertReasons = { DISPUTE_STILL_VALID: "DisputeStillValid", INVALID_DISPUTE_TIMEOUT: "InvalidDisputeTimeout", ESCALATION_NOT_ALLOWED: "EscalationNotAllowed", + INVALID_TARGET_DISPUTE_STATE: "InvalidTargeDisputeState", // Config related FEE_PERCENTAGE_INVALID: "InvalidFeePercentage", diff --git a/test/protocol/DisputeHandlerTest.js b/test/protocol/DisputeHandlerTest.js index abfe2c01d..105ec6e2a 100644 --- a/test/protocol/DisputeHandlerTest.js +++ b/test/protocol/DisputeHandlerTest.js @@ -1,5 +1,5 @@ const { ethers } = require("hardhat"); -const { ZeroAddress, provider, zeroPadBytes, MaxUint256, parseUnits, getContractAt } = ethers; +const { ZeroAddress, provider, zeroPadBytes, MaxUint256, parseUnits, getContractAt, id } = ethers; const { expect, assert } = require("chai"); const Exchange = require("../../scripts/domain/Exchange"); const Dispute = require("../../scripts/domain/Dispute"); @@ -29,6 +29,7 @@ const { mockBuyer, accountId, } = require("../util/mock"); +const { FacetCutAction } = require("../../scripts/util/diamond-utils.js"); /** * Test the Boson Dispute Handler interface @@ -2580,4 +2581,46 @@ describe("IBosonDisputeHandler", function () { }); }); }); + + // Internal functions, tested with TestDisputeHandlerFacet + context("📋 Internal Dispute Handler Methods", async function () { + let testDisputeHandler; + beforeEach(async function () { + // Deploy test facet and cut the test functions + const TestDisputeHandlerFacet = await ethers.getContractFactory("TestDisputeHandlerFacet"); + const testDisputeHandlerFacet = await TestDisputeHandlerFacet.deploy(); + await testDisputeHandlerFacet.waitForDeployment(); + + const protocolDiamondAddress = await disputeHandler.getAddress(); + const cutFacetViaDiamond = await getContractAt("DiamondCutFacet", protocolDiamondAddress); + + // Define the facet cut + const facetCuts = [ + { + facetAddress: await testDisputeHandlerFacet.getAddress(), + action: FacetCutAction.Add, + functionSelectors: [id("finalizeDispute(uint256,uint8)").slice(0, 10)], + }, + ]; + + // Send the DiamondCut transaction + await cutFacetViaDiamond.diamondCut(facetCuts, ZeroAddress, "0x"); + + testDisputeHandler = await getContractAt("TestDisputeHandlerFacet", protocolDiamondAddress); + }); + + context("👉 finalizeDispute()", async function () { + const invalidFinalStates = ["Resolving", "Escalated"]; + + invalidFinalStates.forEach((finalState) => { + it(`final state is ${finalState}`, async function () { + const exchangeId = 1; + + await expect( + testDisputeHandler.finalizeDispute(exchangeId, DisputeState[finalState]) + ).to.revertedWithCustomError(bosonErrors, RevertReasons.INVALID_TARGET_DISPUTE_STATE); + }); + }); + }); + }); }); diff --git a/test/protocol/ExchangeHandlerTest.js b/test/protocol/ExchangeHandlerTest.js index 613a053e6..595dbba01 100644 --- a/test/protocol/ExchangeHandlerTest.js +++ b/test/protocol/ExchangeHandlerTest.js @@ -8132,4 +8132,45 @@ describe("IBosonExchangeHandler", function () { }); }); }); + + // Internal functions, tested with TestExchangeHandlerFacet + context("📋 Internal Exchange Handler Methods", async function () { + let testExchangeHandler; + beforeEach(async function () { + // Deploy test facet and cut the test functions + const TestExchangeHandlerFacet = await ethers.getContractFactory("TestExchangeHandlerFacet"); + const testExchangeHandlerFacet = await TestExchangeHandlerFacet.deploy(0); + await testExchangeHandlerFacet.waitForDeployment(); + + const cutFacetViaDiamond = await getContractAt("DiamondCutFacet", protocolDiamondAddress); + + // Define the facet cut + const facetCuts = [ + { + facetAddress: await testExchangeHandlerFacet.getAddress(), + action: FacetCutAction.Add, + functionSelectors: [id("finalizeExchange(uint256,uint8)").slice(0, 10)], + }, + ]; + + // Send the DiamondCut transaction + await cutFacetViaDiamond.connect(deployer).diamondCut(facetCuts, ZeroAddress, "0x"); + + testExchangeHandler = await getContractAt("TestExchangeHandlerFacet", protocolDiamondAddress); + }); + + context("👉 finalizeExchange()", async function () { + const invalidFinalStates = ["Committed", "Redeemed", "Disputed"]; + + invalidFinalStates.forEach((finalState) => { + it(`final state is ${finalState}`, async function () { + const exchangeId = 1; + + await expect( + testExchangeHandler.finalizeExchange(exchangeId, ExchangeState[finalState]) + ).to.revertedWithCustomError(bosonErrors, RevertReasons.INVALID_TARGET_EXCHANGE_STATE); + }); + }); + }); + }); });