code-423n4 / 2022-11-redactedcartel-findings

3 stars 2 forks source link

Too small `FEE_DENOMINATOR` causes high fees in `AutoPxGmx` and `AutoPxGlp` #276

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L20 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L22

Vulnerability details

Impact

With the fee denominator (10,000) and the default fee values, users of the pools will have to pay unreasonably high fees.

Proof of Concept

FEE_DENOMINATOR is a constant that defines the denominator that's used in fees calculation, it's set to 10000 in AutoPxGmx and AutoPxGlp (AutoPxGmx.sol#L22, AutoPxGlp.sol#L20). With this denominator, fees paid by users of the pools seem too high:

  1. Withdrawal fee is 300 / 10000 = 0.03 = 3% (AutoPxGmx.sol#L28, AutoPxGlp.sol#L24). If a whale withdraws \$1,000,000 worth of assets, they'll pay \$30,000 in withdrawal fees (AutoPxGmx.sol#L173-L217, AutoPxGlp.sol#L151-L195).
  2. Platform fee is 1000 / 10000 = 0.1 = 10% (AutoPxGmx.sol#L29, AutoPxGlp.sol#L25). Platform fee is paid from compounded rewards (AutoPxGmx.sol#L289-L292, AutoPxGlp.sol#L252-L255), which means that 10% of rewards will go to the team/DAO/treasury.
  3. Compound incentive is also 1000 / 10000 = 0.1 = 10% (AutoPxGmx.sol#L30, AutoPxGlp.sol#L26). This incentive is enabled optionally by a caller of the compound function (AutoPxGmx.sol#L246, AutoPxGlp.sol#L213). It's disabled in automated compounding (AutoPxGmx.sol#L321, AutoPxGmx.sol#L345, AutoPxGmx.sol#L227) but, when enabled, it sends 10% of platform fees, or 1% of compounded rewards, to the caller.

    Tools Used

    Manual review

    Recommended Mitigation Steps

    Consider using 1,000,000 (1e6) as the fee denominator in AutoPxGmx and AutoPxGlp. This is the value that's used by PirexGmx (PirexGmx.sol#L44).

Picodes commented 1 year ago

This is a design choice, not a security issue ?

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid