Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

`pointTokenAmount` Paid to Taker in `DeliveryPlace::closeBidTaker` is Incorrect Due to Faulty Formula

krisrenzo

Summary

The pointTokenAmount paid to takers is incorrectly calculated due to a faulty formula, leading to an incorrect distribution of tokens.

Vulnerability Details

When a bid taker calls DeliveryPlace::closeBidTaker to close their stock after settlement, the pointTokenAmount sent to them is incorrectly calculated.

uint256 pointTokenAmount = offerInfo.settledPointTokenAmount.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);

Key Definitions:

  • pointTokenAmount: The amount of point tokens received by takers after makers settle with them.

  • settledPointTokenAmount: The amount of point tokens the maker has paid for settling their traded points.

  • userRemainingPoints: The portion of the taker's points that haven't been settled.

  • usedPoints: The total points traded by the maker.

Formula Breakdown:

  • The formula in plain text:
    pointTokenAmount = (settledPointTokenAmount * userRemainingPoints) / usedPoints

  • Reversing the formula, we get:

    pointTokenAmount == userRemainingPoints (correct)
    settledPointTokenAmount == usedPoints (incorrect)

The Issue: The formula is incorrect because settledPointTokenAmount is not equivalent to usedPoints; it should be equivalent to settledPoints. The formula should use settledPoints instead of usedPoints because those are the points that have actually been settled by the maker.

Impact

The protocol incorrectly assumes that the maker settles all of their usedPoints when they settle. However, makers can partially settle, leading to the taker being paid the wrong amount of point tokens.

Tools Used

  • Manual

Recommendations

Fix the formula by replacing usedPoints with settledPoints, as those are the points that have actually been settled.

uint256 pointTokenAmount = offerInfo.settledPointTokenAmount.mulDiv(
userRemainingPoints,
- offerInfo.usedPoints,
+ offerInfo.settledPoints,
Math.Rounding.Floor
);
Updates

Lead Judging Commences

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

finding-PreMarkets-partial-closeBidTaker-userCollateralFee-wrong-partial

Valid High, afaik, partial settlements are a valid flow and so when closing bid offers by takers and/or when settling offers by makers, we should return a proportionate amount of funds based on points settled. This issues could be related to issue #1008, but seems to be describing a different issue.

Appeal created

krisrenzo Submitter
12 months ago
0xnevi Lead Judge
11 months ago
0xnevi Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-DeliveryPlace-settleAskTaker-settleAskMaker-partial-settlements

Valid high, in settleAskTaker/settleAskMaker, if the original offer maker performs a partial final settlement, the existing checks [here](https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L356-L358) and [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L230-L232) will cause an revert when attempting to complete a full settlement, resulting in their collateral being locked and requiring a rescue from the admin. To note, although examples in the documentation implies settlement in a single click, it is not stated that partial settlements are not allowed, so I believe it is a valid user flow.

Support

FAQs

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