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

`mulDiv()` can round down to 0 in realistic cases, allowing for tax avoidance

charlescheerful

Summary 📌

The code uses unsafely the .mulDiv() function, this function ultimately does: number*numerator/denominator. There are realistic scenarios where: number*numerator < denominator and the division truncates to 0.

This can ultimately be used to avoid taxes with some tokens or avoid platform fees too. Referral codes can potentially get affected by this too.

Vulnerability Details 🔍 && Impact 📈

Trade tax is calculated like so:

uint256 tradeTax = depositAmount.mulDiv(makerInfo.eachTradeTax, Constants.EACH_TRADE_TAX_DECIMAL_SCALER);

Which in math is: depositAmount * makerInfo.eachTradeTax / Constants.EACH_TRADE_TAX_DECIMAL_SCALER

makerInfo.eachTradeTax <= Constants.EACH_TRADE_TAX_DECIMAL_SCALER. Assuming healthy competitive market taxes will go down to offer better trades so its safe to assume that most of the time makerInfo.eachTradeTax < Constants.EACH_TRADE_TAX_DECIMAL_SCALER. Thus the division makerInfo.eachTradeTax / Constants.EACH_TRADE_TAX_DECIMAL_SCALER in solidity will round to 0 if depositAmount multiplied by makerInfo.eachTradeTax is not strong enough to make it > Constants.EACH_TRADE_TAX_DECIMAL_SCALER and get the right calculation.

  • Numerical Example 👁ī¸: EACH_TRADE_TAX_DECIMAL_SCALER==10_000, see here. And lets say a competitive tax of 1% == makerInfo.eachTradeTax == 100. If depositAmount < (10000 / 100) it will round down to 0.

You might notice that for ERC20s with 18 decimals an amount of < 100 is ridiculous and gas cost would be higher than tax savings. But for tokens of <= 2 decimals this is a real problem. As these tokens if used, small purchases from modest clients won't pay taxes. If you payed lets say 3$ == depositAmount == 300, then 300*100/10000 == 3000/10000 == 3$. But in solidity 3000/10000 truncates to 0.

A 2 decimal real token example GUSD (Gemini dollar), see etherscan.

This problem will probably occure in other parts of the code that use .mulDiv() to calculate token amounts mulitiplied by some percentage ratio based on the scalars. Like in calculating platformFee here. Notice that the scaler here is 1_000_000 so tokens with more than 2 decimals can be problematic too. Like famous and super-used USDC with 6 decimals.

Proof Of Concept (PoC) 👨‍đŸ’ģđŸ’ģ

Paste this test in the PreMarkets.t.sol file, import import "forge-std/console.sol";, and run forge test --mt "test_skipTax" -vv. You should also ovrride the decimals() function in the mock contract used by tests, this one. Like so:

function decimals() public pure override returns (uint8) {
return 2;
}
See test code 👁ī¸
function test_skipTax() public {
console.log("-----------------");
console.log("user -> Alice: ", address(user));
console.log("user1 -> Bob: ", address(user1));
console.log("-----------------");
​
console.log("Alice makes an ask maker offer and bob takes it without paying the tax.");
console.log(
"Alice sets a competitive low tax, 1%. Notice that in a competitive enviroment taxes tend to be as low as possible."
);
console.log("The lower the tax the greater the amount you can buy skipping it.");
​
vm.prank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
500 * 1e2,
10_000,
100,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
console.log("Bob takes 1 pts for 0.5$, 1% tax is 5 cents = 5");
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 1);
​
console.log("See Alice balance on tokenManager: ");
uint256 alceBalance =
tokenManager.userTokenBalanceMap(address(user), address(mockUSDCToken), TokenBalanceType.TaxIncome);
console.log("Alice balance Tax Revenue: ", alceBalance);
console.log("Alice should have 5 cents as revenue but round down to 0.");
console.log("Only cost to avoid tax is gas cost, in layer 2 evm compatible where gas is very cheap < tax");
console.log("You can just multicall and avoid tax.");
}

Tools Used 🛠ī¸

  • Manual review.

Recommendations đŸŽ¯

There are multiple approaches to mitigate or even elimiate this issue:

  • Reducing the constant scaler to something smaller than 10_000 in Layer2s or blockchains where gas costs are low to make it harder for this skip to be profitable.

  • Another approach and the one I recommend the most would be, when an operation carries taxes, detecting if the multiplier amount * numerator <= denominator and if so revert saying, minimal amount for avoinding tax evasion is not reached.

ℹī¸ Note 📘 There is probably better ways of fixing this, I do not have enough time to get deeper into it. I encourage the devs to do so.

Updates

Lead Judging Commences

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

finding-PreMarkets-tradeTax-round-down-low-decimal

Valid medium, this will indeed cause a leakage (albeit requires relatively small amount of collateral transacted, and is most significant for lower decimal tokens (does not break ERC20 specifications), resulting in platFormFee rounding to zero and creater of offers not sending fees to capitalPool when `_depositTokenWhenCreateTaker` is invoked. For issues noting rounding directions, it will be low severity given the impact is not proven sufficiently with a PoC/numerical example and most rounding will not result in significant losses. I believe the most appropriate solution here is to increase scale of platFormFees scalar, but to make sure that overflows are considered for higher decimal tokens.

Appeal created

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

finding-PreMarkets-tradeTax-round-down-low-decimal

Valid medium, this will indeed cause a leakage (albeit requires relatively small amount of collateral transacted, and is most significant for lower decimal tokens (does not break ERC20 specifications), resulting in platFormFee rounding to zero and creater of offers not sending fees to capitalPool when `_depositTokenWhenCreateTaker` is invoked. For issues noting rounding directions, it will be low severity given the impact is not proven sufficiently with a PoC/numerical example and most rounding will not result in significant losses. I believe the most appropriate solution here is to increase scale of platFormFees scalar, but to make sure that overflows are considered for higher decimal tokens.

Support

FAQs

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

Give us feedback!