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

Wrong calculation of collateralFee during settlement will lead to total collateral always being sent to the buyer even incase of a partial points settlement.

touthang

Summary

For the purpose of explaining this issue lets look at settleAskTaker function;

The settleAskTaker function in the DeleveryPlace contract has a parameter to specify the amount of points the seller wants to settle. This option is not compatible with the current calculation of collateralFee. So when this is used by a seller who decides to settle only half of the points, his whole collateral will still be deducted instead of half.

Example-

  • A seller sold 1000 points with 1400 USDC as collateral, 140%

  • He decides to settle only 500 points because this option is provided by the function, and accept to lose half of his collateral (700 USDC).

  • But after the transaction is executed he will lose total collateral(1400) to the buyer instead of 700 USDC. And also the buyer will still be settled with 500 points.

Vulnerability Details

When a seller doesn't settle the full amount of points for the buyer the collateral of the seller is sent to the buyer :

} else {
tokenManager.addTokenBalance( // if a seller settles partial amount of points
TokenBalanceType.MakerRefund,
offerInfo.authority,
makerInfo.tokenAddress,
collateralFee
);
}

The amount to sent is calculated by:

uint256 collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
stockInfo.amount,
false,
Math.Rounding.Floor
);

The problem here is collateralFee calculation always use stockInfo.amount which doesn't account for the points being settled and will always calculate as if no points are being settled at all, which will always return the full amount of collateral.

So, whether the seller settle 1 points out of 1000 or 999 out of 1000 in both cases his total collateral be sent to the buyer, which is a loss of both functionality and collateral for the seller.

Impact

  • Loss of functionality(option given by the function to specify amount of _settledPoints)

  • along with loss of funds(collateral).

Note: Similar issue can also be found in the settleAskMaker function. The fix will be different and fixing one will not fix the other, but still reporting this as one submission and leaving for the lead judge to decide if this can be taken as two separate issue.

Tools Used

manual

Recommendations

Fix for settleAskTaker:

implement these changes in the settleAskTaker function :

if (_settledPoints == stockInfo.points) {
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
collateralFee
);
+ } else if(_settledPoints > 0){ //@audit distribute the collateral accordingly if partial settlement
+ // @audit refund the collateral unused to the seller
+ uint256 refundCollateralAmountToSeller = (collateralFee * _settledPoints) / stockInfo.points;
+ tokenManager.addTokenBalance(
+ TokenBalanceType.RemainingCash,
+ _msgSender(),
+ makerInfo.tokenAddress,
+ refundCollateralAmountToSeller
+ );
+ // @audit send the used collateral to the buyer
+ uint256 compensateBuyerByCollateralAmount = collateralFee - refundCollateralAmountToSeller;
+ tokenManager.addTokenBalance(
+ TokenBalanceType.MakerRefund,
+ offerInfo.authority,
+ makerInfo.tokenAddress,
+ compensateBuyerByCollateralAmount
+ );
} else {
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
offerInfo.authority,
makerInfo.tokenAddress,
collateralFee
);
}
​
perMarkets.settleAskTaker(
stockInfo.preOffer,
_stock,
_settledPoints,
settledPointTokenAmount
);
​

OR
completely remove the option to specify points amount to settle and always get the exact mount of points:
This means we have to remove one functionality from the protocol.

- function settleAskTaker(address _stock, uint256 _settledPoints) external {
+ function settleAskTaker(address _stock) external {
+ uint256 _settledPoints = msg.sender == owner() ? 0 : stockInfo.points;

Fix for settleAskMaker`:

Add an else if statement in settleAskMaker function to repay partial collateral for partial settlement:

uint256 makerRefundAmount;
//@audit if full points are settled return all the collateral
if (_settledPoints == offerInfo.usedPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
​
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}
//@audit during partial points settlement, return the collateral for settled points and lock the collateral for unsettled points
+ else if (_settledPoints > 0) {
+ if (offerInfo.offerStatus == OfferStatus.Virgin) {
+ makerRefundAmount = OfferLibraries.getDepositAmount(
+ offerInfo.offerType,
+ offerInfo.collateralRate,
+ offerInfo.amount,
+ true,
+ Math.Rounding.Floor
+ );
+ makerRefundAmount = makerRefundAmount * _settledPoints / offerInfo.points;
+ } else {
+ uint256 usedAmount = offerInfo.amount.mulDiv(
+ offerInfo.usedPoints,
+ offerInfo.points,
+ Math.Rounding.Floor
+ );
+
+ makerRefundAmount = OfferLibraries.getDepositAmount(
+ offerInfo.offerType,
+ offerInfo.collateralRate,
+ usedAmount,
+ true,
+ Math.Rounding.Floor
+ );
+
+ makerRefundAmount = makerRefundAmount * _settledPoints / offerInfo.usedPoints;
+ //@audit make sure to use offerInfo.usedPoints here, not offerInfo.points
+ }
+
+ tokenManager.addTokenBalance(
+ TokenBalanceType.SalesRevenue,
+ _msgSender(),
+ makerInfo.tokenAddress,
+ makerRefundAmount
+ );
+
+ }

This way if some amount of points were setteled by the seller then he only loses the collateral of the points that he did not settle for and get back the collateral for the poits he setteled.

In this case we also need to change the collateral amount given to the buyer in closeBidTaker:

uint256 collateralFee;
if (offerInfo.usedPoints > offerInfo.settledPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
+ if (offerInfo.settledPoints > 0) {
+ //@audit calculate collateral only for the unsettled points
+ collateralFee = collateralFee - ((collateralFee * offerInfo.settledPoints) / offerInfo.points);
+ }
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
​
+ if (offerInfo.settledPoints > 0) {
+ collateralFee = collateralFee - ((collateralFee * offerInfo.settledPoints) / offerInfo.usedPoints);
+ //@audit make sure to use offerInfo.usedPoints here not offerInfo.points.
+ }
}
}
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

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.