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

Memory update instead of storage in `listOffer()` allows unauthorized abortion of listed Turbo offers

0xnbvc

Vulnerability Details

The abortAskOffer() function in PreMarkets.sol is used to abort an ask offer.

A turbo offer isn't allowed to be aborted after it's listed, and the listOffer() function has a check to set the abortOfferStatus of the offer to AbortOfferStatus.SubOfferListed if it's a turbo offer that has been listed.
However, it modifies the abortOfferStatus of originOfferInfo in memory instead of storage. This means the change is not persisted to the contract's state:

/// @dev change abort offer status when offer settle type is turbo
if (makerInfo.offerSettleType == OfferSettleType.Turbo) {
address originOffer = makerInfo.originOffer;
>> OfferInfo memory originOfferInfo = offerInfoMap[originOffer];
​
if (_collateralRate != originOfferInfo.collateralRate) {
revert InvalidCollateralRate();
}
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;
}

This is the check in abortAskOffer() that should ensure that only not-listed turbo offers can be aborted:

PreMarkets.sol#L552-L556

if (offerInfo.abortOfferStatus != AbortOfferStatus.Initialized) {
revert InvalidAbortOfferStatus(
AbortOfferStatus.Initialized,
offerInfo.abortOfferStatus
);

As a result, even after a turbo offer is listed, it can still be aborted, which contradicts the intended behavior and allows users to abuse the system by aborting listed turbo offers and breaking chain of offers. Moreover, takers won't get reimbursed the platform fee and trade tax they paid and thus lose funds.

Impact

Disruption of protocol functionality and takers lose the TradeTax (maker bonus) and PlatformFee they paid.

Proof of Concept

Add this PoC in test/PreMarkets.t.sol and run forge test --mt test_PoC_SubOfferListed -vvvv

function test_PoC_SubOfferListed() public {
// user creates an ask turbo offer
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offer0Addr = GenerateAddress.generateOfferAddress(0);
// Offer has been created by user and AbortOfferStatus is set to Initialized
assertEq(uint256(preMarktes.getOfferInfo(offer0Addr).abortOfferStatus), uint256(AbortOfferStatus.Initialized));
​
// user2 creates a taker for the offer and list it as a sub offer
vm.startPrank(user2);
preMarktes.createTaker(offer0Addr, 500);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
// The abortOfferStatus of the offer should be SubOfferListed to prevent the offer to be aborted, but it is still Initialized
assertEq(uint256(preMarktes.getOfferInfo(offer0Addr).abortOfferStatus), uint256(AbortOfferStatus.Initialized));
​
// user can abort the offer after being listed by a taker (user2)
vm.startPrank(user);
address stock0Addr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stock0Addr, offer0Addr);
assertEq(uint256(preMarktes.getOfferInfo(offer0Addr).abortOfferStatus), uint256(AbortOfferStatus.Aborted));
}

Recommendations

Modify the abortAskOffer() function to update the state variable instead of the memory variable:

/// @dev change abort offer status when offer settle type is turbo
if (makerInfo.offerSettleType == OfferSettleType.Turbo) {
address originOffer = makerInfo.originOffer;
- OfferInfo memory originOfferInfo = offerInfoMap[originOffer];
+ OfferInfo storage originOfferInfo = offerInfoMap[originOffer];
​
if (_collateralRate != originOfferInfo.collateralRate) {
revert InvalidCollateralRate();
}
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;
}
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.