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

`PreMarket::createTaker`  and `closeOffer` Does Not Work With Ongoing Status

4rdiii

[M-01] PreMarket::createTaker and closeOffer Does Not Work With Ongoing Status

Summary

The createTaker function within the PreMarket contract is designed to facilitate the creation of new takers for offers. However, it currently does not support offers that are in an Ongoing status. This limitation prevents users from interacting with offers that have already had one or more trades but still have available points. Since the function only supports offers in a Virgin state, partially filled offers cannot be fully utilized, leading to inefficiencies and missed opportunities for both makers and takers.

Vulnerability Details

The createTaker function is intended to allow users to participate in offers by matching them with available points. However, the function's implementation restricts participation to only those offers that are in a Virgin state, ignoring offers that are Ongoing. This restriction is enforced through a check that rejects any interaction with offers whose status is not Virgin.
Note: Currently due to another bug this issue is masked, the current createTaker function does not update the offer status according to used points, but if this bug is fixed and the offer status is updated correctly this bug will arise eventually.
Here is the edited version of createTaker function which updates the offer status according to usedPoints, Also note that the OfferStatus check only checks for Virgin status and reverting if the status is ongoing.

function createTaker(address _offer, uint256 _points) external payable {
/**
* @dev offer must be virgin
* @dev points must be greater than 0
* @dev total points must be greater than used points plus _points
*/
if (_points == 0x0) {
revert Errors.AmountIsZero();
}
​
OfferInfo storage offerInfo = offerInfoMap[_offer];
MakerInfo storage makerInfo = makerInfoMap[offerInfo.maker];
@> if (offerInfo.offerStatus != OfferStatus.Virgin) {
@> revert InvalidOfferStatus();
@> }
​
if (offerInfo.points < _points + offerInfo.usedPoints) {
revert NotEnoughPoints(
offerInfo.points,
offerInfo.usedPoints,
_points
);
}
​
/// @dev market place must be online
ISystemConfig systemConfig = tadleFactory.getSystemConfig();
{
MarketPlaceInfo memory marketPlaceInfo = systemConfig
.getMarketPlaceInfo(makerInfo.marketPlace);
marketPlaceInfo.checkMarketPlaceStatus(
block.timestamp,
MarketPlaceStatus.Online
);
}
​
ReferralInfo memory referralInfo = systemConfig.getReferralInfo(
_msgSender()
);
​
uint256 platformFeeRate = systemConfig.getPlatformFeeRate(_msgSender()); // > 5 %
​
/// @dev generate stock address
address stockAddr = GenerateAddress.generateStockAddress(offerId);
if (stockInfoMap[stockAddr].authority != address(0x0)) {
revert StockAlreadyExist();
}
​
/// @dev Transfer token from user to capital pool as collateral
uint256 depositAmount = _points.mulDiv(
offerInfo.amount,
offerInfo.points,
Math.Rounding.Ceil
);
uint256 platformFee = depositAmount.mulDiv(
platformFeeRate,
Constants.PLATFORM_FEE_DECIMAL_SCALER
);
uint256 tradeTax = depositAmount.mulDiv(
makerInfo.eachTradeTax,
Constants.EACH_TRADE_TAX_DECIMAL_SCALER
);
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
_depositTokenWhenCreateTaker(
platformFee,
depositAmount,
tradeTax,
makerInfo,
offerInfo,
tokenManager
);
​
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
);
}

Impact

This limitation significantly impacts the usability and effectiveness of the marketplace. Offers that could otherwise be partially filled remain unutilized because the createTaker function does not recognize their potential for further interaction. Users attempting to engage with these offers will encounter a revert error, indicating that the offer status is invalid for their action.

Tools Used

Manual Review

Recommendations

To address this issue, the createTaker function should be modified to support offers in an Ongoing state, allowing users to continue participating in these offers by filling them with available points. This modification involves adjusting the condition that checks the offer's status to include Ongoing alongside Virgin.

function createTaker(address _offer, uint256 _points) external payable {
/**
* @dev offer must be virgin
* @dev points must be greater than 0
* @dev total points must be greater than used points plus _points
*/
if (_points == 0x0) {
revert Errors.AmountIsZero();
}
​
OfferInfo storage offerInfo = offerInfoMap[_offer];
MakerInfo storage makerInfo = makerInfoMap[offerInfo.maker];
- if (offerInfo.offerStatus != OfferStatus.Virgin) {
- revert InvalidOfferStatus();
- }
+ if (offerInfo.offerStatus != OfferStatus.Virgin && offerInfo.offerStatus != OfferStatus.Ongoing) {
+ revert InvalidOfferStatus();
+ }
.
.
.
}
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.

Support

FAQs

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

Give us feedback!