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

If the `offer` creator refuses to execute `DeliveryPlace::settleAskMaker()`, the `Taker` will lose all the tokens they paid.

joicygiore

Summary

If the offer creator refuses to execute DeliveryPlace::settleAskMaker(), the Taker will lose the tokens they paid.

Vulnerability Details

When a user calls PreMarkets::createOffer() to create an offer, they only need to deposit more than 100% of the tokens as collateral.

function createOffer(CreateOfferParams calldata params) external payable {
/**
* @dev points and amount must be greater than 0
* @dev eachTradeTax must be less than 100%, decimal scaler is 10000
@> * @dev collateralRate must be more than 100%, decimal scaler is 10000
*/
// SNIP...
​
{
/// @dev transfer collateral from _msgSender() to capital pool
@> uint256 transferAmount = OfferLibraries.getDepositAmount(
params.offerType,
params.collateralRate,
params.amount,
true,
Math.Rounding.Ceil
);
​
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
tokenManager.tillIn{value: msg.value}(
_msgSender(),
@> params.tokenAddress,
@> transferAmount,
false
);
}
​
// SNIP...
}

The user who calls PreMarkets::createTaker() to execute the transaction triggers the following process:

  1. The internal function PreMarkets::_depositTokenWhenCreateTaker() transfers the required tokens from the caller to the capitalPool.

  2. The internal function PreMarkets::_updateTokenBalanceWhenCreateTaker() updates the balance in the userTokenBalanceMap of the offer creator.

At this stage, the offer creator can withdraw the corresponding balance from the userTokenBalanceMap.

// PreMarkets::createTaker()
function createTaker(address _offer, uint256 _points) external payable {
​
// SNIP...
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
@> _depositTokenWhenCreateTaker(
platformFee,
depositAmount,
tradeTax,
makerInfo,
offerInfo,
tokenManager
);
​
// SNIP..
​
@> _updateTokenBalanceWhenCreateTaker(
_offer,
tradeTax,
depositAmount,
offerInfo,
makerInfo,
tokenManager
);
​
​
/// @dev emit CreateTaker
emit CreateTaker(
_offer,
msg.sender,
stockAddr,
_points,
depositAmount,
tradeTax,
remainingPlatformFee
);
}
// PreMarkets::_depositTokenWhenCreateTaker()
function _depositTokenWhenCreateTaker(
uint256 platformFee,
uint256 depositAmount,
uint256 tradeTax,
MakerInfo storage makerInfo,
OfferInfo storage offerInfo,
ITokenManager tokenManager
) internal {
​
// SNIP...
​
@> tokenManager.tillIn{value: msg.value}(
_msgSender(),
makerInfo.tokenAddress,
transferAmount,
false
);
}
​
// PreMarkets::_updateTokenBalanceWhenCreateTaker()
function _updateTokenBalanceWhenCreateTaker(
address _offer,
uint256 _tradeTax,
uint256 _depositAmount,
OfferInfo storage offerInfo,
MakerInfo storage makerInfo,
ITokenManager tokenManager
) internal {
// SNIP...
​
/// @dev update sales revenue
if (offerInfo.offerType == OfferType.Ask) {
@> tokenManager.addTokenBalance(
@> TokenBalanceType.SalesRevenue,
@> offerInfo.authority,
makerInfo.tokenAddress,
_depositAmount
);
} else {
@> tokenManager.addTokenBalance(
@> TokenBalanceType.SalesRevenue,
@> _msgSender(),
makerInfo.tokenAddress,
_depositAmount
);
}
}

However, the complete process should involve the following steps:

  1. User A creates an offer by calling PreMarkets::createOffer().

  2. User B initiates the transaction by calling PreMarkets::createTaker().

  3. The administrator updates the market by calling SystemConfig::updateMarket().

  4. User A must execute DeliveryPlace::settleAskMaker() to transfer the purchased token to the capitalPool and retrieve the corresponding collateral.

  5. User B calls DeliveryPlace::closeBidTaker() to update the obtained tokens in the userTokenBalanceMap.

If User A fails to execute DeliveryPlace::settleAskMaker(), User B loses all paid tokens and receives nothing. Meanwhile, User A only forfeits the collateral exceeding 100%.

This situation creates an incentive for User A to avoid fulfilling their obligations under DeliveryPlace::settleAskMaker() if the value of their deposited token increases beyond the collateral lost.

Poc

Add the following test code to test/PreMarkets.t.sol and execute it:

In this PoC, the collateralRate is set to 10001, a system-allowed value that results in negligible collateral loss. The collateral amount is insufficient to cover User2's expenditure.

function test_offer_creator_does_not_execute_settleAskMaker() public {
// Cache user's mockPointToken balance
uint256 userStartPointBalance = mockPointToken.balanceOf(user);
// Cache user's mockUSDCToken balance
uint256 userStartBalance = mockUSDCToken.balanceOf(user);
// Cache user2's mockUSDCToken balance
uint256 user2StartBalance = mockUSDCToken.balanceOf(user2);
​
// user calls the PreMarktes::createOffer()
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
10001, // only 10001
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
uint256 userCreateOfferCollateralAmount = userStartBalance - mockUSDCToken.balanceOf(user);
​
console2.log("userCreateOfferCollateralAmount:",userCreateOfferCollateralAmount);
​
​
// Cache user's offerAddr
address userOfferAddr = GenerateAddress.generateOfferAddress(0);
​
// user2 calls the PreMarktes::createTaker()
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(userOfferAddr, 1000);
vm.stopPrank();
uint256 user2CreateTakerAmount = user2StartBalance - mockUSDCToken.balanceOf(user2);
console2.log("user2CreateTakerAmount:",user2CreateTakerAmount);
​
// The user's balance in `userTokenBalanceMap` has been updated, and the user can execute withdraw()
// TokenBalanceType.SalesRevenue
uint256 usermockUSDCTokenAmount_SalesRevenueAfter_user2_createTaker = tokenManager.userTokenBalanceMap(
address(user),
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
console2.log("usermockUSDCTokenAmount_SalesRevenueAfter_user2_createTaker:",usermockUSDCTokenAmount_SalesRevenueAfter_user2_createTaker);
​
// Due to a problem with the authorization in the `TokenManager::_transfer()` method, it is manually executed here
capitalPool.approve(address(mockUSDCToken));
​
// user calls TokenManager::withdraw()
vm.prank(user);
tokenManager.withdraw(address(mockUSDCToken),TokenBalanceType.SalesRevenue);
​
​
// Cache user's mockPointToken balance
uint256 userEndPointBalance = mockPointToken.balanceOf(user);
// Cache user's mockUSDCToken balance
uint256 userEndBalance = mockUSDCToken.balanceOf(user);
// Cache user2's mockUSDCToken balance
uint256 user2EndBalance = mockUSDCToken.balanceOf(user2);
​
console2.log("The amount of mockUSDCToken lost by the user is:",userStartBalance - userEndBalance);
console2.log("The amount of mockUSDCToken lost by the user2 is:",user2StartBalance - user2EndBalance);
assertEq(userEndPointBalance,userStartPointBalance);
}
// [PASS] test_offer_creator_does_not_execute_settleAskMaker() (gas: 912728)
// Logs:
// userCreateOfferCollateralAmount: 10001000000000000
// user2CreateTakerAmount: 10350000000000000
// usermockUSDCTokenAmount_SalesRevenueAfter_user2_createTaker: 10000000000000000
// The amount of mockUSDCToken lost by the user is: 1000000000000
// The amount of mockUSDCToken lost by the user2 is: 10350000000000000

Code Snippet

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L39-L157
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L159-L284
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L813-L837
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L906-L949

Impact

If the offer creator refuses to execute DeliveryPlace::settleAskMaker(), the Taker will lose the tokens they paid.

Tools Used

Manual Review

Recommendations

Potential solutions:

  1. Increase the Minimum Collateral Ratio: Raising the minimum collateral ratio would increase the potential loss for the offer creator, making it less likely for them to abandon their obligations.

  2. Modify the Execution Logic of PreMarkets::createTaker() and DeliveryPlace::settleAskMaker(): Update the token amount handled in PreMarkets::createTaker() -> PreMarkets::_updateTokenBalanceWhenCreateTaker() to be transferred to an intermediate address. Once the offer creator successfully calls DeliveryPlace::settleAskMaker() and transfers the corresponding token, the amount associated with the intermediate address should then be credited to the offer creator’s address. If the offer creator fails to fulfill their obligations, they would forfeit their entire collateral.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

[invalid] finding-DeliveryPlace-owner-do-not-call-settleAskMaker

Invalid, the makers are incentivized to settle offers to earn maker bonuses when subsequent takers and makers make trade using the original collateral put up for points as well as get back their initial collateral. Additionally, if they do not settle on time, they will lose all their initial collateral, forcing the `owner` to come in and perform the settlement and retrieving that collateral. This is noted as a design decision [here](https://tadle.gitbook.io/tadle/how-tadle-works/features-and-terminologies/settlement-and-collateral-rate) If all else fails, the `owner` can come in to settle as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L254-L256) and [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L365-L367) offers to allow closing offers and subsequently allowing refunds. I acknowledge that perhaps a more decentralized

Appeal created

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

finding-PreMarkets-immediate-withdrawal-allow-maker-steal-funds

Valid high severity, given orginal offer makers are not a trusted entity to enforce a settlement. The trade tax set by the maker should be returned back to the takers to avoid abuse of abortion of ask offers to steal trade tax from takers. Note for appeals period: See issue #528 for additional details

Support

FAQs

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