Skip to content
This repository has been archived by the owner on Dec 10, 2023. It is now read-only.

chaduke - There is a divide-before-multiply precision loss in MarketUtils.getFundingFeeAmount(). #193

Closed
sherlock-admin opened this issue Jun 5, 2023 · 3 comments
Labels
Non-Reward This issue will not receive a payout Sponsor Disputed The sponsor disputed this issue's validity

Comments

@sherlock-admin
Copy link
Contributor

chaduke

medium

There is a divide-before-multiply precision loss in MarketUtils.getFundingFeeAmount().

Summary

There is a divide-before-multiply precision loss in MarketUtils.getFundingFeeAmount().

Vulnerability Detail

MarketUtils.getFundingFeeAmount() is used to get the funding fee amount to be deducted or distributed.

[https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1340-L1374])https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1340-L1374)

However, there is a divide-before-multiply precision loss problem shown below:

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1353-L1355

which means 1.999Precision.FLOAT_PRECISION_SQRT will become 1.0 Precision.FLOAT_PRECISION_SQRT.

This might be a minor problem per se. However the internal accounting will have check such as balance < claimableFundingFeeAmount. When getFundingFeeAmount() is not calculated in favor of the system consistently - in our case, the payer might pay less than he is supposed to due to the precision loss as a result of divide-before-multiply. As a result, in the long run, the pool might have no sufficient funds to pay for the receivers, the health of the pool will be in compromise.

Impact

There is a divide-before-multiply precision loss in MarketUtils.getFundingFeeAmount().

Code Snippet

Tool used

VScode

Manual Review

Recommendation

Always multiple first and then divide.

@github-actions github-actions bot added the Medium A valid Medium severity issue label Jun 9, 2023
@xvi10 xvi10 added the Sponsor Disputed The sponsor disputed this issue's validity label Jun 12, 2023
@xvi10
Copy link

xvi10 commented Jun 12, 2023

would classify this as a low since the difference in values should be very small

@IllIllI000
Copy link
Collaborator

"The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team." https://docs.sherlock.xyz/audits/judging/judging . This one is marked as disputed and there is no fix, so closing

@IllIllI000
Copy link
Collaborator

feel free to escalate if you can show non-dust amounts using any of the tokens available in the current GMX: https://app.gmx.io/#/trade

@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue labels Jun 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Non-Reward This issue will not receive a payout Sponsor Disputed The sponsor disputed this issue's validity
Projects
None yet
Development

No branches or pull requests

3 participants