Lines of code
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L154-L158&gt;
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168-L172&gt;
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L212-L227&gt;

Vulnerability details

Impact

The funds (ETH) that users sent along with the bulkExecute may be stolen.

Proof of Concept

When a buyer send a bulkExecute() tx with msg.value > 0 (with order of buying token with eth), the sent ETH may be stolen if the tx contains a malicious selling order which can trigger a reentry attack.

Here are the details of the reentry attack:

1. Buyer B calls bulkExecute() with a list of executions(order pairs) and some ETH. Assume v1 = remainingETH = msg.value, we have balanceOf(Exchange) >= v1.
2. When _execute() one of the executing which is malicious, triggle a call to a deployed malicious contract X. We can know that v2 = remainingETH >= v1.
3. The malicious contract X will call bulkExecute() of the Exchange with at least 1 wei and an empty list of executions. And this call with send all (balanceOf(Exchange) >= v2 + 1wei) ETH to contract X by _returnDust().
4. When X returns, we will have remainingETH == 0 and balanceOf(Exchange) ==0 now.

The invocation of a malicious contract in step 2 above may occur under the following circumstances and may be considered constructed maliciously

1. Contains a sell order whose collection contract is malicious: the logic in step 3 can be executed in _executeTokenTransfer().
2. Contains a sell order with malicious fee recipient: the reentry attack happens when fee is sent to the malicious recipient who is the malicious contract X.

In addition, when the Buyer calls execute() with a msg.value exceeds the actual price, the buyer may also lose the remaining funds by a reentry attack.

Tools Used

VS Code

Recommended Mitigation Steps

This problem is mainly due to the fact that _execute() is reentrancy protected and setupExecution() is not.

Recommended: remove reentrancyGuard from _execute() and add it to external functions (execute() and bulkExecute()) to make setupExecution reentrancy protected.

Here is the diff:

diff --git a/contracts/Exchange.sol b/contracts/Exchange.sol
index ec27b1d..f3fa09b 100644
--- a/contracts/Exchange.sol
+++ b/contracts/Exchange.sol
@@ -155,6 +155,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         external
         payable
         whenOpen
+        reentrancyGuard
         setupExecution
     {
         _execute(sell, buy);
@@ -169,6 +170,7 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
         external
         payable
         whenOpen
+        reentrancyGuard
         setupExecution
     {
         /*
@@ -234,7 +236,6 @@ contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, U
     function _execute(Input calldata sell, Input calldata buy)
         public
         payable
-        reentrancyGuard
         internalCall
     {
         require(sell.order.side == Side.Sell);

---

The text was updated successfully, but these errors were encountered:

All reactions