Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

The `DeliveryPlace::closeBidOffer` function checks if the offer status is `Virgin` instead of `Settling`

bube

Links

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L58-L60

Summary

There is a logic error in the DeliveryPlace::closeBidOffer function. The function incorrectly checks if the offerStatus is Virgin instead of Settling.

Vulnerability Details

The purpose of the DeliveryPlace::closeBidOffer function is to close a bid offer under specific conditions. It ensures that the offer is in the correct state and that the caller has the appropriate authority to close the bid.

​
function closeBidOffer(address _offer) external {
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
,
MarketPlaceStatus status
) = getOfferInfo(_offer);
​
if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
​
if (offerInfo.offerType == OfferType.Ask) {
revert InvalidOfferType(OfferType.Bid, OfferType.Ask);
}
​
if (
status != MarketPlaceStatus.AskSettling &&
status != MarketPlaceStatus.BidSettling
) {
revert InvaildMarketPlaceStatus();
}
​
@> if (offerInfo.offerStatus != OfferStatus.Virgin) {
revert InvalidOfferStatus();
}
​
uint256 refundAmount = OfferLibraries.getRefundAmount(
offerInfo.offerType,
offerInfo.amount,
offerInfo.points,
offerInfo.usedPoints,
offerInfo.collateralRate
);
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
refundAmount
);
​
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
perMarkets.updateOfferStatus(_offer, OfferStatus.Settled);
​
emit CloseBidOffer(
makerInfo.marketPlace,
offerInfo.maker,
_offer,
_msgSender()
);
}

The offer status should be Settling according to the comments above the function:

​
/**
* @notice Close bid offer
* @dev caller must be offer authority
* @dev offer type must Bid
* @dev offer status must be Settling
* @dev refund amount = offer amount - used amount
*/
​

The problem is that the function checks if the offerStatus is Virgin, which is incorrect for the intended functionality.
The correct status to check should be Settling to ensure the offer is in the appropriate state for closing.

Impact

This incorrect check can prevent valid bid offers from being closed. It allows also offers in the wrong state to be closed.

Tools Used

Manual Review

Recommendations

The offerStatus should be checked against OfferStatus.Settling to ensure that the offer is in the correct state for closing:

​
function closeBidOffer(address _offer) external {
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
,
MarketPlaceStatus status
) = getOfferInfo(_offer);
​
if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
​
if (offerInfo.offerType == OfferType.Ask) {
revert InvalidOfferType(OfferType.Bid, OfferType.Ask);
}
​
if (
status != MarketPlaceStatus.AskSettling &&
status != MarketPlaceStatus.BidSettling
) {
revert InvaildMarketPlaceStatus();
}
​
- if (offerInfo.offerStatus != OfferStatus.Virgin) {
- revert InvalidOfferStatus();
- }
​
+ if (offerInfo.offerStatus != OfferStatus.Settling) {
+ revert InvalidOfferStatus();
+ }
.
.
.
Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-closeBidOffer-Virgin-Settling

Based on the current Tadle market system, the `Settling` status is never used (along with `Ongoing` and `Filled`), which is supposed to represent the state before settlement by original maker. While sementically, the `Virgin` status does not represent the correct phase to allow early closures before settlement, this issue does not have any current impact given technically the early closure of bid offers is still allowed. However, if we are basing it off of the correct status implementation (i.e. `Settling` phase appropriately updated when takers create offers), then the DoS will occur, essentially blocking any early closure of bids by subsequent makers, forcing them to follow through to final settlement. Unfortunately, none of these issues identify the correct pre-context mentioned above, but I believe medium severity is appropriate. Note for downgrade to low severity: Agree with above appeals and low severity, this is more of a status accounting error and does not have any impact, given the function of `closeBidOffer` is to withdraw the unused portion of sales proceeds. It can be executed as long as the TGE time has been reached, and it restricts the offer to be in a Virgin state. 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.