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

`SettleAskMaker` function doesn't update the user's balance when they close trades with settlepoints less than the used points

bigsam

Summary

A critical vulnerability has been identified in the `SettleAskMaker` function, where the maker's refund is incorrectly handled if the `settledPoints` are not equal to the `usedPoints`. This issue results in the maker losing a portion of their funds because the function does not update the user's account correctly in this scenario.

Vulnerability Details

The `SettleAskMaker` function is responsible for refunding the maker when they close an ASK offer. The refund amount is calculated based on the `settledPoints` and `usedPoints`. However, the current implementation only updates the maker's account if the `settledPoints` are equal to the `usedPoints`. If the `settledPoints` are less than the `usedPoints`, the function fails to refund the correct amount(refunds 0), leading to a loss of funds for the maker.

### Vulnerable Code

The issue arises in the following code segment:

/**
* @notice Settle ask maker
* @dev caller must be offer authority
* @dev offer status must be Virgin or Canceled
* @dev market place status must be AskSettling
* @param _offer offer address
* @param _settledPoints settled points
*/
function settleAskMaker(address _offer, uint256 _settledPoints) external {
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
MarketPlaceInfo memory marketPlaceInfo,
MarketPlaceStatus status
) = getOfferInfo(_offer);
​
if (_settledPoints > offerInfo.usedPoints) {
revert InvalidPoints();
}
​
if (marketPlaceInfo.fixedratio) {
revert FixedRatioUnsupported();
}
​
if (offerInfo.offerType == OfferType.Bid) {
revert InvalidOfferType(OfferType.Ask, OfferType.Bid);
}
​
if (
offerInfo.offerStatus != OfferStatus.Virgin &&
offerInfo.offerStatus != OfferStatus.Canceled
) {
revert InvalidOfferStatus();
}
​
if (status == MarketPlaceStatus.AskSettling) {
if (_msgSender() != offerInfo.authority) {
revert Errors.Unauthorized();
}
} else {
if (_msgSender() != owner()) {
revert Errors.Unauthorized();
}
if (_settledPoints > 0) {
revert InvalidPoints();
}
}
​
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint *
_settledPoints;
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);
}
​
uint256 makerRefundAmount;
if (_settledPoints == offerInfo.usedPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
​
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}
@audit >>> //@audit NOTE if settled point is not equal to use point we update the settleaskoffer without sending funds HIGH-2 NOTE
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
perMarkets.settledAskOffer(
_offer,
_settledPoints,
settledPointTokenAmount
);
​
emit SettleAskMaker(
makerInfo.marketPlace,
offerInfo.maker,
_offer,
_msgSender(),
_settledPoints,
settledPointTokenAmount,
makerRefundAmount
);
}
​

  • In this implementation, when `_settledPoints != offerInfo.usedPoints`, the function does not update the maker’s balance - 0, leading to a loss of the maker’s funds.

  • The absence of logic to handle the scenario where `settledPoints < usedPoints` means that the remaining funds are not refunded, and the maker loses those funds.

Look at the implementation in SettleAsktaker function, both conditions there are well captured.

@audit>> if (_settledPoints == stockInfo.points) {
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
collateralFee
);
@audit>> } else {
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
offerInfo.authority,
makerInfo.tokenAddress,
collateralFee
);
}

Impact

This vulnerability results in a significant financial loss for the maker when the `settledPoints` are not equal to the `usedPoints`. Makers lose a portion of their funds due to the incorrect handling of the refund process

Tools Used

- Manual code review

Recommendations

To mitigate this issue, the code should include logic to handle cases where `settledPoints < usedPoints`. This can be done by nesting an `else` statement to manage the condition when an order with `settledPoints` less than `usedPoints` is being settled.

### Suggested Code Modification

Kindly review the added logic below, more im=ntegration might still be possible, but this is a possible fix.

/**
* @notice Settle ask maker
* @dev caller must be offer authority
* @dev offer status must be Virgin or Canceled
* @dev market place status must be AskSettling
* @param _offer offer address
* @param _settledPoints settled points
*/
function settleAskMaker(address _offer, uint256 _settledPoints) external {
(
​
------------------------------------------------------
uint256 makerRefundAmount;
if (_settledPoints == offerInfo.usedPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
​
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}
​
else {
​
++ uint256 usedAmount = offerInfo.amount.mulDiv(
++ _settledPoints,
++ offerInfo.points,
++ Math.Rounding.Floor
++ );
​
++ makerRefundAmount = OfferLibraries.getDepositAmount(
++ offerInfo.offerType,
++ offerInfo.collateralRate,
++ usedAmount,
++ true,
++ Math.Rounding.Floor
++ );
++ }
​
++ tokenManager.addTokenBalance(
++ TokenBalanceType.SalesRevenue,
++ _msgSender(),
++ makerInfo.tokenAddress,
++ makerRefundAmount
++ );
​
​
++ }
​
​
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
perMarkets.settledAskOffer(
_offer,
_settledPoints,
settledPointTokenAmount
);

This update ensures that the maker's balance is correctly adjusted and that all funds are refunded when the order is settled, regardless of the relationship between `settledPoints` and `usedPoints`.

Updates

Lead Judging Commences

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

finding-DeliveryPlace-settleAskTaker-settleAskMaker-partial-settlements

Valid high, in settleAskTaker/settleAskMaker, if the original offer maker performs a partial final settlement, the existing checks [here](https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L356-L358) and [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L230-L232) will cause an revert when attempting to complete a full settlement, resulting in their collateral being locked and requiring a rescue from the admin. To note, although examples in the documentation implies settlement in a single click, it is not stated that partial settlements are not allowed, so I believe it is a valid user flow.

Support

FAQs

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