Skip to content

Critical Fee Loss in Withdrawal Process #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hats-bug-reporter bot opened this issue Sep 4, 2024 · 1 comment
Open

Critical Fee Loss in Withdrawal Process #50

hats-bug-reporter bot opened this issue Sep 4, 2024 · 1 comment
Labels
Invalid - lead auditor invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: --
Twitter username: --
Submission hash (on-chain): 0xa4193cb4bbb0f22a9776d3d01c0187fa13715255bdd87e4eab14659b5681ad70
Severity: high

Description:
Description
Summary The Minter contract's withdrawal mechanism, encompassing the previewWithdrawal and requestWithdrawal functions, suffers from two significant issues: precision loss in fee calculations and inconsistent event emission. These issues can lead to financial discrepancies, potential exploitation, and misleading transaction logs.

Attack Scenario
Precision Loss in Fee Calculation: The previewWithdrawal function uses integer division to calculate withdrawal fees, resulting in rounding down to zero for small amounts. This can lead to fee-free withdrawals for small amounts, creating an unfair advantage for users who can split large withdrawals into multiple smaller ones.

Impacts

Financial Implications:
The protocol may lose significant fee revenue due to users exploiting the precision loss by making multiple small withdrawals.
Over time, this could lead to a substantial deficit in expected fees, potentially impacting the protocol's financial stability.

Fairness and User Trust:
Users withdrawing larger amounts will effectively pay higher fee rates than those making smaller withdrawals, creating an unfair system.
This disparity could erode user trust in the protocol's fee structure and overall fairness.

Illustratiom

wihdrawalFee = 100 FEE_DENOMINATOR = 10000

This means the fee is 1% (100/10000 = 0.01 or 1%).

Let's calculate for different withdrawal amounts:

For a withdrawal of 1000 tokens: feeAmount = 1000 * 100 / 10000 = 10
netAmount = 1000 - 10 = 990

For a withdrawal of 100 tokens: feeAmount = 100 * 100 / 10000 = 1
netAmount = 100 - 1 = 99

For a withdrawal of 10 tokens: feeAmount = 10 * 100 / 10000 = 0.1 which rounds down to 0
netAmount = 10 - 0 = 10

For a withdrawal of 1 token: feeAmount = 1 * 100 / 10000 = 0.01 which rounds down to 0
netAmount = 1 - 0 = 1

As we can see, for small amounts (10 tokens or less in this case), the fee rounds down to 0 due to integer division. This means that small withdrawal effectively pay no fee, which could be exploited by users making multiple small withdrawal to avoid fees.

Attachments

  1. Proof of Concept (PoC) File
    function previewWithdrawal(uint256 amount) public view virtual returns (uint256) {
        uint256 feeAmount = amount*withdrawalFee/FEE_DENOMINATOR;
        uint256 netAmount = amount-feeAmount;
        return netAmount;
    }
  1. Revised Code File (Optional)
    function previewWithdrawal(uint256 amount) public view virtual returns (uint256) {
    uint256 feeAmount = (amount * withdrawalFee + FEE_DENOMINATOR - 1) / FEE_DENOMINATOR;
    uint256 netAmount = amount - feeAmount;
    return netAmount;
}

Use ceiling division in previewWithdrawal to ensure accurate fee calculation even for small amounts.

@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Sep 4, 2024
@0xRizwan
Copy link

0xRizwan commented Sep 5, 2024

Duplicate of #7 and #48

@ilzheev ilzheev added the invalid This doesn't seem right label Sep 5, 2024
@0xRizwan 0xRizwan added Invalid - lead auditor and removed bug Something isn't working labels Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Invalid - lead auditor invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants