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

Incomplete collateral refund to ask-offer makers after point settlement

0xbrivan2

Summary

The settleAskMaker function does not return the full collateral to ask makers when they settle their points, particularly when the offer's points are only partially used. This results in ask makers receiving only the sales revenue that was already available for withdraw, not the full amount of collateral they are entitled to, leaving part of their collateral stuck in the contract.

Vulnerability Details

When creating ask offers, makers are required to deposit collateral equivalent to at least 100% of the value they are offering:

function createOffer(CreateOfferParams calldata params) external payable {
// ...
/// @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
);
​
// ...
}

This collateral acts as a security measure and can be liquidated by offer takers if the ask makers do not settle their points during the settlement period, as outlined in closeBidTaker.

During the settlement period, ask makers can call the settleAskMaker function to settle the points they sold. The issue arises when the offer’s points are partially used (offer.usedPoints < offer.points). If the maker settles all the used points, the function does not return the full collateral back to the maker:

function settleAskMaker(address _offer, uint256 _settledPoints) external {
​
// ...
uint256 makerRefundAmount;
if (_settledPoints == offerInfo.usedPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
​
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}
​
// ...
}

As shown above, if the ask maker settles all his used points, and the offer points were used partially, the offer status is Ongoing and the following block will be executed to return the collateral back to the offer maker:

else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
​
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
​

In the code above, if the ask maker settles all the used points and the offer points were partially used, the logic executed will refund only the usedAmount (the amount corresponding to the used points) as sales revenue, not the full collateral that was initially deposited. This is incorrect because the sales revenue is already available for the maker’s withdrawal once the points are sold, as handled in PreMarket::createTaker:

function createTaker(address _offer, uint256 _points) external payable {
// ...
​
_updateTokenBalanceWhenCreateTaker(
_offer,
tradeTax,
depositAmount,
offerInfo,
makerInfo,
tokenManager
);
// ...
}
​
function _updateTokenBalanceWhenCreateTaker(
address _offer,
uint256 _tradeTax,
uint256 _depositAmount,
OfferInfo storage offerInfo,
MakerInfo storage makerInfo,
ITokenManager tokenManager
) internal {
// ...
if (offerInfo.offerType == OfferType.Ask) {
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
offerInfo.authority,
makerInfo.tokenAddress,
_depositAmount
);
}
// ...
}

Proof Of Concept

Consider the following example, which is demonstrated by the test case below:

  1. Alice, an ask-offer maker, lists 1,000 points for sale at $1 per unit and deposits $1,000 as collateral.

  2. Bob, a buyer, purchases 500 points from Alice for $500. Alice [can withdraw the $500 immediately](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L934-L940)

  3. The owner updates market info, sets the TGE event, and the settlment period starts.

  4. Alice needs to settle only 500 points for Bob. So, she calls DeliveryPlace::settleAskMaker

    4.1 Because alice's offer is Ongoing and she settles all required points, the following block from settleAskMaker will be executed:

    else {
    uint256 usedAmount = offerInfo.amount.mulDiv(
    offerInfo.usedPoints,
    offerInfo.points,
    Math.Rounding.Floor
    );
    makerRefundAmount = OfferLibraries.getDepositAmount(
    offerInfo.offerType,
    offerInfo.collateralRate,
    usedAmount,
    true,
    Math.Rounding.Floor
    );
    }
    tokenManager.addTokenBalance(
    TokenBalanceType.SalesRevenue,
    _msgSender(),
    makerInfo.tokenAddress,
    makerRefundAmount
    );

    Alice will only receive the sales revenue of 500 USDC, which was already available after Bob’s purchase, instead of the full 1,000 USDC collateral. This results in Alice not receiving the full collateral amount she is entitled to.

Please, notice that Alice had to get at the end 1500 USDC (500 USDC from Bob's purchase, and 1000 USDC back from the deposited collateral since she settled all the used points). But, she got 1000 USDC only (500 USDC from collateral is not refunded)

To reproduce to test below, some parts of the codebase need to be updated given that there are other vulnerabilities connected here:

  1. Ongoing status needs to be updated after someone takes the offer (explained in detail in another report):

// PreMarket::createTaker
offerInfo.usedPoints = offerInfo.usedPoints + _points;
+ offerInfo.offerStatus = OfferStatus.Ongoing;

  1. Include check for Ongoing status on DeliveryPlace::settleAskMaker (explained in details in another report):

// DeliveryPlace::settleAskMaker
if (
offerInfo.offerStatus != OfferStatus.Virgin &&
offerInfo.offerStatus != OfferStatus.Canceled
+ && offerInfo.offerStatus != OfferStatus.Ongoing
) {
revert InvalidOfferStatus();
}

  1. Fix the vulnerability that allows users to keep withdrawing in TokenManager::withdraw (explained in details in another report):

// TokenManager::withdraw
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] -= claimAbleAmount;
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);

Now, copy and paste the following test case into test/PreMarkets.t.sol:

function test_AskMakersDoNotGetTheirCollateralBackAfterSettlingPoints() public {
vm.startPrank(user1);
capitalPool.approve(address(mockUSDCToken));
​
// Alice (user) create an Ask offer of 1,000 points with amount $1,000
vm.startPrank(user);
uint points = 1000;
uint amount = 1000 * 1e18;
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
points,
amount,
10000, // 100% collateral
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
// Bob (user2) takes the offer and buys 500 points from Alice
vm.startPrank(user2);
preMarktes.createTaker(offerAddr, 500);
​
// Alice withdrwas her revenue from Bob's purchase
vm.startPrank(user);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
​
// owner sets TGE
vm.startPrank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
uint usdcBalanceBeforeSettling = mockUSDCToken.balanceOf(address(user));
vm.startPrank(user);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.settleAskMaker(offerAddr, 500); // Alice settles 500 points to Bob
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
uint usdcBalanceAfterSettling = mockUSDCToken.balanceOf(address(user));
// Alice was refunded only 500 USDC of collateral instead of 1000 USDC
assert(usdcBalanceAfterSettling < usdcBalanceBeforeSettling + amount);
// 500 USDC are not refunded
assert(usdcBalanceAfterSettling - usdcBalanceBeforeSettling == 500 * 1e18);
​
}
forge test --mt test_AskMakersDoNotGetTheirCollateralBackAfterSettlingPoints

Impact

The full collateral is not returned to ask makers after settling their points, leaving part of their collateral stuck in the contract.

Recommendations

The settleAskMaker function is incorrectly crediting the maker with revenue that is already available for withdrawal instead of returning the collateral. Below is a suggested fix for the settleAskMaker function:

function settleAskMaker(address _offer, uint256 _settledPoints) external {
​
// ...
- uint256 makerRefundAmount;
if (_settledPoints == offerInfo.usedPoints) {
- if (offerInfo.offerStatus == OfferStatus.Virgin) {
- makerRefundAmount = OfferLibraries.getDepositAmount(
- offerInfo.offerType,
- offerInfo.collateralRate,
- offerInfo.amount,
- true,
- Math.Rounding.Floor
- );
- } else {
- uint256 usedAmount = offerInfo.amount.mulDiv(
- offerInfo.usedPoints,
- offerInfo.points,
- Math.Rounding.Floor
- );
​
- makerRefundAmount = OfferLibraries.getDepositAmount(
- offerInfo.offerType,
- offerInfo.collateralRate,
- usedAmount,
- true,
- Math.Rounding.Floor
- );
- }
+ uint makerRefundAmount = OfferLibraries.getDepositAmount(
+ offerInfo.offerType,
+ offerInfo.collateralRate,
+ offerInfo.amount,
+ true,
+ Math.Rounding.Floor
+ );
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}
}
Updates

Lead Judging Commences

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

finding-DeliveryPlace-settleAskTaker-settleAskMaker-partial-settlements

Valid high, in settleAskTaker/settleAskMaker, if the original offer maker performs a partial final settlement, the existing checks [here](https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L356-L358) and [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L230-L232) will cause an revert when attempting to complete a full settlement, resulting in their collateral being locked and requiring a rescue from the admin. To note, although examples in the documentation implies settlement in a single click, it is not stated that partial settlements are not allowed, so I believe it is a valid user flow.

Support

FAQs

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