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

Incorrect Calculation in `PreMarkets::abortBidTaker` Function Due to the Miscalculation of `depositAmount`

simon0417

Relevant GitHub Links

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

Summary

The function PreMarkets::abortBidTaker contains a critical error in calculating the depositAmount. The current calculation is incorrect, which could lead to significant discrepancies in token transfers.

Vulnerability Details

The depositAmount is intended to represent the amount of tokens deposited by the bid taker to purchase points from the offer maker. It should be calculated using the formula:

stockInfo.points * preOfferInfo.amount / preOfferInfo.points

However, the code incorrectly calculates it as:

stockInfo.points * preOfferInfo.points / preOfferInfo.amount

This incorrect calculation can lead to the wrong amount of tokens being transferred.

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
);
}
​
@> 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
);
​
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());
}

POC

copy and paste the code below to file PreMartkets.t.sol and run it.

function test_abort_turbo_offer_balance_calculation() public {
vm.startPrank(user);
//user create turbo offer
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
uint256 beforeBalance = mockUSDCToken.balanceOf(user1);
//user1 take the offer
preMarktes.createTaker(offerAddr, 500);
uint256 afterBalance = mockUSDCToken.balanceOf(user1);
uint256 diff = beforeBalance - afterBalance;
vm.stopPrank();
​
vm.prank(user);
//user abort the offer
preMarktes.abortAskOffer(stockAddr, offerAddr);
vm.startPrank(user1);
address stock1Addr = GenerateAddress.generateStockAddress(1);
//user1 abort the bid order
preMarktes.abortBidTaker(stock1Addr, offerAddr);
uint256 afterAbortBalance = mockUSDCToken.balanceOf(user1);
//balance should be the same as the collateral fee should be returned after aborting the bid order but it is different
assert(afterBalance + diff != afterAbortBalance);
vm.stopPrank();
}

Impact

• Likelihood: High
This issue will consistently result in incorrect depositAmount values whenever the function is invoked.

• Impact: High
This could lead to an incorrect number of tokens being transferred to the bid taker, which might never be recovered. Depending on the preOfferInfo.points and preOfferInfo.amount values, the bid taker might receive either more or fewer tokens than intended. If the calculated result is higher than expected, the protocol could suffer a loss of tokens, leading to potential shortages for other users. If the calculated result is lower, a portion of the tokens could become trapped within the protocol.

Tools Used

Manual Review

Recommendations

uint256 depositAmount = stockInfo.points.mulDiv(
++ preOfferInfo.amount,
-- preOfferInfo.points,
++ preOfferInfo.points,
-- preOfferInfo.amount,
Math.Rounding.Floor
);
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.