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

`abortAskOffer` is not refunding the collateral back to the ask offer

0xbrivan2

Vulnerability Details

The abortAskOffer function allows an ask offer maker to abort their offer and reclaim their collateral(+ if no sub-trades have occurred in the case of Turbo mode). The intended behavior of this function is to refund the offer maker with their full collateral, and all ongoing deals with existing takers should be aborted, as confirmed by the sponsor's clarification in the contest channel:
abort behavior confirmation
As highlighted in the sponsor's message: The Maker get all the collateral fund back

However, the current implementation of the function does not refund the maker with their full collateral:

function abortAskOffer(address _stock, address _offer) external {
// ...
int256 remainingAmount;
if (offerInfo.offerStatus == OfferStatus.Virgin) {
remainingAmount = offerInfo.amount;
} else {
remainingAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
}
​
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,
false,
Math.Rounding.Ceil
);
​
///@dev update refund amount for offer authority
uint256 makerRefundAmount;
if (transferAmount > totalDepositAmount) {
makerRefundAmount = transferAmount - totalDepositAmount;
} else {
makerRefundAmount = 0;
}
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
​
offerInfo.abortOfferStatus = AbortOfferStatus.Aborted;
offerInfo.offerStatus = OfferStatus.Settled;
​
emit AbortAskOffer(_offer, _msgSender());
}

In the snippet above, the offer maker is only refunded with the unused portion of the amount (the margin), while the transferAmount represents the unused amount and the totalDepositAmount represents the used points amount:

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

Note: Ignore the incorrect calculation of remainingAmount when the offer is not in the Virgin status, as it should be based on offerInfo.points - offerInfo.usedPoints, not offerInfo.usedPoints. Still, the intended is that remainingAmount represents the amount of the unused points

Impact

The abortAskOffer function is not working as intended and fails to refund the ask offer maker with the full collateral they initially deposited.

Tools Used

Manual Review

Recommendations

Ensure that the ask offer maker is refunded with their entire collateral when aborting the ask offer, as originally intended.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

[invalid] finding-PreMarkets-abortAskOffer-makerRefundAmount-wrong-compute

Note, #148, #826, #1784 all stems from the fact that order statuses are not appropriately updated when a taker order is created against a maker offer. If the status is switched to `Ongoing/Filled` respectively, the virgin checks will fail appropriately and subsequently all this issues will be fixed. They could possibly be duplicates, so leaving open for appeal. This actually is closely related to issue #148. If a taker has created an Bid/Ask offer agains a original maker offer and the Status is adjusted to Ongoing/Filled accordingly, then this wrong refund won't occur, so could be duplicates, given only Virgin orders can be aborted (same applies for canceled orders, since only virgin orders are supposed to be able to be canceled). Since the status update is incorrect, then if a virgin order that is partially filled is aborted, the collateral is not returned correctly even though the order is not settled, causing a loss of funds to makers.

Appeal created

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

[invalid] finding-PreMarkets-abortAskOffer-makerRefundAmount-wrong-compute

Note, #148, #826, #1784 all stems from the fact that order statuses are not appropriately updated when a taker order is created against a maker offer. If the status is switched to `Ongoing/Filled` respectively, the virgin checks will fail appropriately and subsequently all this issues will be fixed. They could possibly be duplicates, so leaving open for appeal. This actually is closely related to issue #148. If a taker has created an Bid/Ask offer agains a original maker offer and the Status is adjusted to Ongoing/Filled accordingly, then this wrong refund won't occur, so could be duplicates, given only Virgin orders can be aborted (same applies for canceled orders, since only virgin orders are supposed to be able to be canceled). Since the status update is incorrect, then if a virgin order that is partially filled is aborted, the collateral is not returned correctly even though the order is not settled, causing a loss of funds to makers.

Support

FAQs

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