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

Abort ask offer calculates refund amount wrong

yotov721

Summary

When a user aborts his offer he is refunded a proportional amount of collateral that was not filled. The calculation of the refund amount in abortAskOffer is wrong.

Vulnerability Details

The porotocol lets users create bid-ask offers in an order-book loke manor. The offers need to be backed with collateral. When a user cancels/aborts his offer only the collateral for the unfilled part of the offer is refunded to them.
The problem is that in the `PreMarkets::abortAskOffer` function the calculation of the refund amount is wrong.

Let's first take a look at the `createOffer` function:

function createOffer(CreateOfferParams calldata params) external payable {
...
{
/// @dev transfer collateral from _msgSender() to capital pool
=> uint256 transferAmount = OfferLibraries.getDepositAmount(
params.offerType,
params.collateralRate,
params.amount,
true,
Math.Rounding.Ceil
);
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
=> tokenManager.tillIn{value: msg.value}(
_msgSender(),
params.tokenAddress,
transferAmount,
false
);
}

When a new offer is created funds are transfered from the user to the storing contract - `CapitalPool`

Taking a look at abortAskOffer:

function abortAskOffer(address _stock, address _offer) external {
...SNIP Checks...
​
uint256 remainingAmount;
​
=> if (offerInfo.offerStatus == OfferStatus.Virgin) { // If the offer has NOT been filled at all
=> remainingAmount = offerInfo.amount;
} else { // if offer is partially filled
=> remainingAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
}
...
}

If the offer has NOT been filled at all (offer status is Virgin) the refund remainingAmount is set to the total deposit amount. Else if the offer is partially filled (not status virgin) the remainingAmount is calculated by:

remainingAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);

This is incorrect as it calculates the used amount instead of the remaining amount of points.
A little background for points and pointsUsed - points are inputed by the maker at order creation and are proportionally equal to the amount of tokens deposited. For example if a user opens an ask/sell offer for 200 TokenA and sets 100 point (2:1) and another user wants to buy the equivalent of 40 points that would be 80 tokens. When the market taker fill the bid part of the offer the `usedPoints` are incremented by 40.

2.

After that the `transferAmount` is calculated. This is the unfilled amount plus proportional collateral

uint256 transferAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType, // offer type = Ask
offerInfo.collateralRate,
remainingAmount,
true, // maker = true
Math.Rounding.Floor
);

Then the used amount is calculated analogically - only here the transferAmount is named totalDepositAmount which actually is total used amount excluding the over-collateralisation part:

uint256 totalUsedAmount = offerInfo.amount.mulDiv(
=> offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Ceil
);
=> uint256 totalDepositAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
totalUsedAmount,
false,
Math.Rounding.Ceil
);

At last the makerRefundAmount is calculated wrong because it compares the used/filled and unused amounts of tokens/collateral. These two values should sum up to the total amount.

if (transferAmount > totalDepositAmount) {
makerRefundAmount = transferAmount - totalDepositAmount;
} else {
makerRefundAmount = 0;
}

This whole code block should be removed. Take the scenario where 20 % of the order is filled and the maker wants to close it - then he would get refunded 60 % of the unfilled amount, not 80. The logic being totalDepositAmount = 20, transferAmount = 80. transferAmount > totalDepositAmount evaluates to true
hence the refund is transferAmount - totalDepositAmount = 80 - 20 = 60.
In the other scenario if the filled amount is greater than the refund amount the maker would not ger refunded.

Impact

User funds stuck in the protocol

Tools Used

Manual Review

Recommendations

In order to calculate the remainingAmount properly multiply by remaining points instead of used points :

remainingAmount = offerInfo.amount.mulDiv(
- offerInfo.usedPoints,
+ offerInfo.points - offerInfo.usedPoints, // points left
offerInfo.points,
Math.Rounding.Floor
);

2.
Replace the check with another verifying that the sum of the filled and unfilled assets is <= to total deposit amount

- uint256 makerRefundAmount;
- if (transferAmount > totalDepositAmount) {
- makerRefundAmount = transferAmount - totalDepositAmount;
- } else {
- makerRefundAmount = 0;
- }
​
+ makerRefundAmount = transferAmount;
​
+ uint256 totalAmount = OfferLibraries.getDepositAmount(
+ offerInfo.offerType, // Ask
+ offerInfo.collateralRate,
+ offerInfo.points, // total points
+ false, // not maker
+ Math.Rounding.Ceil
+ );
​
+ if (transferAmount + totalDepositAmount > totalAmount) revert (); // or handle it another way
Updates

Lead Judging Commences

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

[invalid] finding-PreMarkets-abortAskOffer-remainingAmount-compute

Valid high, for cancelled offers, the unused collateral should be returned back to the maker. The `remainingAmount` is calculated wrongly with regards to usedPoints instead of unused points. Note: See comments under 826 and 907 for invalidation reasons

Appeal created

0xnevi Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-PreMarkets-abortAskOffer-remainingAmount-compute

Valid high, for cancelled offers, the unused collateral should be returned back to the maker. The `remainingAmount` is calculated wrongly with regards to usedPoints instead of unused points. Note: See comments under 826 and 907 for invalidation reasons

Support

FAQs

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