Summary

While reviewing the DeliveryPlace.sol contract, I identified a reentrancy vulnerability within the closeBidOffer and closeBidTaker functions. These functions are designed to close bid offers and takers, respectively, but the way they interact with external contracts leaves them exposed to potential reentrancy attacks. Specifically, the functions make external calls to other contracts before updating internal state variables or emitting events, which could allow an attacker to exploit the contract and cause significant damage.

Vulnerability Details

in closeBidOffer(address _offer) Function

Vulnerable Code:

The closeBidOffer function is used to close a bid offer, calculate the refund amount based on the offer’s parameters, and then interact with external contracts to update token balances and offer status.

This is an external call to the TokenManager contract. The contract's state is not fully updated before this call, leaving the contract in a potentially inconsistent state that an attacker could exploit by re-entering the function.

This is another external call, this time to the PerMarkets contract. Again, the state of the contract is not fully updated before this call is made

The event is emitted after the external calls. Best practice is to emit events before making external calls to reduce the reentrancy attack surface.

Attacker Could Exploit This

Step 1: The attacker could create a malicious contract that interacts with this function. During the execution of closeBidOffer, when tokenManager.addTokenBalance is called, the attacker's contract could use a fallback function to re-enter closeBidOffer.

Step 2: By re-entering the function, the attacker could trigger another refund before the status is updated to Settled, potentially causing multiple refunds or other unintended state changes.

Step 3: This manipulation could lead to draining the contract of funds or leaving the contract in an inconsistent state, affecting the integrity of the marketplace.

in closeBidTaker(address _stock) Function

The closeBidTaker function is designed to finalize a bid taker, calculate any collateral fees, and update token balances and stock status accordingly.

This is an external call to TokenManager before the function’s internal state is fully updated. This leaves the contract exposed to reentrancy.

This second external call happens before the contract's internal state has been finalized and before any event is emitted, leaving the contract vulnerable to reentrancy.

Yet another external call made before the event is emitted. This sequence further exposes the contract to a potential reentrancy attack.

The event is emitted after all external calls, which is a less secure practice and increases the vulnerability to reentrancy attacks.

Attacker Could Exploit This:

Step 1: The attacker could deploy a contract that interacts with closeBidTaker. When the function calls tokenManager.addTokenBalance, the attacker’s contract could trigger a fallback function to re-enter closeBidTaker.

Step 2: By re-entering, the attacker could manipulate the state to issue multiple payments or other unintended actions before the internal state is finalized.

Step 3: This can lead to unauthorized fund withdrawals, inconsistent stock statuses, or multiple unintended token transfers.

Impact

The primary impact of these vulnerabilities is that they allow an attacker to perform reentrancy attacks, which could lead to the contract being drained of funds or left in an inconsistent state. Such exploits could cause significant financial losses and compromise the integrity of the entire marketplace.

Tools Used

Manual Code Review

Recommendations

Implement ReentrancyGuard:
Use OpenZeppelin’s ReentrancyGuard in these functions to prevent reentrancy attacks. This will ensure that once a function is entered, it cannot be re-entered until it completes.

Example Implementation

Reorder Function Logic:
Reorder the function code so that all state changes and event emissions occur before any external calls. This practice minimizes the risk of reentrancy attacks.

Example Fix for closeBidOffer: