[FLB-02M] Potential Gas Bombing Attack Vector (#897)

* Make price discovery client

* pricediscovery unit tests and fixes

* Unit tests pass

* support erc721 unsafe transfer

* Protocol can move the vouchers not the client

* PriceDiscoveryAddress part of protocol addresses

* ConfigHandler tests

* Initialize price discovery address during upgrade

* Price discovery client tests

* Prevent wrong tokenId transfer + natspec update

* Fix failing unit tests

* Remove unused variables and clean up code

* Fix failing unit tests

* WIP withdraw royalties

* Update existing unit tests

* Withdraw royalty recipient funds

* Fix failing unit tests

* Remove wrong comment

contracts/domain/BosonConstants.sol

@@ -21,9 +21,6 @@ uint256 constant ALL_REGIONS_MASK = (1 << (uint256(type(BosonTypes.PausableRegio
 uint256 constant NOT_ENTERED = 1;
 uint256 constant ENTERED = 2;
 
-// Seller related
-string constant DEFAULT_ROYALTY_RECIPIENT = "Treasury";
-
 // Twin handler
 uint256 constant SINGLE_TWIN_RESERVED_GAS = 160000;
 uint256 constant MINIMAL_RESIDUAL_GAS = 230000;

contracts/domain/BosonTypes.sol

@@ -114,6 +114,11 @@ contract BosonTypes {
         bool active;
     }
 
+    struct RoyaltyRecipient {
+        uint256 id;
+        address payable wallet;
+    }
+
     struct DisputeResolver {
         uint256 id;
         uint256 escalationResponsePeriod;
@@ -330,17 +335,16 @@ contract BosonTypes {
         Wrapper // Side is not relevant from the protocol perspective
     }
 
-    struct RoyaltyRecipient {
-        address wallet;
-        uint256 minRoyaltyPercentage;
-        string externalId;
-    }
-
     struct RoyaltyInfo {
         address payable[] recipients;
         uint256[] bps;
     }
 
+    struct RoyaltyRecipientInfo {
+        address payable wallet;
+        uint256 minRoyaltyPercentage;
+    }
+
     struct PremintParameters {
         uint256 reservedRangeLength;
         address to;

contracts/interfaces/events/IBosonAccountEvents.sol

@@ -32,7 +32,7 @@ interface IBosonAccountEvents {
     );
     event RoyaltyRecipientsChanged(
         uint256 indexed sellerId,
-        BosonTypes.RoyaltyRecipient[] royaltyRecipients,
+        BosonTypes.RoyaltyRecipientInfo[] royaltyRecipients,
         address indexed executedBy
     );
     event BuyerCreated(uint256 indexed buyerId, BosonTypes.Buyer buyer, address indexed executedBy);

contracts/interfaces/handlers/IBosonAccountHandler.sol

@@ -10,7 +10,7 @@ import { IBosonAccountEvents } from "../events/IBosonAccountEvents.sol";
  *
  * @notice Handles creation, update, retrieval of accounts within the protocol.
  *
- * The ERC-165 identifier for this interface is: 0x079a9d3b
+ * The ERC-165 identifier for this interface is: 0x0757010c
  */
 interface IBosonAccountHandler is IBosonAccountEvents, BosonErrors {
     /**
@@ -165,7 +165,7 @@ interface IBosonAccountHandler is IBosonAccountEvents, BosonErrors {
      */
     function addRoyaltyRecipients(
         uint256 _sellerId,
-        BosonTypes.RoyaltyRecipient[] calldata _royaltyRecipients
+        BosonTypes.RoyaltyRecipientInfo[] calldata _royaltyRecipients
     ) external;
 
     /**
@@ -191,7 +191,7 @@ interface IBosonAccountHandler is IBosonAccountEvents, BosonErrors {
     function updateRoyaltyRecipients(
         uint256 _sellerId,
         uint256[] calldata _royaltyRecipientIds,
-        BosonTypes.RoyaltyRecipient[] calldata _royaltyRecipients
+        BosonTypes.RoyaltyRecipientInfo[] calldata _royaltyRecipients
     ) external;
 
     /**

contracts/mock/BuyerContract.sol

@@ -2,6 +2,7 @@
 pragma solidity 0.8.22;
 
 import { IERC721Receiver } from "../interfaces/IERC721Receiver.sol";
+import { IERC721 } from "../interfaces/IERC721.sol";
 
 /**
  * @title BuyerContract
@@ -32,9 +33,28 @@ contract BuyerContract is IERC721Receiver {
         address from,
         uint256 tokenId,
         bytes calldata data
-    ) external returns (bytes4) {
+    ) external virtual returns (bytes4) {
         if (failType == FailType.Revert) revert("BuyerContract: revert");
         if (failType == FailType.ReturnWrongSelector) return 0x12345678;
         return IERC721Receiver.onERC721Received.selector;
     }
 }
+
+contract BuyerContractMalicious is BuyerContract {
+    /**
+     * @dev Return wrong selector to test revert
+     */
+    function onERC721Received(
+        address operator,
+        address from,
+        uint256 tokenId,
+        bytes calldata data
+    ) external override returns (bytes4) {
+        (address protocolAddress, address bosonVoucher, uint256 tokenIdA) = abi.decode(
+            data,
+            (address, address, uint256)
+        );
+
+        IERC721(bosonVoucher).safeTransferFrom(protocolAddress, address(this), tokenIdA);
+    }
+}

contracts/protocol/bases/OfferBase.sol

@@ -392,7 +392,7 @@ contract OfferBase is ProtocolBase, IBosonOfferEvents {
     ) internal view {
         if (_royaltyInfo.recipients.length != _royaltyInfo.bps.length) revert ArrayLengthMismatch();
 
-        RoyaltyRecipient[] storage royaltyRecipients = _lookups.royaltyRecipientsBySeller[_sellerId];
+        RoyaltyRecipientInfo[] storage royaltyRecipients = _lookups.royaltyRecipientsBySeller[_sellerId];
 
         uint256 totalRoyalties;
         for (uint256 i = 0; i < _royaltyInfo.recipients.length; ) {

contracts/protocol/bases/SellerBase.sol

@@ -95,10 +95,9 @@ contract SellerBase is ProtocolBase, IBosonAccountEvents {
         // Set treasury as the default royalty recipient
         if (_voucherInitValues.royaltyPercentage > protocolLimits().maxRoyaltyPercentage)
             revert InvalidRoyaltyPercentage();
-        RoyaltyRecipient[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[sellerId];
-        RoyaltyRecipient storage defaultRoyaltyRecipient = royaltyRecipients.push();
+        RoyaltyRecipientInfo[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[sellerId];
+        RoyaltyRecipientInfo storage defaultRoyaltyRecipient = royaltyRecipients.push();
         // We don't store the defaultRoyaltyRecipient.wallet, since it's always the trasury
-        // We don't store the defaultRoyaltyRecipient.externalId, since the default recipient is always the treasury
         defaultRoyaltyRecipient.minRoyaltyPercentage = _voucherInitValues.royaltyPercentage;
 
         // Calculate seller salt and check that it is unique
@@ -243,14 +242,7 @@ contract SellerBase is ProtocolBase, IBosonAccountEvents {
      */
     function fetchRoyaltyRecipients(
         uint256 _sellerId
-    ) internal view returns (RoyaltyRecipient[] memory royaltyRecipients) {
-        royaltyRecipients = protocolLookups().royaltyRecipientsBySeller[_sellerId];
-
-        // If the seller did not change the default recipient name, return the default name
-        // royaltyRecipients[0] exists because the default recipient is always present
-        if (bytes(royaltyRecipients[0].externalId).length == 0) {
-            royaltyRecipients[0].externalId = DEFAULT_ROYALTY_RECIPIENT;
-        }
-        return royaltyRecipients;
+    ) internal view returns (RoyaltyRecipientInfo[] memory royaltyRecipients) {
+        return protocolLookups().royaltyRecipientsBySeller[_sellerId];
     }
 }

contracts/protocol/facets/FundsHandlerFacet.sol

@@ -75,7 +75,7 @@ contract FundsHandlerFacet is IBosonFundsHandler, ProtocolBase {
     }
 
     /**
-     * @notice Withdraws the specified funds. Can be called for seller, buyer or agent.
+     * @notice Withdraws the specified funds. Can be called for seller, buyer, agent or royalty recipient.
      *
      * Emits FundsWithdrawn event if successful.
      *
@@ -96,34 +96,7 @@ contract FundsHandlerFacet is IBosonFundsHandler, ProtocolBase {
         address[] calldata _tokenList,
         uint256[] calldata _tokenAmounts
     ) external override fundsNotPaused nonReentrant {
-        address payable sender = payable(msgSender());
-
-        // Address that will receive the funds
-        address payable destinationAddress;
-
-        // First check if the caller is a buyer
-        (bool exists, uint256 callerId) = getBuyerIdByWallet(sender);
-        if (exists && callerId == _entityId) {
-            // Caller is a buyer
-            destinationAddress = sender;
-        } else {
-            // Check if the caller is an assistant
-            (exists, callerId) = getSellerIdByAssistant(sender);
-            if (exists && callerId == _entityId) {
-                // Caller is an assistant. In this case funds are transferred to the treasury address
-                (, Seller storage seller, ) = fetchSeller(callerId);
-                destinationAddress = seller.treasury;
-            } else {
-                (exists, callerId) = getAgentIdByWallet(sender);
-                if (exists && callerId == _entityId) {
-                    // Caller is an agent
-                    destinationAddress = sender;
-                } else {
-                    // In this branch, caller is neither buyer, assistant or agent or does not match the _entityId
-                    revert NotAuthorized();
-                }
-            }
-        }
+        address payable destinationAddress = getDestinationAddress(_entityId);
 
         withdrawFundsInternal(destinationAddress, _entityId, _tokenList, _tokenAmounts);
     }
@@ -324,4 +297,47 @@ contract FundsHandlerFacet is IBosonFundsHandler, ProtocolBase {
             }
         }
     }
+
+    /**
+     * @notice For a given entity id, it returns the address, where the funds are withdrawn.
+     *
+     * Reverts if:
+     * - Caller is not associated with the entity id
+     *
+     * @param _entityId - id of entity for which funds should be withdrawn
+     * @return destinationAddress - address where the funds are withdrawn
+     */
+    function getDestinationAddress(uint256 _entityId) internal view returns (address payable destinationAddress) {
+        address payable sender = payable(msgSender());
+
+        // First check if the caller is a buyer
+        (bool exists, uint256 callerId) = getBuyerIdByWallet(sender);
+        if (exists && callerId == _entityId) {
+            // Caller is a buyer
+            return sender;
+        }
+
+        // Check if the caller is an assistant
+        (exists, callerId) = getSellerIdByAssistant(sender);
+        if (exists && callerId == _entityId) {
+            // Caller is an assistant. In this case funds are transferred to the treasury address
+            (, Seller storage seller, ) = fetchSeller(callerId);
+            return seller.treasury;
+        }
+
+        (exists, callerId) = getAgentIdByWallet(sender);
+        if (exists && callerId == _entityId) {
+            // Caller is an agent
+            return sender;
+        }
+
+        callerId = protocolLookups().royaltyRecipientIdByWallet[sender];
+        if (callerId > 0 && callerId == _entityId) {
+            // Caller is a royalty recipient
+            return sender;
+        }
+
+        // In this branch, caller is neither buyer, assistant or agent or does not match the _entityId
+        revert NotAuthorized();
+    }
 }

contracts/protocol/facets/ProtocolInitializationHandlerFacet.sol

@@ -217,7 +217,7 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
         for (uint256 i = 0; i < _royaltyPercentages.length; i++) {
             // Populate sellers' Royalty Recipients
             for (uint256 j = 0; j < _sellerIds[i].length; j++) {
-                RoyaltyRecipient storage defaultRoyaltyRecipient = lookups
+                RoyaltyRecipientInfo storage defaultRoyaltyRecipient = lookups
                     .royaltyRecipientsBySeller[_sellerIds[i][j]]
                     .push();
                 defaultRoyaltyRecipient.minRoyaltyPercentage = _royaltyPercentages[i];

contracts/protocol/facets/SellerHandlerFacet.sol

@@ -160,7 +160,7 @@ contract SellerHandlerFacet is SellerBase {
                 uint256 royaltyRecipientId = royaltyRecipientIndexBySellerAndRecipient[_seller.treasury];
 
                 if (royaltyRecipientId != 0) {
-                    RoyaltyRecipient[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_seller.id];
+                    RoyaltyRecipientInfo[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_seller.id];
 
                     // If the new treasury is already a royalty recipient, remove it
                     royaltyRecipientId--; // royaltyRecipientId is 1-based, so we need to decrement it to get the index
@@ -474,11 +474,11 @@ contract SellerHandlerFacet is SellerBase {
      *  - some royalty percentage is above the limit
      *
      * @param _sellerId - seller id
-     * @param _royaltyRecipients - list of royalty recipients to add
+     * @param _royaltyRecipients - list of royalty recipients to add, including minimal royalty percentage
      */
     function addRoyaltyRecipients(
         uint256 _sellerId,
-        RoyaltyRecipient[] calldata _royaltyRecipients
+        RoyaltyRecipientInfo[] calldata _royaltyRecipients
     ) external sellersNotPaused nonReentrant {
         // Cache protocol lookups and sender for reference
         ProtocolLib.ProtocolLookups storage lookups = protocolLookups();
@@ -487,26 +487,27 @@ contract SellerHandlerFacet is SellerBase {
         (Seller storage seller, address sender) = validateAdminStatus(lookups, _sellerId);
         address treasury = seller.treasury;
 
-        RoyaltyRecipient[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
+        RoyaltyRecipientInfo[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
+        uint256 maxRoyaltyPercentage = protocolLimits().maxRoyaltyPercentage;
         uint256 royaltyRecipientsStorageLength = royaltyRecipients.length + 1;
         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 ||
                 _royaltyRecipients[i].wallet == address(0) ||
                 royaltyRecipientIndexByRecipient[_royaltyRecipients[i].wallet] != 0
             ) revert RecipientNotUnique();
 
-            if (_royaltyRecipients[i].minRoyaltyPercentage > protocolLimits().maxRoyaltyPercentage)
-                revert InvalidRoyaltyPercentage();
+            if (_royaltyRecipients[i].minRoyaltyPercentage > maxRoyaltyPercentage) revert InvalidRoyaltyPercentage();
 
             royaltyRecipients.push(_royaltyRecipients[i]);
             royaltyRecipientIndexByRecipient[_royaltyRecipients[i].wallet] = royaltyRecipientsStorageLength + i;
 
+            createRoyaltyRecipientAccount(_royaltyRecipients[i].wallet);
+
             unchecked {
                 i++;
             }
@@ -515,6 +516,23 @@ contract SellerHandlerFacet is SellerBase {
         emit RoyaltyRecipientsChanged(_sellerId, fetchRoyaltyRecipients(_sellerId), sender);
     }
 
+    function createRoyaltyRecipientAccount(address payable _royaltyRecipient) internal {
+        mapping(address => uint256) storage royaltyRecipientIdByWallet = protocolLookups().royaltyRecipientIdByWallet;
+        // If account exists, do nothing
+        if (royaltyRecipientIdByWallet[_royaltyRecipient] > 0) {
+            return;
+        }
+
+        uint256 royaltyRecipientId = protocolCounters().nextAccountId++;
+
+        protocolEntities().royaltyRecipients[royaltyRecipientId] = RoyaltyRecipient({
+            id: royaltyRecipientId,
+            wallet: _royaltyRecipient
+        });
+
+        royaltyRecipientIdByWallet[_royaltyRecipient] = royaltyRecipientId;
+    }
+
     /**
      * @notice Updates seller's royalty recipients.
      *
@@ -538,7 +556,7 @@ contract SellerHandlerFacet is SellerBase {
     function updateRoyaltyRecipients(
         uint256 _sellerId,
         uint256[] calldata _royaltyRecipientIds,
-        RoyaltyRecipient[] calldata _royaltyRecipients
+        RoyaltyRecipientInfo[] calldata _royaltyRecipients
     ) external sellersNotPaused nonReentrant {
         // Cache protocol lookups and sender for reference
         ProtocolLib.ProtocolLookups storage lookups = protocolLookups();
@@ -553,8 +571,7 @@ contract SellerHandlerFacet is SellerBase {
         uint256 royaltyRecipientIdsLength = _royaltyRecipientIds.length;
         if (royaltyRecipientIdsLength != _royaltyRecipients.length) revert ArrayLengthMismatch();
 
-        RoyaltyRecipient[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
-
+        RoyaltyRecipientInfo[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
         uint256 royaltyRecipientsLength = royaltyRecipients.length;
         for (uint256 i = 0; i < royaltyRecipientIdsLength; ) {
             uint256 royaltyRecipientId = _royaltyRecipientIds[i];
@@ -587,6 +604,8 @@ contract SellerHandlerFacet is SellerBase {
 
             royaltyRecipients[royaltyRecipientId] = _royaltyRecipients[i];
 
+            createRoyaltyRecipientAccount(_royaltyRecipients[i].wallet);
+
             unchecked {
                 i++;
             }
@@ -622,7 +641,7 @@ contract SellerHandlerFacet is SellerBase {
         // Make sure admin is the caller and get the sender's address
         (, address sender) = validateAdminStatus(lookups, _sellerId);
 
-        RoyaltyRecipient[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
+        RoyaltyRecipientInfo[] storage royaltyRecipients = lookups.royaltyRecipientsBySeller[_sellerId];
 
         // We loop from the end of the array to ensure correct ids are removed
         // _royaltyRecipients must be sorted in ascending order
@@ -852,7 +871,7 @@ contract SellerHandlerFacet is SellerBase {
      */
     function getRoyaltyRecipients(
         uint256 _sellerId
-    ) external view returns (RoyaltyRecipient[] memory royaltyRecipients) {
+    ) external view returns (RoyaltyRecipientInfo[] memory royaltyRecipients) {
         return fetchRoyaltyRecipients(_sellerId);
     }
 

contracts/protocol/facets/SequentialCommitHandlerFacet.sol

@@ -137,7 +137,6 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
                 // Get the price, originally paid by the reseller
                 uint256 oldPrice;
                 unchecked {
-                    // Get price paid by current buyer
                     uint256 len = exchangeCosts.length;
                     oldPrice = len == 0 ? offer.price : exchangeCosts[len - 1].price;
                 }