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

Incorrect `userCollateralFee` Calculation in `DeliveryPlace::closeBidTaker`

krisrenzo

Summary

The userCollateralFee in the DeliveryPlace::closeBidTaker function is calculated using an incorrect formula, leading to potential inaccuracies in the reimbursement amount for unsettled points.

Vulnerability Details

The userCollateralFee is incorrectly calculated due to an incorrect formula used in DeliveryPlace::closeBidTaker. There is a contradiction between how collateralFee is calculated versus how usedPoints is calculated.

Key Variable Definitions:

  • userRemainingPoints: Represents the unsettled points by the Ask Maker. It is correctly calculated as: userRemainingPoints == listOfferInfo.points - listOfferInfo.usedPoints.

  • collateralFee: Represents the total deposit made by the Maker when they created their offer. You can collateralFee in DeliveryPlace::closeBidTaker and transferAmount in PreMarket::createOffer

The Incorrect Formula:

The problematic calculation is as follows:

uint256 userCollateralFee = collateralFee.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);

When this formula is reversed, the following relationship is implied:

  • userCollateralFee corresponds to userRemainingPoints (correct).

  • collateralFee corresponds to usedPoints (incorrect; it should correspond to points).

Given the definitions of userRemainingPoints and collateralFee, it is evident that the formula is incorrect because collateralFee should be matched with offerInfo.points, which represents the total points in the Maker's offer corresponding to their total deposit (collateralFee).

Impact

The protocol reimburses the Bid Taker an incorrect amount when they call for a refund of their unsettled points, potentially leading to financial discrepancies.

Tools Used

Manual analysis.

Recommendations

Fix the formula by using the correct variable. Replace offerInfo.usedPoints with offerInfo.points when calculating userCollateralFee:

uint256 userCollateralFee = collateralFee.mulDiv(
userRemainingPoints,
- offerInfo.usedPoints,
+ offerInfo.points,
Math.Rounding.Floor
);
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year 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

0xnevi Lead Judge about 1 year 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.