code-423n4 / 2023-08-goodentry-findings

3 stars 2 forks source link

Possible to brick the contract due to open ended array growth #452

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L115-L132

Vulnerability details

Impact

GeVault.pushTick and GeVault.shiftTick functions both add an extra array item to ticks , there is nowhere in the code that reduces the ticks array.

With low market cap tokens the ticks array could grow to a large enough number such that any calls to GeVault.getActiveTickIndex results in an out of gas revert which would cause GeVault.deployAssets to revert. This function is called every block by the rebalance function renderiung the contract un-usable

Proof of Concept

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L115-L132


  function pushTick(address tr) public onlyOwner {
    TokenisableRange t = TokenisableRange(tr);
    (ERC20 t0,) = t.TOKEN0();
    (ERC20 t1,) = t.TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    if (ticks.length == 0) ticks.push(t);
    else {
      // Check that tick is properly ordered
      if (baseTokenIsToken0) 
        require( t.lowerTick() > ticks[ticks.length-1].upperTick(), "GEV: Push Tick Overlap");
      else 
        require( t.upperTick() < ticks[ticks.length-1].lowerTick(), "GEV: Push Tick Overlap");

      ticks.push(TokenisableRange(tr));
    }
    emit PushTick(tr);
  } 

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L137-L161

  function shiftTick(address tr) public onlyOwner {
    TokenisableRange t = TokenisableRange(tr);
    (ERC20 t0,) = t.TOKEN0();
    (ERC20 t1,) = t.TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    if (ticks.length == 0) ticks.push(t);
    else {
      // Check that tick is properly ordered
      if (!baseTokenIsToken0) 
        require( t.lowerTick() > ticks[0].upperTick(), "GEV: Shift Tick Overlap");
      else 
        require( t.upperTick() < ticks[0].lowerTick(), "GEV: Shift Tick Overlap");

      // extend array by pushing last elt
      ticks.push(ticks[ticks.length-1]); // @audit-issue possible DOS - ticks array is gonna get too big, does it get reduced anywhere else
      // shift each element
      if (ticks.length > 2){
        for (uint k = 0; k < ticks.length - 2; k++) 
          ticks[ticks.length - 2 - k] = ticks[ticks.length - 3 - k];
        }
      // add new tick in first place
      ticks[0] = t;
    }
    emit ShiftTick(tr);
  }

https://github.com/code-423n4/2023-08-goodentry/blob/4b785d455fff04629d8675f21ef1d1632749b252/contracts/GeVault.sol#L297-L307


  function getReserves() public view returns (uint amount0, uint amount1){
    for (uint k = 0; k < ticks.length; k++){
      TokenisableRange t = ticks[k];
      address aTick = lendingPool.getReserveData(address(t)).aTokenAddress;
      uint bal = ERC20(aTick).balanceOf(address(this));
      (uint amt0, uint amt1) = t.getTokenAmounts(bal);
      amount0 += amt0;
      amount1 += amt1;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Reduce the size of the array when possible, ie. when a tick does not contain any liquidity.

Assessed type

DoS

141345 commented 1 year ago

array not likely to grow huge, so gas DoS is not a big issue:

        for (uint k = 0; k < ticks.length - 2; k++) 
          ticks[ticks.length - 2 - k] = ticks[ticks.length - 3 - k];
        }

QA might be more appropriate.

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

c4-pre-sort commented 1 year ago

141345 marked the issue as low quality report

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Overinflated severity