Skip to content

Commit

Permalink
Add missing reentrancy guard (#844)
Browse files Browse the repository at this point in the history
* Add missing reentrancy guard

* remove duplicate tests

* remove unnecessary reenctrancy protection
  • Loading branch information
zajck authored Dec 11, 2023
1 parent d96fcfd commit 33fbbbd
Show file tree
Hide file tree
Showing 3 changed files with 3,630 additions and 3,867 deletions.
13 changes: 8 additions & 5 deletions contracts/protocol/facets/ExchangeHandlerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,10 @@ contract ExchangeHandlerFacet is DisputeBase, BuyerBase, IBosonExchangeHandler,
* - Voucher has expired
* - New buyer's existing account is deactivated
*
* N.B. This method is not protected with reentrancy guard, since it clashes with price discovery flows.
* Given that it does not rely on msgSender() for authentication and it does not modify it, it is safe to leave it unprotected.
* In case of reentrancy the only inconvenience that could happen is that `executedBy` field in `VoucherTransferred` event would not be set correctly.
*
* @param _tokenId - the voucher id
* @param _newBuyer - the address of the new buyer
*/
Expand Down Expand Up @@ -605,6 +609,10 @@ contract ExchangeHandlerFacet is DisputeBase, BuyerBase, IBosonExchangeHandler,
* - Offer price is discovery, transaction is not starting from protocol nor seller is _from address
* - Any reason that ExchangeHandler commitToOfferInternal reverts. See ExchangeHandler.commitToOfferInternal
*
* N.B. This method is not protected with reentrancy guard, since it clashes with price discovery flows.
* Given that it does not rely on msgSender() for authentication and it does not modify it, it is safe to leave it unprotected.
* In case of reentrancy the only inconvenience that could happen is that `executedBy` field in `BuyerCommitted` event would not be set correctly.
*
* @param _tokenId - the voucher id
* @param _to - the receiver address
* @param _from - the address of current owner
Expand All @@ -620,11 +628,6 @@ contract ExchangeHandlerFacet is DisputeBase, BuyerBase, IBosonExchangeHandler,
// Cache protocol status for reference
ProtocolLib.ProtocolStatus storage ps = protocolStatus();

// Make sure that protocol is not reentered
// Cannot use modifier `nonReentrant` since it also changes reentrancyStatus to `ENTERED`
// This would break the flow since the protocol should be allowed to re-enter in this case.
if (ps.reentrancyStatus == ENTERED) revert ReentrancyGuard();

// Derive the offer id
uint256 offerId = _tokenId >> 128;

Expand Down
2 changes: 1 addition & 1 deletion contracts/protocol/facets/PriceDiscoveryHandlerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ contract PriceDiscoveryHandlerFacet is IBosonPriceDiscoveryHandler, PriceDiscove
address payable _buyer,
uint256 _tokenIdOrOfferId,
PriceDiscovery calldata _priceDiscovery
) external payable override exchangesNotPaused buyersNotPaused {
) external payable override exchangesNotPaused buyersNotPaused nonReentrant {
// Make sure buyer address is not zero address
if (_buyer == address(0)) revert InvalidAddress();

Expand Down
Loading

0 comments on commit 33fbbbd

Please sign in to comment.