Tadle

Tadle

Tadle

DeFi
30,000 USDC
Ended
Submission Details
Severity: low
Invalid

Abort offer doesn't return all collateral as expected

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),
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 5 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.

Appeal created

cryptomoon Auditor
5 months ago

this is clearly invalid. I agree that sponsor mentioned this in chat but we can't consider sponsor comments in codehawks rule of judging. The code is source of truth and looking into how things are working, the above issue is invalid.

0xbrivan2 Auditor
5 months ago

I disagree with the appeal above. the code is clearly not working as intended. Aborting offers means canceling all the deals and returning the collateral back to the offer maker. This is an obvious concept in the order-book model.

Concerning the sponsor's comment, he just answered questions in a public chat.

This issue is valid

cryptomoon Auditor
5 months ago

"Aborting offers means canceling all the deals and returning the collateral back to the offer maker."

No, aborting offer means canceling all the deals and returning collateral corresponding to unused points. If some of the points are already used, the full amount won't be refunded to maker. Let me explain why this is designed in such a way:

If a maker creates a ASK offer and taker buys points from it. 

When maker aborts the ASK offer, he should only get collateral corresponding to unused points. He shouldn't get full collateral because taker has already invested funds in his offer and that can also be claimed by the maker.

My question to submitter is,

"If maker should get full collateral back after aborting the offer, from which fund will taker claim his refund by calling "abortBidTaker"?"

The sponsors comment was totally wrong because the code is designed in such a way that maker gets partial collateral compared to unused points + taker's funds and taker can claim refund by calling "abortBidTaker"

@0xnevi, sorry if my first comment wasn't providing proper context. 

cryptomoon Auditor
5 months ago

The impact mentioned in the issue is:

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

Where is loss of funds for the maker?

| wrong implementation of the contract based on the sponsor's comment? 
@0xnevi, we can't consider sponsor's comment and mention bugs based on that. You can check with equinous about that. 

0xbrivan2 Auditor
5 months ago

For hawk 8, you said:

No, aborting offer means canceling all the deals and returning collateral corresponding to unused points

This is incorrect, closeOffer  is used to close the offer and get refunded for the unused points. It seems you are misunderstanding some functions of the protocol

"If maker should get full collateral back after aborting the offer, from which fund will taker claim his refund by calling "abortBidTaker"?"

Yes, abortBidTaker is called for takers to get refunded for aborted offers.

Where is loss of funds for the maker?

Read the function again and notice how the full collateral is not returned back to the offer maker when calling abortAsOffer

Again, the issue is valid as abortAskOffer only refunds the offer maker with the unused portion of the amount (the margin)

Duplicate #887 described this issue better tho.

cryptomoon Auditor
5 months ago

For Hawk 9,

"Read the function again and notice how the full collateral is not returned back to the offer maker when calling abortAsOffer"

My response:

The maker will also get the fund that is sent by taker. So, he shouldn't get his full collateral back. check this function, https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L265-L273
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L906-L949 
maker is already getting funds corresponding to used points. His balance is updated which he can claim later. So, maker should get refund of unused points and not full collateral. Doing it otherwise will be loss to the protocols.

cryptomoon Auditor
5 months ago

@nevi, if you add the following console.logs in the POC and rerun it:

console2.log(
"Sales balance %d",
tokenManager.userTokenBalanceMap(address(user), address(mockUSDCToken), TokenBalanceType.SalesRevenue)
);

following is printed:

Refund balance 9000000000000000
Sales balance 3000000000000000
Total deposited amount 12000000000000000

That means maker deposited 12000000000000000. He can claim 3000000000000000 and gets refund of 9000000000000000.

The submitter says that there is loss to maker. Where is the loss. He is getting all his deposited collateral back

0xnevi Lead Judge
5 months ago

hawk 8 is correct, this issue and its duplicates are invalid and closely related to issue #826. The implementation is correct, abortAskOffer should only be refunding unused points. The portion of 3000000000000000 will be present in the makers SalesRevenue and is withdrawable when the taker creates a taker offer against the maker offer as seen here

0xnevi Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
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.

Support

FAQs

Can’t find an answer? Join our Discord or follow us on Twitter.

Cyfrin
Updraft
CodeHawks
Solodit
Resources