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

Ask makers are not refunded on a partial settlement

dadekuma

Summary

When an ASK offer maker settles their offer with a partial amount, they permanently lose the entire collateral instead of getting a partial refund.

Vulnerability Details

  1. Alice creates an ASK offer for 1000 points and 2000 collateral

  2. Bob creates a BID order to buy 400 points, sending 800 as payment

  3. Charlie creates a BID order to buy 200 points, sending 400 as payment

  4. Alice settles 599 points on her offer (instead of 600)

  5. Bob gets 399.33333333... points (instead of 400)

  6. Charlie gets 199.6666666... points (instead of 200)

  7. Alice loses the entire collateral because the settlement was partial, even if the bidders got their partial points

  8. Alice now has: (1000 - 598.93) points and 0 collateral, and 1200 payment

  9. Alice should have (2000 - 2.14) collateral left instead of 0. She lost 99.9% of her collateral.

Poc, run forge test --via-ir --match-test test_h10_ask_offer_maker_lose_collateral_partial_settlement -vv

function test_h10_ask_offer_maker_lose_collateral_partial_settlement() public {
​
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address charlie = makeAddr("charlie");
​
// give funds to users
uint256 aliceInitialBalance = 100_000 * 1e18;
deal(address(mockUSDCToken), alice, aliceInitialBalance);
vm.prank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
uint256 bobInitialBalance = 100_000 * 1e18;
deal(address(mockUSDCToken), bob, bobInitialBalance);
​
deal(address(mockUSDCToken), address(capitalPool), bobInitialBalance);
​
vm.prank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
uint256 charlieInitialBalance = 100_000 * 1e18;
deal(address(mockUSDCToken), charlie, charlieInitialBalance);
vm.prank(charlie);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
// alice creates offer
vm.startPrank(alice);
​
uint256 collateralAmount = 2000 * 1e18;
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000, //points
collateralAmount, //amount
10_000, //collateralRate
0, //eachTradeTax
OfferType.Ask,
OfferSettleType.Protected
)
);
​
assertEq(mockUSDCToken.balanceOf(alice), aliceInitialBalance - collateralAmount, "alice collateral balance after offer");
​
address offer0Addr = GenerateAddress.generateOfferAddress(0);
​
// bob is taker
vm.startPrank(bob);
uint256 bobPoints = 400;
preMarktes.createTaker(offer0Addr, bobPoints);
address stock1Addr = GenerateAddress.generateStockAddress(1);
assertEq(mockUSDCToken.balanceOf(bob), bobInitialBalance - 800 * 1e18 - 4 * 1e18, "bob collateral balance after offer");
vm.stopPrank();
​
// charlie is taker
vm.startPrank(charlie);
uint256 charliePoints = 200;
preMarktes.createTaker(offer0Addr, charliePoints);
address stock2Addr = GenerateAddress.generateStockAddress(2);
assertEq(mockUSDCToken.balanceOf(charlie), charlieInitialBalance - 400 * 1e18 - 2 * 1e18, "charlie collateral balance after offer");
vm.stopPrank();
​
// move market status
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
1e18,
block.timestamp - 1,
3600
);
​
vm.startPrank(alice);
uint256 alicePoints = 10_000 * 1e18;
deal(address(mockPointToken), alice, alicePoints);
mockPointToken.approve(address(tokenManager), type(uint256).max);
deliveryPlace.settleAskMaker(offer0Addr, 599);
​
// note, it's not possible to settle with multiple steps
vm.expectRevert(IPerMarkets.InvalidOfferStatus.selector);
deliveryPlace.settleAskMaker(offer0Addr, 1);
vm.stopPrank();
​
vm.startPrank(bob);
deliveryPlace.closeBidTaker(stock1Addr);
vm.stopPrank();
​
vm.startPrank(charlie);
deliveryPlace.closeBidTaker(stock2Addr);
vm.stopPrank();
// NOTE -> 1 Point = 2 Collateral
uint256 tokenAmount;
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 0 * 1e18, "refund alice"); // alice lost her entire collateral. she should have a refund of ~1998 * 1e18
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 1200 * 1e18, "sales alice");
​
// bob
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 800 * 1e18, "cash bob");
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.PointToken);
assertApproxEqRel(tokenAmount, 400 * 1e18, 1e16, "token bob"); // 399333333333333333333
​
// charlie
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 400 * 1e18, "cash charlie");
​
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.PointToken);
assertApproxEqRel(tokenAmount, 200 * 1e18, 1e16, "token charlie"); // 199666666666666666666
​
// withdraw alice
vm.startPrank(alice);
capitalPool.approve(address(mockUSDCToken));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.PointToken);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.RemainingCash);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
vm.stopPrank();
​
// alice lost the entire collateral
assertEq(mockUSDCToken.balanceOf(alice), aliceInitialBalance + 800 * 1e18 + 400 * 1e18 - collateralAmount, "alice collateral balance");
} // forge test --via-ir --match-test test_h10_ask_offer_maker_lose_collateral_partial_settlement -vv

Impact

Impact: High (High loss of funds for the ask makers)
Likelihood: Medium (Ask makers must settle a partial order)

Risk: High

Tools Used

Manual review

Recommendations

In DeliveryPlace change settleAskMaker so that the refund logic is always executed:

- 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
);
- }

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L276-L299

When calculating the maker refund amount, instead of offerInfo.amount, you should refund a percentage based on how many points were settled.

The refund logic should look like this, but remember to apply a scaling factor to not lose precision: offerInfo.amount * _settledPoints / offerInfo.usedPoints

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months 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.