Tadle

DeFi

30,000 USDC

View results

Previous Next

Submission Details

Severity: low

Invalid

Abort offer doesn't return all collateral as expected

ge6a

Summary

In a comment on the sponsor's public Discord, it was confirmed that upon the abort of an offer, the entire collateral is expected to be returned to the maker. The problem is that in the current implementation, this does not work as expected, and the collateral corresponding to the usedPoints is not returned. I will demonstrate this using the PreMarket.abortAskOffer function.

Vulnerability Details

First, here is a fragment from the sponsor's comment with a link to it.

Let me drop some context about the question mostly being mentioned.

The difference between abortOffer and cancelOffer:

Example:

1 Maker is selling 100 pts.

2 Takers bought 60 pts from the offer.

If the 2 Takers don't relist the holdings/stock pts, then the Maker can abortOffer and all deals between the Maker and the Takers will be set to INVALID. The Maker get all the collateral fund back. No liability to settle any token.

Whether the Takers relist or not, the Maker can cancelOffer and get the REST collateral (for remaining 40 pts) back. Then no taker can buy pts from this CANCELLED offer. When TGE comes, the Maker still obtain the liability to settle the tokens related to the sold 60 points before.

https://discord.com/channels/1127263608246636635/1268902639555579954/1270804093463695502

I am also attaching the part of the PreMarket.abortAskOffer function that shows how the corresponding refund amount is calculated. It is clearly seen that the entire collateral is reduced by the collateral corresponding to the recorded amount of usedPoints (i.e., the points that were purchased by the taker).

uint256 transferAmount = OfferLibraries.getDepositAmount(

offerInfo.offerType,

offerInfo.collateralRate,

remainingAmount,

true,

Math.Rounding.Floor

);

uint256 totalUsedAmount = offerInfo.amount.mulDiv(

offerInfo.usedPoints,

offerInfo.points,

Math.Rounding.Ceil

);

uint256 totalDepositAmount = OfferLibraries.getDepositAmount(

offerInfo.offerType,

offerInfo.collateralRate,

totalUsedAmount,

false,

Math.Rounding.Ceil

);

///@dev update refund amount for offer authority

uint256 makerRefundAmount;

if (transferAmount > totalDepositAmount) {

makerRefundAmount = transferAmount - totalDepositAmount;

} else {

makerRefundAmount = 0;

}

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L595-L621

Finally, I am attaching a proof of concept (POC) that demonstrates this in practice.

POC

function test_ask_protected_chain_poc() public {

vm.startPrank(user);

uint256 userUSDTBalance0 = mockUSDCToken.balanceOf(user);

preMarktes.createOffer(

CreateOfferParams(

marketPlace,

address(mockUSDCToken),

1000,

0.01 * 1e18,

12000,

300,

OfferType.Ask,

OfferSettleType.Protected

)

);

uint256 userUSDTBalance1 = mockUSDCToken.balanceOf(user);

address offerAddr = GenerateAddress.generateOfferAddress(0);

vm.stopPrank();

vm.startPrank(user1);

mockUSDCToken.approve(address(tokenManager), type(uint256).max);

preMarktes.createTaker(offerAddr, 300);

vm.stopPrank();

vm.startPrank(user);

address originStock = GenerateAddress.generateStockAddress(0);

address originOffer = GenerateAddress.generateOfferAddress(0);

console.log("Refund balance %d", tokenManager.userTokenBalanceMap(

address(user),

address(mockUSDCToken),

TokenBalanceType.MakerRefund

));

//preMarktes.closeOffer(originStock, originOffer);

console.log("Refund balance %d", tokenManager.userTokenBalanceMap(

address(user),

address(mockUSDCToken),

TokenBalanceType.MakerRefund

));

preMarktes.abortAskOffer(originStock, originOffer);

console.log("Refund balance %d", tokenManager.userTokenBalanceMap(

address(user),

address(mockUSDCToken),

TokenBalanceType.MakerRefund

));

console.log("Total deposited amount %d", userUSDTBalance0-userUSDTBalance1);

return;

}

Refund balance 9000000000000000

Total deposited amount 12000000000000000

Impact

Loss of funds for the maker and wrong implementation of the contract based on the sponsor's comment

Tools Used

Manual review

Recommendations

Updates

Lead Judging Commences

0xnevi Lead Judge 9 months ago

Submission Judgement Published

Validated

Assigned finding tags:

[invalid] finding-PreMarkets-abortAskOffer-makerRefundAmount-wrong-compute

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 this wrong refund won't occur, so could be duplicates, given only Virgin orders can be aborted (same applies for canceled orders, since only virgin orders are supposed to be able to be canceled). Since the status update is incorrect, then if a virgin order that is partially filled is aborted, the collateral is not returned correctly even though the order is not settled, causing a loss of funds to makers.