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

Bid taker is refunded with wrong amount of collateral token when aborting order

baz1ka

Relevant GitHub Links

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

Summary

For various reasons, Ask Maker can abort order with PreMarktes.abortAskOffer(). In that case, Bid Taker must execute PreMarktes.abortBidTaker() to get provided collateral back. But because of a mistake in calculation refund amount Bid Taker is getting refunded with nothing.

Vulnerability Details

Incorrect calculation of refunded amount occurs on these lines:

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

Refund amount should be the same as user deposited when created taker. In provided code above user would get less collateral back because all values are divided by preOfferInfo.amount which is amount of collateral token provided by Ask Maker. Because most tokens have 18 decimals numerator of expression is drastically lesser than the denominator and in most cases taker would be refunded with 0 collateral tokens.

Impact

The taker loses his order funds.

Proof of Concept

Modify test/PreMarkets.t.sol test with next code and execute this test with forge test --match-test test_abort_bid_taker_can_not_refund_tokens:

function test_abort_bid_taker_can_not_refund_tokens() public {
CreateOfferParams memory createOfferParams = CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
);
​
vm.prank(user);
preMarktes.createOffer(createOfferParams);
​
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stock1Addr = GenerateAddress.generateStockAddress(1);
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
vm.prank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
vm.startPrank(user1);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
vm.stopPrank();
​
// refund amount is 0 because if large amount of collateral token
uint256 refundedTokensAmount = tokenManager.userTokenBalanceMap(
user1,
createOfferParams.tokenAddress,
TokenBalanceType.MakerRefund
);
​
assertEq(refundedTokensAmount, 0);
}

Recommended Mitigation Steps

Use the same calculation as in PreMarktes.createTaker():

uint256 depositAmount = stockInfo.points.mulDiv(
- preOfferInfo.points,
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.