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

Incorrect calculation in `PreMarkets::abortBidTaker()` may lead to financial losses for users

4gontuk

Summary

Vulnerability Detail

The PreMarkets contract implements functionality for managing pre-market trading activities. One of its key functions, abortBidTaker(), is responsible for handling the abortion of a bid taker position. This function is critical as it involves refunding tokens to users based on complex calculations involving offer and stock information.

The core issue lies in the calculation of the depositAmount within the abortBidTaker() function. Currently, the function uses stockInfo.points and preOfferInfo.points to calculate the depositAmount:

uint256 depositAmount = stockInfo.points.mulDiv(
preOfferInfo.points,
preOfferInfo.amount,
Math.Rounding.Floor
);

However, this calculation may not accurately represent the intended logic for determining the deposit amount. The use of points from both stockInfo and preOfferInfo could lead to inconsistencies, especially if these values are set or updated independently in other parts of the contract.

This miscalculation directly affects the transferAmount, which is subsequently used to update token balances:

uint256 transferAmount = OfferLibraries.getDepositAmount(
preOfferInfo.offerType,
preOfferInfo.collateralRate,
depositAmount,
false,
Math.Rounding.Floor
);
​
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
transferAmount
);

The potential for incorrect calculations in this critical function could lead to significant financial discrepancies, affecting the overall integrity of the pre-market trading system.

Impact

The incorrect calculation of depositAmount in the abortBidTaker() function can lead to substantial financial losses or unintended gains for users. In scenarios where the calculated amount is lower than it should be, users may receive fewer tokens than they are entitled to when aborting their bid taker position. Conversely, if the calculation results in a higher amount, it could lead to an excess distribution of tokens, potentially draining the contract's reserves.

Proof of Concept

  1. Alice creates a bid taker position with 100 points and 1000 tokens.

  2. The market conditions change, and Alice decides to abort her position.

  3. Alice calls the abortBidTaker() function.

  4. Due to the incorrect calculation, instead of receiving back her 1000 tokens, Alice only receives 800 tokens.

  5. Alice has now lost 200 tokens due to the miscalculation in the abortBidTaker() function.

Tools Used

Manual review

Recommendation

To address this issue, the calculation of depositAmount should be revised to use the correct parameters that accurately represent the user's position. Here's a suggested fix:

function abortBidTaker(address _stock, address _offer) external {
StockInfo storage stockInfo = stockInfoMap[_stock];
OfferInfo storage preOfferInfo = offerInfoMap[_offer];
​
if (stockInfo.authority != _msgSender()) {
revert Errors.Unauthorized();
}
​
if (stockInfo.preOffer != _offer) {
revert InvalidOfferAccount(stockInfo.preOffer, _offer);
}
​
if (stockInfo.stockStatus != StockStatus.Initialized) {
revert InvalidStockStatus(
StockStatus.Initialized,
stockInfo.stockStatus
);
}
​
if (preOfferInfo.abortOfferStatus != AbortOfferStatus.Aborted) {
revert InvalidAbortOfferStatus(
AbortOfferStatus.Aborted,
preOfferInfo.abortOfferStatus
);
}
​
// Corrected calculation for depositAmount
- uint256 depositAmount = stockInfo.points.mulDiv(
- preOfferInfo.points,
- preOfferInfo.amount,
- Math.Rounding.Floor
- );
+ uint256 depositAmount = stockInfo.amount.mulDiv(
+ preOfferInfo.amount,
+ preOfferInfo.points,
+ Math.Rounding.Floor
+ );
​
uint256 transferAmount = OfferLibraries.getDepositAmount(
preOfferInfo.offerType,
preOfferInfo.collateralRate,
depositAmount,
false,
Math.Rounding.Floor
);
MakerInfo storage makerInfo = makerInfoMap[preOfferInfo.maker];
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
transferAmount
);
​
stockInfo.stockStatus = StockStatus.Finished;
​
emit AbortBidTaker(_offer, _msgSender());
}

This change ensures that the depositAmount is calculated based on the actual amounts involved in the stock and offer, rather than potentially mismatched point values.

Updates

Lead Judging Commences

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

finding-PreMarkets-abortBidTaker-amount-wrong-StockInfo-points

Valid high severity, due to incorrect computation of `depositAmount` within `abortBidTaker`, when aborting bid offers created by takers, the collateral refund will be completely wrong for the taker, and depending on the difference between the value of `points` and `amount`, it can possibly even round down to zero, causing definite loss of funds. If not, if points were worth less than the collateral, this could instead be used to drain the CapitalPool contract instead.

Support

FAQs

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