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

0 stars 0 forks source link

getActiveTickIndex implementation error #43

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/GoodEntry-io/ge/blob/c7c7de57902e11e66c8186d93c5bb511b53a45b8/contracts/GeVault.sol#L470

Vulnerability details

Impact

The implementation of getActiveTickIndex is wrong, and the searched ticks do not meet expectations, causing funds to be incorrectly allocated to edge ticks, and there is basically no staking income.

Proof of Concept

    // if base token is token0, ticks above only contain base token = token0 and ticks below only hold quote token = token1
    if (newTickIndex > 1) 
      depositAndStash(
        ticks[newTickIndex-2], 
        baseTokenIsToken0 ? 0 : availToken0 / liquidityPerTick,
        baseTokenIsToken0 ? availToken1 / liquidityPerTick : 0
      );

  /// @notice Return first valid tick
  function getActiveTickIndex() public view returns (uint activeTickIndex) {
    // loop on all ticks, if underlying is only base token then we are above, and tickIndex is 2 below
    for (uint tickIndex = 0; tickIndex < ticks.length; tickIndex++){
      (uint amt0, uint amt1) = ticks[tickIndex].getTokenAmountsExcludingFees(1e18);
      // found a tick that's above price (ie its only underlying is the base token)
      if( (baseTokenIsToken0 && amt0 == 0) || (!baseTokenIsToken0 && amt0 == 0) ) return tickIndex;
    }
    // all ticks are below price
    return ticks.length;
  }

According to code comments:

getActiveTickIndex checks amt0 twice in the code is wrong, which causes baseTokenIsToken0 && amt0 == 0 to be true when the tick is below the current price. That is, the searched tick is the first tick lower than the current price, not the first tick greater than the current price, which is the first tick in the list. This results in funds being staked to marginal ticks and unable to obtain staking income.

Tools Used

Manual review

Recommended Mitigation Steps

      // found a tick that's above price (ie its only underlying is the base token)
      if( (baseTokenIsToken0 && amt1 == 0) || (!baseTokenIsToken0 && amt0 == 0) ) return tickIndex;

Assessed type

Context

c4-judge commented 12 months ago

gzeon-c4 marked the issue as primary issue

c4-judge commented 12 months ago

gzeon-c4 changed the severity to 2 (Med Risk)

c4-judge commented 12 months ago

gzeon-c4 marked the issue as selected for report

c4-judge commented 12 months ago

gzeon-c4 marked the issue as satisfactory