[Lines of code](https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L216) # Vulnerability details ## Impact This vulnerability allows recipients to reenter the Exchange contract, stealing remaining ETH. When a batch of orders are matched using the bulkExecute method a malicious recipient of assets may reenter the bulkExecute method, stealing ETH that would've gone to filling further orders in the batch and / or would've been returned to the original caller. Specifically the following type of recipients may exploit this vulnerability: * sellers who receive ETH for a sale * buyers receiving ER721 / ERC1155 tokens ## Proof of Concept The [bulkExecute method]() allows the caller to match multiple orders by passing a list of Execution structs which include both buy and sell side inputs of an order. The bulkExecute method is payable, accepting ETH from the sender which can subsequently be used to fill orders. In bulkExecute, reverts from failing order matches are [intentionally](https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/README.md?plain=1#L24) contained and skipped, continuing with the matching of the next order. Sellers who receive ETH as part of an order match ([L631](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L631>)) get forwarded all gas, allowing them to reenter the Exchange contract if they're a contract that does so upon its receive method being invoked. Similarly an NFT recipient could reenter upon getting sent a token via the execution delegate ([L663]()), which sends ERC721 tokens via the safeTransferFrom method ([L95]() the list of executions can be left empty as any call to _execute in the attacker's context would silently revert. The setupExecution modifier around the bulkExecute will [store the msg.value in remainingETH](), which is later [checked by the _returnDust method](), which is why at least 1 wei must be sent by the attacker. At the end of the bulkExecute method, [_returnDust is triggered](), sending the contract's [entire balance]() to the attacker, irrespective of the ETH the attacker originally sent. Returning to the original entry _execute and subsequently bulkExecute call contexts, order matching is resumed whereby every "internal" call to _execute will silently revert due to the [isInternal flag]() having been [set to false by the reentrant call](). ## Tools Used Manual review. ## Recommended Mitigation Steps To avoid someone triggering the _returnDust method during a running context move up the reentrancy guard from the internal _execute method to the public facing execute and bulkExecute methods. --- The text was updated successfully, but these errors were encountered: All reactions

Query Builder