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

Bid/buy offer owner receives full collateralToken along with pointsToken while settling askTaker using settleAskTaker()

0xaraj

Summary

Bid/buy offer owner receives full collateralToken along with pointsToken while settling askTaker using settleAskTaker() due to wrong calculation of collateralFee as it doesn't take account of settledPoint

Vulnerability Details

A user/seller can settle his askTaker using deliveryPlace:settleAskTaker(), passing the amount of _settledPoints which he wanted to settle. Now if _settledPoints are not equal to stockInfo.points then bider/buyer receives the makerRefund.

Now the problem is, when calculating makerRefund it doesn't take account of _settledPoints, which means if seller settles 0 point or even 1 point less than stockInfo.points then buyer will receives the full collateralToken back along with pointsToken which seller settled.

function settleAskTaker(address _stock, uint256 _settledPoints) external {
...
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, marketPlaceInfo.tokenAddress, settledPointTokenAmount
);
}
​
@> uint256 collateralFee = OfferLibraries.getDepositAmount(
offerInfo.offerType, offerInfo.collateralRate, stockInfo.amount, false, Math.Rounding.Floor
);
​
if (_settledPoints == stockInfo.points) {
tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash, _msgSender(), makerInfo.tokenAddress, collateralFee
);
@> } else {
tokenManager.addTokenBalance(
TokenBalanceType.MakerRefund, offerInfo.authority, makerInfo.tokenAddress, collateralFee
);
}
}

Suppose a user bid offer for 1000 points at 1000e18 collateralToken. A seller created ask taker for 1000 points. Now seller failed to settle askTaker for 1000 points, instead he settles it for 500 points. Now buyer should get 500e18 collateralToken back along with 500 pointsToken. But buyer receives full 1000e18 collateralToken along with 500 pointsToken.

//Here is PoC which shows the above situations

Note: settleAskTaker() has owner verification issue as it checks msg.sender with offerInfo.authority instead of stockInfo.authority, fix it. Also while adding pointsToken to buyer address, it uses collateralToken address instead of pointsToken address, fix it also. I've submitted both issue separately.

function test_buyerReceivesFullCollateralToken() public {
//User created a bid offer of 1000 points with 1000e18 amount
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 1000e18, 10000, 300, OfferType.Bid, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
//User2 created ask/sell offer for all 1000 points
vm.prank(user2);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1000);
​
vm.prank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 1e18, block.timestamp - 1, 3600);
​
//User2 settled the askTaker with 500 points only instead of 1000 points
vm.startPrank(user2);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
address stock1Addr = GenerateAddress.generateStockAddress(1);
deliveryPlace.settleAskTaker(stock1Addr, 500);
vm.stopPrank();
​
//User should receive 500e18 collateralToken as refund but receives full refund of 1000e18 collateralToken
uint256 makerRefund =
tokenManager.userTokenBalanceMap(user, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(makerRefund, 1000e18);
//PointsToken of user is 500e18 pointsToken
uint256 pointsToken =
tokenManager.userTokenBalanceMap(user, address(mockPointToken), TokenBalanceType.PointToken);
assertEq(pointsToken, 500e18);
}

Impact

Buyer receives full collateralAmount if seller fails to settle full points

Tools Used

Manual Review

Recommendations

Consider taking _settledPoints into account while calculating makerRefund for buyer

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 about 1 year 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.