mstpr-brainbot - Token with low decimals can have problems on fee and partial filling calculations

[sherlock-admin2]

mstpr-brainbot

medium

Token with low decimals can have problems on fee and partial filling calculations

Summary

Fee calculations and partial fill calculations can be off when working with tokens with low decimals like 0-1-2.

Vulnerability Detail

When calculating the fee to be deducted from an order, the following code is used:

uint256 feeAmount = fee.applyFee
                ? order.outputs[i].amount.mulDivUp(fee.fee, DENOM)
                : order.outputs[i].amount.mulDivUp(baseFee, DENOM);

Let's assume the output token is EURS, which has 2 decimals, and the output amount is 10 1e2 EURS (10 EURS). The baseFee is set to "10" as default. The calculation for the feeAmount will be: 10 1e2 * 10 / 100_000 = 1, due to mulDivUp rounding the "0" to "1". Hence, the fee will be calculated as 1 EURS. However, the baseFee was 10, and the DENOM was 100_000, so the fee to be taken normally would be 0.1. But due to rounding in Solidity, the fee taken will be 1 EURS, which is 10x more.

The protocol team can set the fee tier to the maximum, such as 1000 (1%), to address the issue. However, this would mean that the protocol team would be taking a 1% fee just to mitigate the rounding error.

The same issue can also occur in the partial filling of a token like EURS. This is how the portioned output amount is calculated given a partial fill: uint256 outPart = quantity.mulDivUp(output[0].amount, input.amount);

Assuming the quantity is 1e18, the total input amount is 1001 1e18, and the output amount is 100. The outPart will be calculated as: 1e18 100 / 1001 * 1e18 = 0, which rounds up to 1. This means if a filler fills this trade with this quantity, they will incur losses because outPart 1 is not the correct amount; it should have been 0.1. But due to Solidity rounding down, it is 10x higher. Fillers can choose not to fill this to avoid this loss, but that would mean that partial orders of such trades will not be possible.

Impact

As stated in the README the code should work with any token with any decimals. There are such liquid tokens exists with low decimals such as; EURS, TEL, GLD and integrating those tokens with GladiusReactor can not work on some cases described in the details section. Considering that, I'll label this as medium.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L63-L116

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/lib/PartialFillLib.sol#L85-L108

Tool used

Manual Review

Recommendation

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

0xAadi commented:

Invalid: OOS, custom implementation tokens are not supported

[daoio]

it's not intended to support low-decimals tokens

[cvetanovv]

I agree with the sponsor. The Readme does not mention to use these tokens: https://github.com/sherlock-audit/2024-02-rubicon-finance?tab=readme-ov-file#q-do-you-expect-to-use-any-of-the-following-tokens-with-non-standard-behaviour-with-the-smart-contracts

[mstpr]

Escalate

Tokens with low decimals are still standard ERC20's they just have lesser precision.

Also, read me says any ERC20 token can be used.

Those tokens can't be adapted to the adapter due to above reasons stated in the issue

[sherlock-admin2]

Escalate

Tokens with low decimals are still standard ERC20's they just have lesser precision.

Also, read me says any ERC20 token can be used.

Those tokens can't be adapted to the adapter due to above reasons stated in the issue

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ArnieSec]

@mstpr EURS is not a standard erc 20 token though, not only do they have fee on transfer, they also have a blacklist, and can be frozen.

[ArnieSec]

the ReadMe is up for interpretation if it accepts this issue because of this

Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?

Missing return values Tokens with Blocklists

Definition of standard

used or accepted as normal or average.

2 decimals is definitely not the norm or average, so according to readme, this issue is invalid. Also sponsor states in this issue that they do not plan to use erc 20 tokens with 2 decimals.

Based on readme and sponsor comments issue should remain invalid.

[ArnieSec]

@mstpr

Furthermore

From Sherlock docs

Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as weird-tokens (https://github.com/d-xo/weird-erc20) are not considered valid by default unless these tokens are explicitly mentioned in the README.

If we click the link and scroll down we see low decimal tokens as a category. Since the protocol didn't explicitly state that they would use low decimal token, according to Sherlock rules this issue is by default invalid.

Issue should remain invalid without a doubt.

[mstpr]

@ArnieGod I didn't know that the tokens with lower decimals are non standard ERC20's. Seeing your response, I agree, if it's considered as not standard then the issue is invalid and escalation can be rejected. Thanks for the info!

[cvetanovv]

This Issue should remain invalid. In order for this report to be valid, this token type must appear at the end of the readme under "non-standard behavior tokens". ArnieGod comments are completely accurate.

[Czar102]

Also, read me says any ERC20 token can be used.

"Any" token means that there is no finite list of tokens used, but restrictions in other parts of the README still hold.

As noted by @ArnieGod, we consider use the mentioned repo to give examples of weird ERC20 tokens, and low decimal tokens are explicitly mentioned.

Hence, planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: rejected