Merge branch 'main' into audit_v2_4_0_bbe_01c

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%

contracts/interfaces/handlers/IBosonSequentialCommitHandler.sol

@@ -37,7 +37,7 @@ interface IBosonSequentialCommitHandler is BosonErrors, IBosonExchangeEvents, IB
      *   - Calling transferFrom on token fails for some reason (e.g. protocol is not approved to transfer)
      *   - Received ERC20 token amount differs from the expected value
      *   - Protocol does not receive the voucher
-     *   - Transfer of voucher to the buyer fails for some reasong (e.g. buyer is contract that doesn't accept voucher)
+     *   - Transfer of voucher to the buyer fails for some reason (e.g. buyer is contract that doesn't accept voucher)
      *   - Reseller did not approve protocol to transfer exchange token in escrow
      * - Call to price discovery contract fails
      * - Protocol fee and royalties combined exceed the secondary price

contracts/mock/BuyerContract.sol

@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity 0.8.22;
+
+import { IERC721Receiver } from "../interfaces/IERC721Receiver.sol";
+
+/**
+ * @title BuyerContract
+ *
+ * @notice Contract that acts as a buyer for testing purposes
+ */
+contract BuyerContract is IERC721Receiver {
+    enum FailType {
+        None,
+        Revert,
+        ReturnWrongSelector
+    }
+
+    FailType public failType;
+
+    /**
+     * @dev Set fail type
+     */
+    function setFailType(FailType _failType) external {
+        failType = _failType;
+    }
+
+    /**
+     * @dev Return wrong selector to test revert
+     */
+    function onERC721Received(
+        address operator,
+        address from,
+        uint256 tokenId,
+        bytes calldata data
+    ) external returns (bytes4) {
+        if (failType == FailType.Revert) revert("BuyerContract: revert");
+        if (failType == FailType.ReturnWrongSelector) return 0x12345678;
+        return IERC721Receiver.onERC721Received.selector;
+    }
+}

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);
+    }
+}

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);
+    }
+}

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;
     }
 
     /**
@@ -147,7 +140,7 @@ contract PriceDiscoveryBase is ProtocolBase {
             if (_bosonVoucher.ownerOf(_tokenId) != address(this)) revert VoucherNotReceived();
 
             // Transfer voucher to buyer
-            _bosonVoucher.transferFrom(address(this), _buyer, _tokenId);
+            _bosonVoucher.safeTransferFrom(address(this), _buyer, _tokenId);
         }
 
         uint256 overchargedAmount = _priceDiscovery.price - actualPrice;

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;

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;

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

contracts/protocol/facets/SellerHandlerFacet.sol

@@ -172,6 +172,8 @@ contract SellerHandlerFacet is SellerBase {
                             1;
                     }
                     royaltyRecipients.pop();
+
+                    delete royaltyRecipientIndexBySellerAndRecipient[_seller.treasury];
                 }
 
                 // Update treasury
@@ -487,19 +489,21 @@ contract SellerHandlerFacet is SellerBase {
 
         RoyaltyRecipient[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
         for (uint256 i = 0; i < _royaltyRecipients.length; ) {
+            // Cache storage pointer to avoid multiple lookups
+            mapping(address => uint256) storage royaltyRecipientIndexByRecipient = lookups
+                .royaltyRecipientIndexBySellerAndRecipient[_sellerId];
+
             // No uniqueness check for externalIds since they are not used in the protocol
             if (
                 _royaltyRecipients[i].wallet == treasury ||
-                lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][_royaltyRecipients[i].wallet] != 0
+                royaltyRecipientIndexByRecipient[_royaltyRecipients[i].wallet] != 0
             ) revert RecipientNotUnique();
 
             if (_royaltyRecipients[i].minRoyaltyPercentage > protocolLimits().maxRoyaltyPercentage)
                 revert InvalidRoyaltyPercentage();
 
             royaltyRecipients.push(_royaltyRecipients[i]);
-            lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][
-                _royaltyRecipients[i].wallet
-            ] = royaltyRecipients.length; // can be optimized to use counter instead of array length
+            royaltyRecipientIndexByRecipient[_royaltyRecipients[i].wallet] = royaltyRecipients.length; // can be optimized to use counter instead of array length
 
             unchecked {
                 i++;
@@ -559,18 +563,16 @@ contract SellerHandlerFacet is SellerBase {
             if (royaltyRecipientId == 0) {
                 if (_royaltyRecipients[i].wallet != address(0)) revert WrongDefaultRecipient();
             } else {
-                uint256 royaltyRecipientIndex = lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][
-                    _royaltyRecipients[i].wallet
-                ];
+                // Cache storage pointer to avoid multiple lookups
+                mapping(address => uint256) storage royaltyRecipientIndexByRecipient = lookups
+                    .royaltyRecipientIndexBySellerAndRecipient[_sellerId];
+
+                uint256 royaltyRecipientIndex = royaltyRecipientIndexByRecipient[_royaltyRecipients[i].wallet];
 
                 if (royaltyRecipientIndex == 0) {
                     // update index
-                    lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][_royaltyRecipients[i].wallet] =
-                        royaltyRecipientId +
-                        1;
-                    delete lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][
-                        royaltyRecipients[royaltyRecipientId].wallet
-                    ];
+                    royaltyRecipientIndexByRecipient[_royaltyRecipients[i].wallet] = royaltyRecipientId + 1;
+                    delete royaltyRecipientIndexByRecipient[royaltyRecipients[royaltyRecipientId].wallet];
                 } else {
                     if (royaltyRecipientIndex - 1 != royaltyRecipientId) revert RecipientNotUnique();
                 }
@@ -628,15 +630,15 @@ contract SellerHandlerFacet is SellerBase {
             if (royaltyRecipientId >= previousId) revert RoyaltyRecipientIdsNotSorted(); // this also ensures that royaltyRecipientId will never be out of bounds
             if (royaltyRecipientId == 0) revert CannotRemoveDefaultRecipient();
 
-            delete lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][
-                royaltyRecipients[royaltyRecipientId].wallet
-            ];
+            // Cache storage pointer to avoid multiple lookups
+            mapping(address => uint256) storage royaltyRecipientIndexByRecipient = lookups
+                .royaltyRecipientIndexBySellerAndRecipient[_sellerId];
+
+            delete royaltyRecipientIndexByRecipient[royaltyRecipients[royaltyRecipientId].wallet];
 
             if (royaltyRecipientId != lastRoyaltyRecipientsId) {
                 royaltyRecipients[royaltyRecipientId] = royaltyRecipients[lastRoyaltyRecipientsId];
-                lookups.royaltyRecipientIndexBySellerAndRecipient[_sellerId][
-                    royaltyRecipients[royaltyRecipientId].wallet
-                ] = royaltyRecipientId;
+                royaltyRecipientIndexByRecipient[royaltyRecipients[royaltyRecipientId].wallet] = royaltyRecipientId;
             }
             royaltyRecipients.pop();
             lastRoyaltyRecipientsId--; // will never underflow. Even if all non-default royalty recipients are removed, default recipient will remain

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.
@@ -65,7 +60,7 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
      *   - Calling transferFrom on token fails for some reason (e.g. protocol is not approved to transfer)
      *   - Received ERC20 token amount differs from the expected value
      *   - Protocol does not receive the voucher
-     *   - Transfer of voucher to the buyer fails for some reasong (e.g. buyer is contract that doesn't accept voucher)
+     *   - Transfer of voucher to the buyer fails for some reason (e.g. buyer is contract that doesn't accept voucher)
      *   - Reseller did not approve protocol to transfer exchange token in escrow
      * - Call to price discovery contract fails
      * - Protocol fee and royalties combined exceed the secondary price
@@ -139,9 +134,12 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
                     revert FeeAmountTooHigh();
                 }
 
-                // Get price paid by current buyer
-                uint256 len = exchangeCosts.length;
-                uint256 currentPrice = len == 0 ? offer.price : exchangeCosts[len - 1].price;
+                uint256 currentPrice;
+                unchecked {
+                    // Get price paid by current buyer
+                    uint256 len = exchangeCosts.length;
+                    currentPrice = len == 0 ? offer.price : exchangeCosts[len - 1].price;
+                }
 
                 // Calculate the minimal amount to be kept in the escrow
                 escrowAmount =
@@ -156,7 +154,10 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
             }
 
             // Make sure enough get escrowed
-            payout = exchangeCost.price - escrowAmount;
+            // Escrow amount is guaranteed to be less than or equal to price
+            unchecked {
+                payout = exchangeCost.price - escrowAmount;
+            }
 
             if (_priceDiscovery.side == Side.Ask) {
                 if (escrowAmount > 0) {
@@ -179,7 +180,7 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
                 }
 
                 if (payout > 0) {
-                    FundsLib.transferFundsFromProtocol(exchangeToken, payable(seller), payout); // also emits FundsWithdrawn
+                    FundsLib.transferFundsFromProtocol(exchangeToken, payable(seller), payout);
                 }
             }
         }

contracts/protocol/libs/FundsLib.sol

@@ -342,7 +342,6 @@ library FundsLib {
 
         // protocolFee and sellerRoyalties can be multiplied by effectivePriceMultiplier just at the end
         protocolFee = (protocolFee * effectivePriceMultiplier) / 10000;
-        sellerRoyalties = sellerRoyalties;
     }
 
     /**

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.

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",
@@ -174,6 +175,8 @@ exports.RevertReasons = {
   SAFE_ERC20_LOW_LEVEL_CALL: "SafeERC20: low-level call failed",
   SAFE_ERC20_OPERATION_FAILED: "SafeERC20: ERC20 operation did not succeed",
   INITIALIZABLE_ALREADY_INITIALIZED: "Initializable: contract is already initialized",
+  ERC721_NON_RECEIVER: "ERC721: transfer to non ERC721Receiver implementer",
+  BUYER_CONTRACT_REVERT: "BuyerContract: revert",
 
   // Meta-Transactions related
   NONCE_USED_ALREADY: "NonceUsedAlready",
@@ -190,6 +193,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",