Tadle

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

Incorrect Check in closeBidOffer function

Summary

The closeBidOffer function is intended to close a bid offer, but it's checking if the offer status is "Virgin" (presumably meaning untouched or new). This doesn't align with the function's purpose or the comment above the function.

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

The comment states:

/**
* @dev offer status must be Settling
*/

However, the code is checking for the opposite condition:

if (offerInfo.offerStatus != OfferStatus.Virgin) {
revert InvalidOfferStatus();
}

This means the function will revert for any offer that isn't in the Virgin status, which contradicts the intended behavior described in the comment.

Impact

Funds could potentially be locked in the contract. If bid offers can't be closed properly, the deposited funds for these offers might become inaccessible. This could lead to a loss of funds for users who can't retrieve their unused bid amounts.

Mitigation

The condition should be changed to check for the Settling status instead.

if (offerInfo.offerStatus != OfferStatus.Settling) {
revert InvalidOfferStatus();
}

And the correct comment should be:

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

Poc

function test_close_bid_offer() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
preMarktes.setOfferStatus(offerAddr, OfferStatus.Settling);
deliveryPlace.closeBidOffer(offerAddr);
OfferInfo memory offerInfo = preMarktes.getOfferInfo(offerAddr);
assertEq(uint(offerInfo.offerStatus), uint(OfferStatus.Settled), "Offer should be in Settled status");
vm.stopPrank();
}

Add this function for testing purposes

function setOfferStatus(address _offer, OfferStatus _status) public {
require(block.chainid == 31337, "This function is only for testing");
offerInfoMap[_offer].offerStatus = _status;
}
Updates

Lead Judging Commences

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

Appeal created

h2134 Auditor
12 months ago
0xbrivan2 Auditor
12 months ago
0xnevi Lead Judge
11 months ago
0xnevi Lead Judge 11 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.