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

Wrong calculation of `depositAmount` in the `abortBidTaker` leads to loss of funds

0xjoyboy03

Summary

wrong math for depositAmount in the abortBidTaker function leads to loss of funds for the users

Vulnerability Details

when the origin offer aborts its own offer by calling the abortAskOffer function, sub-offers of the origin offer are eligible to abort their own by calling the abortBidTaker function. however, they claim 0 amount after that because of the wrong math in the depositAmount calculation

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
);
// ...
}
​

the depositAmount calculation must be stockInfo.points * preOfferInfo.amount / preOfferInfo.points but it's not. since there is a precision for the amount (1e18), this will always be greater than the numerator which leads to 0

The following code demonstrates this scenario by adding it to the Premarkets.t.sol:

function test_wrongMathOnAborting() public {
address _offer1 = GenerateAddress.generateOfferAddress(0);
address _stock1 = GenerateAddress.generateStockAddress(0);
address _stock2 = GenerateAddress.generateStockAddress(1);
​
vm.prank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
));
​
vm.prank(user2);
preMarktes.createTaker(_offer1, 800);
​
vm.prank(user);
preMarktes.abortAskOffer(_stock1, _offer1);
​
vm.prank(user2);
preMarktes.abortBidTaker(_stock2, _offer1);
​
uint256 expectedAmount = 800 * 0.01 * 1e18 / 1000;
uint256 a = tokenManager.userTokenBalanceMap(
address(user2),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
assertTrue(expectedAmount != a);
assertEq(a, 0);
}

Impact

Suboffers of the origin offer can't claim their funds after the origin calls the abortAskOffer function

Tools Used

manual review, unit tests

Recommendations

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