code-423n4 / 2023-09-goodentry-mitigation-findings

0 stars 0 forks source link

Compound charges are sent to GeVault, making them potentially vulnerable to theft #49

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/GoodEntry-io/ge/blob/c7c7de57902e11e66c8186d93c5bb511b53a45b8/contracts/TokenisableRange.sol#L186-L189

Vulnerability details

Impact

In the previous version, the compound fee would be saved in the TokenisableRange before being deposited into LP, and would be deposited into LP after reaching 1%. After reconstruction, the fees are sent directly to GeVault for distribution through getTVL. However, this part of the fee was not calculated in advance when calling GeVault.deposit, causing getTVL to be undervalued, and additional LP would be minted for the user. After rebalance, this part of the fee is sent to GeVault, and the user can immediately withdraw and steal the fee funds.

Proof of Concept

GeVault.deposit logic can be simplified to the following code:

  // 1. Get TVL, which does not include the fees to be claimed in TokenisableRange. TVL is undervalued.
  uint vaultValueX8 = getTVL();

  // 2. Calculate LP based on TVL, mint extra LP since TVL is undervalued
  liquidity = tSupply * valueX8 / vaultValueX8;

  // 3. rebalance will invoke withdraw to claim fees in TokenisableRange
  rebalance();

The attacker can then immediately call withdraw to withdraw money and steal part of the fee. Of course, geVault will charge fees when depositing and withdrawing, and the specific benefits will be reduced. For markets with low transaction frequency, the accumulated fees can be substantial enough to carry out an attack.

Tools Used

Manual review

Recommended Mitigation Steps

Do not calculate the value by getTVL, but call removeFromAllTicks first, and then count all token0 and token1 values ​​after collecting all fees.

Assessed type

Context

c4-judge commented 12 months ago

gzeon-c4 marked the issue as duplicate of #57

c4-judge commented 12 months ago

gzeon-c4 marked the issue as satisfactory