Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Partial settling ask maker does not refund collateral.

kupiasec

Github link

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L276-L307

Summary

In the DeliveryPlace.settleAskMaker function, it refunds the collateral only if _settledPoints == offerInfo.usedPoints and sets the offerInfo.offerStatus as OfferStatus.Settled.
As a result, if _settledPoints < offerInfo.usedPoints, the authority of ask offer can not receive the collateral.

Vulnerability Details

In the DeliveryPlace.settleAskMaker function, it refunds the collateral only if _settledPoints == offerInfo.usedPoints from L276.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L276-L307

L276: if (_settledPoints == offerInfo.usedPoints) {
[...]
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}

If _settledPoints < offerInfo.usedPoints, collateral can not be refunded.

Impact

If _settledPoints < offerInfo.usedPoints, the authority of ask offer can not receive the collateral.

Tools Used

Manual Review

Recommendations

Change the code to refund the collateral in case of _settledPoints < offerInfo.usedPoints.

Updates

Lead Judging Commences

0xnevi Lead Judge 10 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.