Tadle

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

Created offers cannot be completely filled if they are partially filled by the first taker

Summary

User can call Premarkets::createTaker(...) to fully or partially fill offers. But this can lead to a DOS if the first taker only fills the offer partially

Vulnerability Details

Premarkets::createTaker(...)does not update the offerto Ongoingconsidering that the as shown in the OfferStatus.solNATSPEC comment below

File: OfferStatus.sol
9: * @param Ongoing offer has been listed, and already one trade.

The problem is that,

  • contrary to the DOCs, orders do not get updated from Virgin to Ongoing when they get at least one trade

  • the check in the createTaker(...)does not give room for ongoing orders to to be filled by takers and as such if the proper update is done from Virginto Ongoing, as pointed out in this report, the remaing unfill points in the offer cannot be filled or by another taker and the

File: PreMarkets.sol
163: */
164: function createTaker(address _offer, uint256 _points) external payable {
165: /**
166: * @dev offer must be virgin
167: * @dev points must be greater than 0
168: * @dev total points must be greater than used points plus _points
169: */
170: if (_points == 0x0) {
171: revert Errors.AmountIsZero();
172: }
173:
174: OfferInfo storage offerInfo = offerInfoMap[_offer];
175: MakerInfo storage makerInfo = makerInfoMap[offerInfo.maker];
176:
177: @> if (offerInfo.offerStatus != OfferStatus.Virgin) { // @audit SUGG: add || OfferStatus.Ongoing
178: @> revert InvalidOfferStatus();
179: }

Impact

I am reporting this as a low because it is a missing update, however, if the update is done in the createTaker(...)function and it is not

Tools Used

Manual review

Recommendations

Modify the createTaker(...)function as shown below

File: PreMarkets.sol
163: */
164: function createTaker(address _offer, uint256 _points) external payable {
165: /**
166: * @dev offer must be virgin
167: * @dev points must be greater than 0
168: * @dev total points must be greater than used points plus _points
169: */
170: if (_points == 0x0) {
171: revert Errors.AmountIsZero();
172: }
173:
174: OfferInfo storage offerInfo = offerInfoMap[_offer];
175: MakerInfo storage makerInfo = makerInfoMap[offerInfo.maker];
176:
-177: @> if (offerInfo.offerStatus != OfferStatus.Virgin) {
+177: @> if (offerInfo.offerStatus != OfferStatus.Virgin || offerInfo.offerStatus != OfferStatus.Ongoing) {
178: @> revert InvalidOfferStatus();
179: }
+ if (offerInfo.offerStatus == OfferStatus.Virgin) {
+ 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.