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

Incorrect Transfer Amount Calculation in abortBidTaker Function

Tomas0707

Summary

The abortBidTaker() function in the PreMarkets.sol contract miscalculates the depositAmount, leading to a situation where bid takers do not receive the correct amount of MakerRefund tokens when an offer is aborted.

Vulnerability Details

This is the calculation of depositAmount in the abortBidTaker function.

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

This calculation is incorrect and often results in a value of 0, which prevents the correct amount of MakerRefund tokens from being allocated to the bid taker. The intended logic was likely to calculate the proportional amount of deposit relative to the points and offer amount, but the formula is flawed.

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

Impact

Due to this miscalculation, bid takers are not properly refunded their MakerRefund tokens when an offer is aborted. This can lead to financial loss for users and a significant trust issue within the Tadle protocol.

Proof of Concept

This is the test code.

function test_ask_turbo_chain() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
vm.stopPrank();
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 700);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
vm.prank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
vm.prank(user2);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
}

The result is like this.

│ ├─ [14593] PreMarktes::abortBidTaker(0x7AFBC26d177182f1C9b99c324a78c99aF4545ba8, 0xE619a2899a8db14983538159ccE0d238074a235d) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ ├─ [8126] UpgradeableProxy::addTokenBalance(4, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 0)
│ │ │ ├─ [7599] TokenManager::addTokenBalance(4, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 0) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(2) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(3) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0]
│ │ │ │ ├─ emit AddTokenBalance(accountAddress: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 4, amount: 0)
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Return]
│ │ ├─ emit AbortBidTaker(stock: 0xE619a2899a8db14983538159ccE0d238074a235d, authority: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
│ │ └─ ← [Stop]
│ └─ ← [Return]

As you can see, 0 token balances are added to the bid taker (user2).

Tools Used

Manual code review.

Recommendations

The calculation should be corrected to accurately reflect the intended logic.

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.