From 67603dcd3afded4a32db283b4106aeaf751cfe6c Mon Sep 17 00:00:00 2001 From: Ana Julia Date: Mon, 22 May 2023 13:38:52 -0300 Subject: [PATCH] Review changes --- .../handlers/IBosonExchangeHandler.sol | 28 +++++++++ contracts/protocol/bases/BundleBase.sol | 8 +-- contracts/protocol/bases/GroupBase.sol | 30 +++------- contracts/protocol/bases/OfferBase.sol | 8 +-- contracts/protocol/bases/ProtocolBase.sol | 23 ++++++++ .../protocol/facets/ExchangeHandlerFacet.sol | 3 + .../protocol/facets/OfferHandlerFacet.sol | 18 +----- test/protocol/ExchangeHandlerTest.js | 57 +++++++++---------- 8 files changed, 95 insertions(+), 80 deletions(-) diff --git a/contracts/interfaces/handlers/IBosonExchangeHandler.sol b/contracts/interfaces/handlers/IBosonExchangeHandler.sol index 1840925d0..14e54b7d0 100644 --- a/contracts/interfaces/handlers/IBosonExchangeHandler.sol +++ b/contracts/interfaces/handlers/IBosonExchangeHandler.sol @@ -43,6 +43,34 @@ interface IBosonExchangeHandler is IBosonExchangeEvents, IBosonFundsLibEvents, I */ function commitToOffer(address payable _buyer, uint256 _offerId) external payable; + /** + * @notice Commits to an conditional offer (first step of an exchange). + * + * Emits a BuyerCommitted event if successful. + * Issues a voucher to the buyer address. + * + * Reverts if: + * - The exchanges region of protocol is paused + * - The buyers region of protocol is paused + * - OfferId is invalid + * - Offer has been voided + * - Offer has expired + * - Offer is not yet available for commits + * - Offer's quantity available is zero + * - Buyer address is zero + * - Buyer account is inactive + * - Conditional commit requirements not met or already used + * - Offer price is in native token and caller does not send enough + * - Offer price is in some ERC20 token and caller also sends native currency + * - Contract at token address does not support ERC20 function transferFrom + * - 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 + * - Seller has less funds available than sellerDeposit + * + * @param _buyer - the buyer's address (caller can commit on behalf of a buyer) + * @param _offerId - the id of the offer to commit to + * @param _tokenId - the id of the token to use for the conditional commit + */ function commitToConditionalOffer(address payable _buyer, uint256 _offerId, uint256 _tokenId) external payable; /** diff --git a/contracts/protocol/bases/BundleBase.sol b/contracts/protocol/bases/BundleBase.sol index 1e7b7c0c3..298c3804c 100644 --- a/contracts/protocol/bases/BundleBase.sol +++ b/contracts/protocol/bases/BundleBase.sol @@ -171,13 +171,7 @@ contract BundleBase is ProtocolBase, IBosonBundleEvents { uint256 _offerId ) internal view returns (uint256 offersTotalQuantity) { // make sure all offers exist and belong to the seller - Offer storage offer = getValidOffer(_offerId); - - // Get seller, we assume seller exists if offer exists - (, Seller storage seller, ) = fetchSeller(offer.sellerId); - - // Caller must be seller's assistant address - require(seller.assistant == msgSender(), NOT_ASSISTANT); + (Offer storage offer, ) = getValidOfferAndSeller(_offerId); // Unchecked because we're handling overflow below unchecked { diff --git a/contracts/protocol/bases/GroupBase.sol b/contracts/protocol/bases/GroupBase.sol index 353ef1247..9980435da 100644 --- a/contracts/protocol/bases/GroupBase.sol +++ b/contracts/protocol/bases/GroupBase.sol @@ -49,13 +49,7 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents { for (uint256 i = 0; i < _group.offerIds.length; i++) { // make sure offer exists and belongs to the seller - Offer storage offer = getValidOffer(_group.offerIds[i]); - - // Get seller, we assume seller exists if offer exists - (, Seller storage seller, ) = fetchSeller(offer.sellerId); - - // Caller must be seller's assistant address - require(seller.assistant == msgSender(), NOT_ASSISTANT); + getValidOfferAndSeller(_group.offerIds[i]); // Offer should not belong to another group already (bool exist, ) = getGroupIdByOffer(_group.offerIds[i]); @@ -108,11 +102,11 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents { * * A invalid condition is one that fits any of the following criteria: * - EvaluationMethod.None: any fields different from 0 - * - EvaluationMethod.Threshold: token address, maxCommits or threshold is zero and length is not zero + * - EvaluationMethod.Threshold: token address, maxCommits or threshold is zero or length is not zero * - EvaluationMethod.SpecificToken: * - tokenType is FungibleToken * - tokenType is NonFungibleToken and threshold is not zero - * - tokenType is NonFunbileToken, tokenId is not zero and length is zero + * - tokenId is not zero and length is zero * - tokenType is MultiToken and threshold is zero * - maxCommits is zero * - token address is zero @@ -139,14 +133,14 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents { _condition.maxCommits > 0 && _condition.tokenType != TokenType.FungibleToken); // FungibleToken not allowed for SpecificToken + if (_condition.tokenId != 0) { + // SpecificToken with tokenId should have length + valid = valid && _condition.length > 0; + } + // SpecificToken with NonFungibleToken should not have threshold if (_condition.tokenType == TokenType.NonFungibleToken) { valid = valid && _condition.threshold == 0; - - if (_condition.tokenId != 0) { - // SpecificToken with NonFungibleToken and tokenId should have length - valid = valid && _condition.length > 0; - } } else { // SpecificToken with MultiToken should have threshold valid = valid && _condition.threshold > 0; @@ -185,14 +179,8 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents { for (uint256 i = 0; i < _offerIds.length; i++) { uint256 offerId = _offerIds[i]; - // make sure offer exist and belong to the seller - Offer storage offer = getValidOffer(offerId); - - // Get seller, we assume seller exists if offer exists - (, Seller storage seller, ) = fetchSeller(offer.sellerId); - // Caller must be seller's assistant address - require(seller.assistant == msgSender(), NOT_ASSISTANT); + getValidOfferAndSeller(offerId); // Offer should not belong to another group already (bool exist, ) = getGroupIdByOffer(offerId); diff --git a/contracts/protocol/bases/OfferBase.sol b/contracts/protocol/bases/OfferBase.sol index ee50226dd..93fab6810 100644 --- a/contracts/protocol/bases/OfferBase.sol +++ b/contracts/protocol/bases/OfferBase.sol @@ -300,13 +300,7 @@ contract OfferBase is ProtocolBase, IBosonOfferEvents { address _to ) internal offersNotPaused exchangesNotPaused { // Get offer, make sure the caller is the assistant - Offer storage offer = getValidOffer(_offerId); - - // Get seller, we assume seller exists if offer exists - (, Seller storage seller, ) = fetchSeller(offer.sellerId); - - // Caller must be seller's assistant address - require(seller.assistant == msgSender(), NOT_ASSISTANT); + (Offer storage offer, ) = getValidOfferAndSeller(_offerId); // Prevent reservation of an empty range require(_length > 0, INVALID_RANGE_LENGTH); diff --git a/contracts/protocol/bases/ProtocolBase.sol b/contracts/protocol/bases/ProtocolBase.sol index 7686a775f..e5ee5e6a8 100644 --- a/contracts/protocol/bases/ProtocolBase.sol +++ b/contracts/protocol/bases/ProtocolBase.sol @@ -535,6 +535,29 @@ abstract contract ProtocolBase is PausableBase, ReentrancyGuardBase { require(!offer.voided, OFFER_HAS_BEEN_VOIDED); } + /** + * @notice Gets offer and seller from protocol storage + * + * Reverts if: + * - Offer does not exist + * - Offer already voided + * - Seller assistant is not the caller + * + * @param _offerId - the id of the offer to check + */ + function getValidOfferAndSeller( + uint256 _offerId + ) internal view returns (Offer storage offer, Seller storage seller) { + // Get offer + offer = getValidOffer(_offerId); + + // Get seller, we assume seller exists if offer exists + (, seller, ) = fetchSeller(offer.sellerId); + + // Caller must be seller's assistant address + require(seller.assistant == msgSender(), NOT_ASSISTANT); + } + /** * @notice Gets the bundle id for a given offer id. * diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index 8073fb635..14814c114 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -119,6 +119,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { * * @param _buyer - the buyer's address (caller can commit on behalf of a buyer) * @param _offerId - the id of the offer to commit to + * @param _tokenId - the id of the token to use for the conditional commit */ function commitToConditionalOffer( address payable _buyer, @@ -139,6 +140,8 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Get the condition Condition storage condition = fetchCondition(groupId); + require(condition.method != EvaluationMethod.None, GROUP_HAS_NO_CONDITION); + bool allow = authorizeCommit(_buyer, condition, groupId, _tokenId); require(allow, CANNOT_COMMIT); diff --git a/contracts/protocol/facets/OfferHandlerFacet.sol b/contracts/protocol/facets/OfferHandlerFacet.sol index 89b02b8c3..b72a94169 100644 --- a/contracts/protocol/facets/OfferHandlerFacet.sol +++ b/contracts/protocol/facets/OfferHandlerFacet.sol @@ -163,14 +163,8 @@ contract OfferHandlerFacet is IBosonOfferHandler, OfferBase { * @param _offerId - the id of the offer to void */ function voidOffer(uint256 _offerId) public override offersNotPaused nonReentrant { - // Get offer, make sure the caller is the assistant - Offer storage offer = getValidOffer(_offerId); - - // Get seller, we assume seller exists if offer exists - (, Seller storage seller, ) = fetchSeller(offer.sellerId); - - // Caller must be seller's assistant address - require(seller.assistant == msgSender(), NOT_ASSISTANT); + // Get offer. Make sure caller is assistant + (Offer storage offer, ) = getValidOfferAndSeller(_offerId); // Void the offer offer.voided = true; @@ -220,13 +214,7 @@ contract OfferHandlerFacet is IBosonOfferHandler, OfferBase { */ function extendOffer(uint256 _offerId, uint256 _validUntilDate) public override offersNotPaused nonReentrant { // Make sure the caller is the assistant, offer exists and is not voided - Offer storage offer = getValidOffer(_offerId); - - // Get seller, we assume seller exists if offer exists - (, Seller storage seller, ) = fetchSeller(offer.sellerId); - - // Caller must be seller's assistant address - require(seller.assistant == msgSender(), NOT_ASSISTANT); + (Offer storage offer, ) = getValidOfferAndSeller(_offerId); // Fetch the offer dates OfferDates storage offerDates = fetchOfferDates(_offerId); diff --git a/test/protocol/ExchangeHandlerTest.js b/test/protocol/ExchangeHandlerTest.js index 409ccac70..90dd68a34 100644 --- a/test/protocol/ExchangeHandlerTest.js +++ b/test/protocol/ExchangeHandlerTest.js @@ -1553,36 +1553,6 @@ describe("IBosonExchangeHandler", function () { }); }); - context("✋ Group without condition", async function () { - let tokenId = "0"; - beforeEach(async function () { - // Create a new offer - await offerHandler - .connect(assistant) - .createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId); - - // Required constructor params for Group - groupId = "1"; - offerIds = [(++offerId).toString()]; - - // Create Condition - condition = mockCondition({ method: EvaluationMethod.None, threshold: "0", maxCommits: "0" }); - expect(condition.isValid()).to.be.true; - - // Create Group - group = new Group(groupId, seller.id, offerIds); - expect(group.isValid()).is.true; - await groupHandler.connect(assistant).createGroup(group, condition); - }); - - it("should emit a BuyerCommitted event", async function () { - // Commit to offer. - await expect( - exchangeHandler.connect(buyer).commitToConditionalOffer(buyer.address, offerId, tokenId, { value: price }) - ).to.emit(exchangeHandler, "BuyerCommitted"); - }); - }); - context("💔 Revert Reasons", async function () { let tokenId; @@ -1717,6 +1687,33 @@ describe("IBosonExchangeHandler", function () { exchangeHandler.connect(buyer).commitToConditionalOffer(buyer.address, offerId, tokenId, { value: price }) ).to.revertedWith(RevertReasons.OFFER_SOLD_OUT); }); + + it("Group without condition ", async function () { + let tokenId = "0"; + + // Create a new offer + await offerHandler + .connect(assistant) + .createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId); + + // Required constructor params for Group + groupId = "1"; + offerIds = [(++offerId).toString()]; + + // Create Condition + condition = mockCondition({ method: EvaluationMethod.None, threshold: "0", maxCommits: "0" }); + expect(condition.isValid()).to.be.true; + + // Create Group + group = new Group(groupId, seller.id, offerIds); + expect(group.isValid()).is.true; + await groupHandler.connect(assistant).createGroup(group, condition); + + // Commit to offer. + await expect( + exchangeHandler.connect(buyer).commitToConditionalOffer(buyer.address, offerId, tokenId, { value: price }) + ).to.revertedWith(RevertReasons.GROUP_HAS_NO_CONDITION); + }); }); });