Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Aborted bid-taker will not fully receive the collateral from the aborted ask-maker

fyamf

Summary

In case that an ask maker aborts her offer, it is expected that after taker aborts his order, the taker would receive the full collateral of the maker as punishment to the maker because of not keeping the promise. But, due to a mis-implementation, the taker only receives a portion of maker's collateral that does not include the collateral rate effect.

This breaks a main invariant of the protocol, because the collateral rate is a main mechanism to incentivise makers to keep their promises and ensuring takers that they will be compensated in case of maker's failure.

The flow of bug is:

  • Maker creates an ask offer createOffer

  • Taker places a bid order createTaker

  • Maker aborts her offer abortAskOffer

  • Taker aborts his order abortBidTaker (bug happens here)

Please note that this bug may seem similar to another finding with title Aborted ask offer returns the extra collateral to the maker that leads to breaking a main invariant of the protocol, but the root cause and impacts are different.

Vulnerability Details

  • Suppose Alice(maker) creates an ask offer for 1000 points with $1 as collateral with collateral rate 12000 (equivalent to 120%). In total she deposits $1.2.

  • Bob places a bid order against Alice's offer to buy those 1000 points.

  • Then, Alice aborts her offer.

  • Now, Bob can also abort his order because Alice's offer is already aborted.

  • When Bob calls abortBidTaker, it is expected that the Alice's collateral be dedicated to Bob (even those extra portion of collateral related to collateral rate, i.e. $0.2). Because, Alice is aborting her offer and she is not keeping her promise, so she should be punished. So, we expect that after Bob calls abortBidTaker to have:

userTokenBalanceMap[Bob][USDC][TokenBalanceType.MakerRefund] = $1.2

But due to the miscalcualtion we will have:

userTokenBalanceMap[Bob][USDC][TokenBalanceType.MakerRefund] = $1

The miscalculation happens here where the transferAmount is calculated. It calculates the transfer amount as $1, because the parameters of getDepositAmount are forwarded as:

  • _offerType: ask

  • _collateralRate: 12000 (equivalent to 20%)

  • _amount: $1

  • _isMaker: false

  • _rounding: Floor

uint256 transferAmount = OfferLibraries.getDepositAmount(
preOfferInfo.offerType,
preOfferInfo.collateralRate,
depositAmount,
false,
Math.Rounding.Floor
);

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L677

function getDepositAmount(
OfferType _offerType,
uint256 _collateralRate,
uint256 _amount,
bool _isMaker,
Math.Rounding _rounding
) internal pure returns (uint256) {
/// @dev bid offer
if (_offerType == OfferType.Bid && _isMaker) {
return _amount;
}
​
/// @dev ask order
if (_offerType == OfferType.Ask && !_isMaker) {
return _amount;
}
​
return
Math.mulDiv(
_amount,
_collateralRate,
Constants.COLLATERAL_RATE_DECIMAL_SCALER,
_rounding
);
}

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/libraries/OfferLibraries.sol#L27

While the parameter _isMaker should be set to true.

PoC

In the following test Alice creates a turbo ask offer to sell 1000 points for $1 with collateral rate 12000 (equivalent to 120%), and Bob places a bid order to buy all those 1000 points.
Later, Alice aborts her offer by calling abortAskOffer, and then Bob calls abortBidTaker to abort his order. The output shows that only $1 is dedicated to Bob balance as MakerRefund, while it should be $1.2.

An important note is that in this test, the following code is fixed (which is reported in another finding with title Stealing the fund by misusing the vulnerability in the function abortBidTaker), to be able to show the impact of this finding.

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

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

function test_wrong_maker_refund() public {
address Alice = vm.addr(10); // maker
address Bob = vm.addr(11); // taker
​
////// Alice(maker) creates an ask offer
vm.startPrank(Alice);
deal(address(mockUSDCToken), Alice, 1.2 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
vm.startPrank(Alice);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
0,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
vm.stopPrank();
////////////////
​
///// Bob (taker) creates a bid order
vm.startPrank(Bob);
deal(address(mockUSDCToken), Bob, type(uint256).max);
​
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1000);
​
vm.stopPrank();
/////////////
​
////// Alice aborts her offer
vm.startPrank(Alice);
address stock0Addr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stock0Addr, offerAddr);
​
vm.stopPrank();
/////////////
​
//////// Bob aborts his bid order
vm.startPrank(Bob);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
​
vm.stopPrank();
/////////////
​
console.log(
"Bob balance of USDC in the protocol as MakerRefund: ",
tokenManager.userTokenBalanceMap(
address(Bob),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
)
);
}

The output is:

Logs:
Bob balance of USDC in the protocol as MakerRefund: 1000000000000000000

Impact

  • Aborted bid-taker will not fully receive the collateral from the aborted ask-maker

  • Taker will not be compensated properly if the maker aborts

Tools Used

Recommendations

uint256 transferAmount = OfferLibraries.getDepositAmount(
preOfferInfo.offerType,
preOfferInfo.collateralRate,
depositAmount,
true, // here is modified
Math.Rounding.Floor
);

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L677

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-abortAskOffer-isMaker-false

Valid high severity, the `totalDepositAmount` of collateral computed from the amount of point used (posted to transact) should use the same isMaker flag as when computing the original collateral deposited by maker, if not, the amount available for withdrawal during abortion will be overestimated

Appeal created

fyamf Submitter
10 months ago
cryptomoon Auditor
10 months ago
0xnevi Lead Judge
9 months ago
0xnevi Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.