M-03 MitigationConfirmed

[c4-submissions]

Lines of code

Vulnerability details

Comments

The tick index returned by getActiveTickIndex may be a boundary value, and there is no verification of overflow when accessing adjacent indexes, which may lead to access errors. A notable impact scenario is:

1. The user completes the deposit while activeTickIndex is a median value
2. Market prices fluctuate and activeTickIndex becomes a boundary value
3. Accessing the adjacent index at this time will fail, resulting in withdraw DOS, because after withdrawal, it will be deposited again for rebalance.

Mitigation

    if (newTickIndex > 1) 
      depositAndStash(
        ticks[newTickIndex-2], 
        baseTokenIsToken0 ? 0 : availToken0 / liquidityPerTick,
        baseTokenIsToken0 ? availToken1 / liquidityPerTick : 0
      );
    if (newTickIndex > 0) 
      depositAndStash(
        ticks[newTickIndex-1], 
        baseTokenIsToken0 ? 0 : availToken0 / liquidityPerTick,
        baseTokenIsToken0 ? availToken1 / liquidityPerTick : 0
      );

  function depositAndStash(TokenisableRange t, uint amount0, uint amount1) internal returns (uint liquidity){
    if (amount0 == 0 && amount1 == 0) return 0;
    checkSetApprove(address(token0), address(t), amount0);
    checkSetApprove(address(token1), address(t), amount1);
    try t.deposit(amount0, amount1) returns (uint lpAmt){
      liquidity = lpAmt;
    }
    catch {
      return 0;
    }

    uint bal = t.balanceOf(address(this));
    if (bal > 0){
      checkSetApprove(address(t), address(lendingPool), bal);
      lendingPool.deposit(address(t), bal, address(this), 0);
    }
  }

1. Skip related deposit operations by detecting whether each index overflows.
2. Use try/catch to backward compatibility the failure of a single tick from affecting other ticks.
3. The activeIndex logic has been reconstructed, and the fluctuation range of ticks has been modified to two ticks up and down. Using parameterized liquidityPerTick to support dynamic adjustment of asset distribution.

By the way, the new implementation of getActiveTickIndex is wrong and has nothing to do with this question, submit it as a new question.

Conclusion

LGTM

[c4-judge]

gzeon-c4 marked the issue as unmitigated

[gzeon-c4]

Would consider this as unmitigated since getActiveTickIndex still may return wrong value https://github.com/code-423n4/2023-09-goodentry-mitigation-findings/issues/43

[c4-judge]

gzeon-c4 marked the issue as confirmed for report

[c4-judge]

gzeon-c4 marked the issue as satisfactory