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

Wrong accounting logic for refunds when a ASK offer is settled

dadekuma

Summary

Makers who settled an ASK offer have incorrect accounting, as they are credited with the wrong type of payment.

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. Alice settles 400 points on Bob's order, and she gets her collateral back

  4. Alice should have: 600 points, 2000 collateral, and 800 sales revenue; but instead, she has 600 points and 2800 collateral

  5. Alice's collateral is credited as sales revenue instead of a refund

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

POC, run forge test --via-ir --match-test test_m1_wrong_sales_accounting -vv:

function test_m1_wrong_sales_accounting() 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 = 400;
preMarktes.createTaker(offer0Addr, bobPoints);
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, 400);
vm.stopPrank();
​
// NOTE -> 1 Point = 2 Collateral
uint256 tokenAmount;
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.MakerRefund);
assertEq(tokenAmount, 0 * 1e18, "refund alice"); //should be 2000 collateral refund instead
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.RemainingCash);
assertEq(tokenAmount, 0 * 1e18, "cash alice");
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.PointToken);
assertEq(tokenAmount, 0 * 1e18, "token alice");
​
tokenAmount = tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
assertEq(tokenAmount, 2800 * 1e18, "sales alice"); //should be 800 sales instead
} // forge test --via-ir --match-test test_m1_wrong_sales_accounting -vv

Impact

Impact: Medium (Funds are indirectly at risk because the accounting logic used for withdrawals is wrong)
Likelihood: High (It will happen without preconditions everytime an ask offer is settled)

Risk: Medium

Tools Used

Manual Review

Recommendations

In DeliveryPlace ensure that the correct accounting label is used:

function settleAskMaker(address _offer, uint256 _settledPoints) external {
// rest of the code
​
uint256 makerRefundAmount;
if (_settledPoints == offerInfo.usedPoints) {
if (offerInfo.offerStatus == OfferStatus.Virgin) {
makerRefundAmount = 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
);
​
makerRefundAmount = OfferLibraries.getDepositAmount(
offerInfo.offerType,
offerInfo.collateralRate,
usedAmount,
true,
Math.Rounding.Floor
);
}
​
tokenManager.addTokenBalance(
- TokenBalanceType.SalesRevenue,
+ TokenBalanceType.MakerRefund,
_msgSender(),
makerInfo.tokenAddress,
makerRefundAmount
);
}
​
// rest of the code
}
Updates

Lead Judging Commences

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

finding-DeliveryPlace-settleAskMaker-addTokenBalance-wrong-TokenBalanceType

Valid low severity, while the token type inputted is wrong, userTokenBalanceMap is still incremented appropriately, so users can still withdraw their funds. So this would technically only affect accounting and public view functions.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.