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

on Aborted Ask/Sell offer, malicious BidTaker can use `DeliveryPlace::closeBidTaker` instead of `preMarket::abortBidTaker` to steal extra funds

honour

Summary

When an Ask/Sell offer is aborted via preMarket::abortAskOffer the seller withdraws all remaining collateral deposited minus the amount recieved by any BidTaker on that offer, the bidtaker can then withdraw their initial deposited amount via PreMarkets::abortBidTaker. However , a malicious bidtaker can instead use DeliveryPlace::closeBidTaker - which only assumes the offer was settled(i.e. doesn't account for settling via Aborting) and refunds the bidtaker their deposit * offer's collateralRate

Vulnerability Details

From PreMarkets::abortAskOffer we can confirm/verify that the amount returned to maker (makerRefundAmount) does include all deposited collateral
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L584-L632
Assuming offerInfo.offerStatus == OfferStatus.Virgin for simplicity (issue still exists even if it's not) and remainingAmount = offerInfo.amount.

uint256 transferAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
remainingAmount,
true,
Math.Rounding.Floor
);

transferAmount calculated here equals remainingAmount * collateralRate as _isMaker == true and offerType is Ask

uint256 totalUsedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Ceil
);
uint256 totalDepositAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
totalUsedAmount,
false,
Math.Rounding.Ceil
);

totalDepositAmount is always equal to totalUsedAmount as _isMaker == false & offerType is Ask. makerRefundAmount = totalDepositAmount - totalUsedAmount.
So Maker is redunded all their deposited collateral minus the amount they recieved by any BidTaker on that offer.

PreMarkets::abortBidTaker only refunds the BidTaker deposit amount
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L671-L683

uint256 depositAmount = stockInfo.points.mulDiv(
preOfferInfo.points,
preOfferInfo.amount,
Math.Rounding.Floor
);
​
uint256 transferAmount = OfferLibraries.getDepositAmount(
preOfferInfo.offerType,
preOfferInfo.collateralRate,
depositAmount,
false,
Math.Rounding.Floor
);

depositAmount == transferAmount as _isMaker == false & offerType is Ask

However , from DeliveryPlace::closeBidTaker
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L151-L176

uint256 collateralFee;
if (offerInfo.usedPoints > offerInfo.settledPoints) {//note if the user closses/settles with 0 points , same thing as aborting
if (offerInfo.offerStatus == OfferStatus.Virgin) {//note this branch cannot be reached because the offer was been aborted
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
);
}
}

The amound refunded collateralFee is equal to usedAmount * collateralRate as _isMaker == true and offerType is Ask. collateralFee >= usedAmount because of the invariant collateralRate >= 1 .
The bid taker withdraws more funds than their supposed to.

Impact

HIGH - If offer maker is in cahoots with BidTaker or perhaps same person ,with sufficient capital to cover initial collateral deposit on the offer , this exploit can be repeated over and over to completely drain contract funds

Tools Used

Manual Review

Recommendations

Check the abortOfferStatus of the offer, if aborted then _isMaker == false so collateralFee == usedAmount .

collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
- true,
+ offerInfo.abortOfferStatus == OfferAbortStatus.Aborted ? false : true,
Math.Rounding.Floor
);
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-closeBidTaker-lack-check-abort-status-drain

Valid high, for unsettled ask offers by the original maker, the initial remaining maker collateral is already refunded as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L624-L629)

Support

FAQs

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

Give us feedback!