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

The `PreMarkets.abortAskOffer` function refunds more collateral than actual value.

kupiasec

Github link

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

Summary

The PreMarkets.abortAskOffer function calculates the totalDepositAmount incorrectly.
As a result, more collateral is refunded to offer authority.

Vulnerability Details

In the PreMarkets.abortAskOffer function, it refunds collaterals for unused points from L624.
transferAmount is the seller deposited collateral from L595 and totalDepositAmount must be collateral amount for used points.

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

L595: uint256 transferAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
remainingAmount,
true,
Math.Rounding.Floor
);
uint256 totalUsedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Ceil
);
uint256 totalDepositAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
totalUsedAmount,
L611: false, // @audit-issue true
Math.Rounding.Ceil
);
uint256 makerRefundAmount;
if (transferAmount > totalDepositAmount) {
makerRefundAmount = transferAmount - totalDepositAmount;
} else {
makerRefundAmount = 0;
}
ITokenManager tokenManager = tadleFactory.getTokenManager();
L624: tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);

totalDepositAmount is calculated using _offerType parameter as OfferType.Ask and _isMaker parameter as false from L611.
Thus, the OfferLibraries.getDepositAmount function returns _amount and this means that totalDepositAmount is just same as totalUsedAmount, not collateral amount for used points.

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

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
L40: if (_offerType == OfferType.Ask && !_isMaker) {
return _amount;
}
​
return
Math.mulDiv(
_amount,
_collateralRate,
Constants.COLLATERAL_RATE_DECIMAL_SCALER,
_rounding
);
}

As a result, makerRefundAmount is greater than collateral amount for unused points.

Impact

This causes the protocol's loss of funds.

Tools Used

Manual Review

Recommendations

It is recommended to change the code as following:

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

uint256 totalDepositAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
totalUsedAmount,
- false,
+ true,
Math.Rounding.Ceil
);
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year 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

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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