Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Maker can call `PreMarkets::closeOffer` on existing offer even there's taker, causing taker's fund to be stuck in the protocol.

4lifemen

Summary

The PreMarkets::closeOffer function allows makers to close offers with active takers, causing the takers' funds to be stuck in the protocol. This issue arises from the createTaker function not updating the offer status from Virgin to Ongoing after a taker is created.

Vulnerability Details

The createTaker function did not update the offer status from Virgin after a taker createTaker. This allows closeOffer to be called on offers with takers, causing their funds to be stuck in the protocol.

In PreMarkets::createTaker function:

function createTaker(address _offer, uint256 _points) external payable {
...
OfferInfo storage offerInfo = offerInfoMap[_offer];
if (offerInfo.offerStatus != OfferStatus.Virgin) {
revert InvalidOfferStatus();
}
...
offerInfo.usedPoints = offerInfo.usedPoints + _points;
...
}

The function updates usedPoints but doesn't change the offer status from Virgin to Ongoing, allowing closeOffer to be called with takers.

Proof of code
Add this test to PreMarkets.t.sol:

function test_offerCanBeClosedWithTakers() public {
/// 1st user create offer
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
10000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
​
vm.stopPrank();
​
/// 2nd user create taker
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 500);
preMarktes.createTaker(offerAddr, 200);
vm.stopPrank();
​
/// 1st user can close offer
vm.startPrank(user);
preMarktes.closeOffer(stockAddr, offerAddr);
vm.stopPrank();
}

Impact

When a maker closes an offer with active takers, the takers' funds will be permanently stuck in the protocol. There is no mechanism for takers to withdraw their funds after an offer is closed, resulting in direct financial loss for the takers.

Tools Used

  • Manual review

  • Foundry

Recommendations

Update offer status after a taker is created:

function createTaker(address _offer, uint256 _points) external payable {
...
OfferInfo storage offerInfo = offerInfoMap[_offer];
if (offerInfo.offerStatus != OfferStatus.Virgin) {
revert InvalidOfferStatus();
}
...
offerInfo.usedPoints = offerInfo.usedPoints + _points;
+ offerInfo.offerStatus = OfferStatus.Ongoing;
...
}
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-createTaker-Ongoing-Status

The reason for severity for this issue and duplicates are very similar to issue #1164. However, in this case, the issues correctly identified that offer statuses should be updated accordingly based on points transaction (partially filled orders = `Ongoing`, fully filled orders = `Filled`). There is currently no impact on the tadle system since the above two statuses are unused, and while sementically `Virgin` status is not the correct status representing whether taker orders can be created agains maker offers, it can still be performed without a DoS. However, if we are basing it off of the correct status implementation (i.e. `Ongoing` phase appropriately updated when takers create taker offers), then the DoS will occur, essentially blocking any taker offers from being created by subsequent takers for partially filled orders. All issues that does not mention the DoS impact will be low severity, since they still correctly highlighted the wrong status accounting. All issues that mention the possible bypass of `Virgin` status is incorrect, because the usedPoint checks will always ensure points are filled only up to the points posted for offer as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L180-L186). Note for downgrade to low severity: Agree with appeals and low severity, this is more of a status accounting error and does not have any impact, since the statuses consistently do not utilize a switch from Vigin to Ongoing/Filled and the protocol can function appropriately even without the use of such statuses (presuming other bugs are fixed), the DoS impact will not occur.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.