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

The depositAmount calculation formula in the `PreMarkets::abortBidTaker()` function is incorrect, causing the caller to lose all collateral.

joicygiore

Summary

The depositAmount calculation formula in the PreMarkets::abortBidTaker() function is incorrect, causing the caller to lose all collateral.

Vulnerability Details

The issue is in the PreMarkets::abortBidTaker() function. The depositAmount is calculated using the formula highlighted below (indicated by @>). This value is then used to compute transferAmount, which is subsequently passed to the tokenManager.addTokenBalance() method. This method updates the amount corresponding to TokenBalanceType.MakerRefund in the storage mapping.

However, due to an error in the depositAmount calculation formula, depositAmount is set to 0, resulting in the caller losing all collateral.

function abortBidTaker(address _stock, address _offer) external {
StockInfo storage stockInfo = stockInfoMap[_stock];
OfferInfo storage preOfferInfo = offerInfoMap[_offer];
​
​
// SNIP...
​
​
@> 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

To demonstrate the issue, add the following test code to test/PreMarkets.t.sol and run it:

function test_PreMarktes_abortBidTaker_depositAmount_error() public {
///////////////////////
// user createOffer //
///////////////////////
vm.startPrank(user);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
///////////////////////
// user1 createTaker //
///////////////////////
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
preMarktes.createTaker(offerAddr, 1000);
vm.stopPrank();
​
////////////////////////
// user abortAskOffer //
////////////////////////
vm.prank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
/////////////////////////
// user1 abortAskTaker //
/////////////////////////
vm.startPrank(user1);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
vm.stopPrank();
​
////////////////////////
// check user balance //
////////////////////////
// user1
uint256 user1mockUSDCTokenAmount_MakerRefund = tokenManager.userTokenBalanceMap(
address(user1),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
console2.log("user1mockUSDCTokenAmount_MakerRefund:",user1mockUSDCTokenAmount_MakerRefund);
}
// [PASS] test_PreMarktes_abortBidTaker_depositAmount_error() (gas: 921095)
// Logs:
// user1mockUSDCTokenAmount_MakerRefund: 0

The test output shows that depositAmount == 0, resulting in the caller losing all collateral.

Code Snippet

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

Impact

The incorrect calculation of depositAmount in the PreMarkets::abortBidTaker() function causes the caller to lose all collateral.

Tools Used

Manual Review

Recommendations

Use the correct calculation formula: depositAmount = preOfferInfo.amount * stockInfo.points / preOfferInfo.points:

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

After applying this fix, the test should pass successfully, as shown below:

[PASS] test_PreMarktes_abortBidTaker_depositAmount_error() (gas: 940998)
Logs:
user1mockUSDCTokenAmount_MakerRefund: 10000000000000000
Updates

Lead Judging Commences

0xnevi Lead Judge over 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.

Give us feedback!