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

settleAskMaker function fails to return the unused collateral of a seller who does not settle the points.

touthang

Summary

There can be a seller whose points are not fully bought, which means the collateral is also not fully used and there are some unused collateral. And if this seller decides to not settle the points then the collateral along with the unused collateral will be locked.

  • on the buyer side the buyer will get only the used collateral which is correct

  • but on the seller's side the seller will lose all of his collateral including the unused one.

This is because the settleAskMaker function overlooks this scenario.

Vulnerability Details

As we can see from the settleAskMaker` function, the collateral tokens are sent back to the seller only if he settles the points if (_settledPoints == offerInfo.usedPoints):

uint256 makerRefundAmount;
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
);
}

This means that if a seller fails to settle the points, the function always assumes that all the seller's points are bought and will never send back the unused collateral.

Example-

  • Alice sells 1000 points for 1000 USDC, and supplys 1500 USDC as collateral, 150%.

  • Bob buys 100 points for 100 USDC, now collateral used is 150 USDC and remaining = 1350 USDC.

  • Alice still waits for other buyers, so she never cancels the offer

  • It is now time for the settlement period, and Alice did not get any new buyers.

  • Alice decides not to settle the points anymore and is willing to give Bob the used collateral(150 USDC) and keep all the pointTokens, so she calls the settleAskMaker with 0 as _settledPoints.

  • Now since the function overlook this scenario, it will assume that Alice sold all the points and never return the unused collateral i.e. 1350 USDC.

  • Now, Bob calls closeBidTaker to get the collateral;

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
);
} 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
);
}
}

  • Bob gets 150 USDC from Alice's collateral.

  • The remaining 1350 USDC collateral are stuck in the capital pool and never sent back to Alice.

Impact

loss of unused collateral tokens for the seller and locked in the capital pool

Tools Used

manual

Recommendations

make sure the unused collateral is sent back to the seller in this case.
The fix is complicated for this one as it can introduce new issues and this fix will not be compatible with the other fixes in this function itself(repoted in my other submission).

So it is better to re-design the whole settlement function which does not overlook this scenario.

Updates

Lead Judging Commences

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.