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

`PreMarket::createTaker` Should Update the `offerInfo.offerStatus` According to `amount usedPoints`

4rdiii

[L-01] PreMarket::createTaker Should Update the offerInfo.offerStatus According to amount usedPoints

Summary

The offerStatus enum in the PreMarket contract has several statuses to indicate the state of an offer. However, the createTaker function does not update the offerInfo.offerStatus based on whether at least one trade has happened or if all points in an order are used. This oversight contradicts the documentation and NATSPEC, which specify that offerStatus should be set to Ongoing when at least one trade has occurred or Filled if all points in an order are used.

Vulnerability Details

The offerInfo.offerStatus is crucial for users to understand the availability and state of an offer. Despite this, the createTaker function does not update the offerInfo.offerStatus at any point during its execution, even though it modifies the usedPoints.

/**
* @dev Offer status
* @notice Unknown, Virgin, Ongoing, Canceled, Filled, Settling, Settled
* @param Unknown offer not yet exist.
* @param Virgin offer has been listed, but not one trade.
* @param Ongoing offer has been listed, and already one trade.
* @param Canceled offer has been canceled.
* @param Filled offer has been filled.
* @param Settling offer is settling.
* @param Settled offer has been settled, the last status.
*/
enum OfferStatus {
Unknown,
Virgin,
@> Ongoing,
Canceled,
@> Filled,
Settling,
Settled
}

The createTaker function should update the offerInfo.offerStatus after updating the usedPoints according to the usedPoints value.

function createTaker(address _offer, uint256 _points) external payable {
.
.
.
​
@> offerInfo.usedPoints = offerInfo.usedPoints + _points;
@> //e Missing logic to update offerStatus
​
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
stockType: offerInfo.offerType == OfferType.Ask
? StockType.Bid
: StockType.Ask,
authority: _msgSender(),
maker: offerInfo.maker,
preOffer: _offer,
points: _points,
amount: depositAmount,
offer: address(0x0)
});
​
offerId = offerId + 1;
uint256 remainingPlatformFee = _updateReferralBonus(
platformFee,
depositAmount,
stockAddr,
makerInfo,
referralInfo,
tokenManager
);
makerInfo.platformFee = makerInfo.platformFee + remainingPlatformFee;
​
_updateTokenBalanceWhenCreateTaker(
_offer,
tradeTax,
depositAmount,
offerInfo,
makerInfo,
tokenManager
);
​
/// @dev emit CreateTaker
emit CreateTaker(
_offer,
msg.sender,
stockAddr,
_points,
depositAmount,
tradeTax,
remainingPlatformFee
);
}

Impact

Users may attempt to interact with offers that are marked as Virgin or Unknown but are actually Filled or Ongoing, leading to potential reverts. This issue undermines the reliability of the contract's documentation and NATSPEC, potentially causing confusion among users.

Tools Used

Manual Review

Recommendations

The createTaker function should explicitly update the offerInfo.offerStatus based on the usedPoints to accurately reflect the offer's state. Here's how the corrected function might look:

function createTaker(address _offer, uint256 _points) external payable {
.
.
.
​
offerInfo.usedPoints = offerInfo.usedPoints + _points;
​
+ if (offerInfo.usedPoints == offerInfo.points){
+ offerInfo.offerStatus = OfferStatus.Filled;
+ }else{
+ offerInfo.offerStatus = OfferStatus.Ongoing;
+ }
​
stockInfoMap[stockAddr] = StockInfo({
id: offerId,
stockStatus: StockStatus.Initialized,
stockType: offerInfo.offerType == OfferType.Ask
? StockType.Bid
: StockType.Ask,
authority: _msgSender(),
maker: offerInfo.maker,
preOffer: _offer,
points: _points,
amount: depositAmount,
offer: address(0x0)
});
​
offerId = offerId + 1;
uint256 remainingPlatformFee = _updateReferralBonus(
platformFee,
depositAmount,
stockAddr,
makerInfo,
referralInfo,
tokenManager
);
makerInfo.platformFee = makerInfo.platformFee + remainingPlatformFee;
​
_updateTokenBalanceWhenCreateTaker(
_offer,
tradeTax,
depositAmount,
offerInfo,
makerInfo,
tokenManager
);
​
/// @dev emit CreateTaker
emit CreateTaker(
_offer,
msg.sender,
stockAddr,
_points,
depositAmount,
tradeTax,
remainingPlatformFee
);
}
Updates

Lead Judging Commences

0xnevi Lead Judge over 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.

Appeal created

0xbrivan2 Auditor
over 1 year ago
0xnevi Lead Judge
over 1 year ago
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.

Give us feedback!