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

Makers can abort ASK Offers in TURBO mode even after takers have placed orders and listed their acquired stock

vinica_boy

Summary

In Turbo mode, the initial offer maker is responsible for settling all subsequent trades that stem from their ASK offer. Consequently, the maker should be prevented from canceling the ASK offer if there are any subsequent listings based on the stock originally acquired from it. Permitting the maker to abort the ASK offer under these circumstances would disrupt the continuity of trades, potentially leaving transactions unsettled. However, the current implementation fails to correctly track whether the initial offer has such subsequent listings, leading to potential disruptions in the trading process.

Vulnerability Details

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

Line #342 in listOffer() function inside PreMarkets.sol is intended to change the abortOfferStatus of the initial offer and this variable is later checked in on line #552 in abortAskOffer() in the same file. |
Saving OfferInfo in memory with OfferInfo memory originOfferInfo = offerInfoMap[originOffer]; and then modifying it with originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed; only updates the local memory copy, not the original data in offerInfoMap. This leads to an inconsistent state, as the changes do not persist in the contract's storage.

...
if (makerInfo.offerSettleType == OfferSettleType.Turbo) {
address originOffer = makerInfo.originOffer;
// variable saved in memory, instead of storage
OfferInfo memory originOfferInfo = offerInfoMap[originOffer];
​
if (_collateralRate != originOfferInfo.collateralRate) {
revert InvalidCollateralRate();
}
// changing value of abortOfferStatus, wouldnt change the state variable
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;
}
...
​
function abortAskOffer(address _stock, address _offer) external {
StockInfo storage stockInfo = stockInfoMap[_stock];
OfferInfo storage offerInfo = offerInfoMap[_offer];
​
if (offerInfo.authority != _msgSender()) {
revert Errors.Unauthorized();
}
​
if (stockInfo.offer != _offer) {
revert InvalidOfferAccount(stockInfo.offer, _offer);
}
​
if (offerInfo.offerType != OfferType.Ask) {
revert InvalidOfferType(OfferType.Ask, offerInfo.offerType);
}
​
// Check should prevent of aborting offers which have subsequent listing of stocks acquired from this offer.
if (offerInfo.abortOfferStatus != AbortOfferStatus.Initialized) {
revert InvalidAbortOfferStatus(
AbortOfferStatus.Initialized,
offerInfo.abortOfferStatus
);
}
...
}

The following test shows a scenario, where initial Maker can abort the offer after subsequent listing.

function testAbortAskOfferAfterSubsequentListing() 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 stockAddr1 = GenerateAddress.generateStockAddress(1);
address offerAddr = GenerateAddress.generateOfferAddress(0);
address offerAddr1 = GenerateAddress.generateOfferAddress(1);
​
preMarktes.createTaker(offerAddr, 500);
preMarktes.listOffer(stockAddr1, 500, 12000);
vm.stopPrank();
​
vm.startPrank(user3);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr1, 500);
vm.stopPrank();
​
vm.prank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
}

Impact

While this issue does not cause direct financial losses for the protocol or its users, it can indirectly harm potential traders who may be unable to purchase the points they intended. This could lead to missed opportunities.

Tools Used

Manual review, Foundry.

Recommendations

Update the abortOfferStatus directly in the offerInfoMap storage rather than in memory.
OfferInfo storage originOfferInfo = offerInfoMap[originOffer];

Updates

Lead Judging Commences

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

finding-PreMarkets-listOffer-originIOfferInfo-storage-memory

Valid high severity, because the `abortOfferStatus` of the offer is not updated and persist through `storage` when listing an offer for turbo mode within the `offerInfoMap` mapping, it allows premature abortion given the `abortOfferStatus` defaults to `Initialized`, allowing the bypass of this [check](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L552-L557) here and allow complete refund of initial collateral + stealing of trade tax which can potentially be gamed for profits using multiple addresses

Support

FAQs

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