nican0r

medium

Loss of rTokens owed if a user is owed more rTokens than the Position Manager's balance

Summary

If a user calls BasePositionManager::burnRTokens and they're owed more than the Position Manager's current balance they only receive up to the balance of the Pool and have their remaining balance of rTokenOwed wiped out.

Vulnerability Detail

In BasePositionManager::burnRTokens when the rTokenQty > rTokenBalance the following call to Pool::burnRTokens burns the entire balance of the BasePositionManager:

(amount0, amount1) = pool.burnRTokens(
      rTokenQty > rTokenBalance ? rTokenBalance : rTokenQty,
      false
);

this results in the LP that called the function only receiving rTokenBalance - rTokenQty when the burn is complete.

PoC:

Bob has rTokenQty = 100
The BasePositionManager only has rTokenBalance = 90 because the other 10 were earned from fees gained through rewards earned on the reinvested rTokens and so are only minted when _syncFeeGrowth is called
If bob calls BasePositionManager::burnRTokens , it sets pos.rTokenOwed = 0 and calls pool.burnRTokens(90)
Pool::burnRTokens then makes a call to _syncFeeGrowth which mints the 10 remaining rTokens but the function was already called with _qty = 90, so only those are burned for Bob and the remaining 10 stay in BasePositionManager
If Bob tries to call BasePositionManager::burnRTokens again he won't receive any reward because he's had rTokenOwed for his position reset to 0

Impact

LPs lose rTokenQty - rTokenBalance whenever they try to burn rTokenQty > rTokenBalance. They are unable to recover these lost rTokens and they remain stuck in BasePositionManager.

Code Snippet

BaseTokenManager::burnRTokens

    rTokenQty = pos.rTokenOwed;
    require(rTokenQty > 0, 'No rToken to burn');

    PoolInfo memory poolInfo = _poolInfoById[pos.poolId];
    IPool pool = _getPool(poolInfo.token0, poolInfo.token1, poolInfo.fee);

    pos.rTokenOwed = 0;
    uint256 rTokenBalance = IERC20(address(pool)).balanceOf(address(this));
    (amount0, amount1) = pool.burnRTokens(
      rTokenQty > rTokenBalance ? rTokenBalance : rTokenQty, // @audit when the rTokenQty is greater this only burns rTokenQty - rTokenBalance for the balance of BasePositionManager before reinvestment fees are minted
      false
    );

Pool::burnRTokens

  function burnRTokens(uint256 _qty, bool isLogicalBurn)
    external
    override
    lock
    returns (uint256 qty0, uint256 qty1)
  {
    if (isLogicalBurn) {
      _burn(msg.sender, _qty);

      emit BurnRTokens(msg.sender, _qty, 0, 0);
      return (0, 0);
    }
    // SLOADs for gas optimizations
    uint128 baseL = poolData.baseL;
    uint128 reinvestL = poolData.reinvestL;
    uint160 sqrtP = poolData.sqrtP;
    _syncFeeGrowth(baseL, reinvestL, poolData.feeGrowthGlobal, false); // @audit feeGrowth is synced here to account for change in reinvested liquidity

    // totalSupply() is the reinvestment token supply after syncing, but before burning
    uint256 deltaL = FullMath.mulDivFloor(_qty, reinvestL, totalSupply());
    reinvestL = reinvestL - deltaL.toUint128();
    poolData.reinvestL = reinvestL;
    poolData.reinvestLLast = reinvestL;
    // finally, calculate and send token quantities to user
    qty0 = QtyDeltaMath.getQty0FromBurnRTokens(sqrtP, deltaL);
    qty1 = QtyDeltaMath.getQty1FromBurnRTokens(sqrtP, deltaL);

    _burn(msg.sender, _qty); // @audit when _qty passed in is the BasePositionManager balance before syncing only that much is burned

Pool::_syncFeeGrowth

  /// @dev sync the value of feeGrowthGlobal and the value of each reinvestment token.
  /// @dev update reinvestLLast to latest value if necessary
  /// @return the lastest value of _feeGrowthGlobal
  function _syncFeeGrowth(
    uint128 baseL,
    uint128 reinvestL,
    uint256 _feeGrowthGlobal,
    bool updateReinvestLLast
  ) internal returns (uint256) {
    uint256 rMintQty = ReinvestmentMath.calcrMintQty(
      uint256(reinvestL),
      uint256(poolData.reinvestLLast),
      baseL,
      totalSupply()
    );
    if (rMintQty != 0) {
      rMintQty = _deductGovermentFee(rMintQty);
      _mint(address(this), rMintQty);
      // baseL != 0 because baseL = 0 => rMintQty = 0
      unchecked {
        _feeGrowthGlobal += FullMath.mulDivFloor(rMintQty, C.TWO_POW_96, baseL);
      }
      poolData.feeGrowthGlobal = _feeGrowthGlobal;
    }
    // update poolData.reinvestLLast if required
    if (updateReinvestLLast) poolData.reinvestLLast = reinvestL;
    return _feeGrowthGlobal;
  }

Tool used

Manual Review

Recommendation

Add the following conditional statement to ensure pos.rTokenOwed isn't set to 0 in the case where rTokenQty > rTokenBalance:

 if(rTokenQty > rTokenBalance) {
      pos.rTokenOwed = rTokenBalance - rTokenQty;
    } else {
      pos.rTokenOwed = 0;
    }

this will allow the LP to call BasePositionManager::burnRTokens again to burn their remaining rTokens once the fees for reinvested liquidity growth are synced.

Sponsor Disputed

The PoC and the finding are invalid.
Auditor’s PoC is wrong at step 4 such that Pool contract does not mint RToken to PositionManager when function Pool::burnRTokens() is called. At step 5, if Bob calls to BasePositionManager::syncFeeGrowth() , he still receives 10 rToken and still manages to call BasePositionManager::burnRTokens() to burn to get fees.
For the reason of burning all rTokens in PositionManager but updating rTokens owed by the position to 0: there will be a few less wei minted to the Position Manager than expected rTokens owed, due to rounding down in Pool logic, but the value is small enough so that we just consider as the full amount has been burnt instead of the actual burnt rTokens amount.
Reference from Uniswap v3

Escalate

The part that the rTokens don't get minted to the PositionManager is correct, they are in fact minted to the Pool because the call to _syncFeeGrowth passes the to value in _mint as `address(this):

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L637

however the mint itself will still occur in the given scenario because the calculation for rMintQty will be > 0:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L634C7

The additional 10 rTokens would then be in the Pool contract which as stated the user would try to redeem by first calling BasePositionManager::syncFeeGrowth which makes a call to Pool::tweakPosZeroLiq:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L228-L231

this calls _syncFeeGrowth again, however because the initial call had the updateReinvestLLast boolean set to false the tokens are again minted to the Pool, increasing the supply of rTokens in the Pool once again with rTokens that have already been accounted for:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L276

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L599

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L643

for the user to be able to redeem their rTokens the BasePositionManager::syncFeeGrowth then tries to update the value of pos.rTokenOwed in the call to _updateRTokenOwedAndFeeGrowth:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L233-L238

however, given that the feeGrowth hasn't changed the logic that increments pos.rTokenOwed is skipped:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

and therefore the original issue still holds as the pos.rTokenOwed = 0 from the initial call to BasePositionManager::burnRTokens

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

which triggers the require statement on L255 to revert:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L255

Escalate

The part that the rTokens don't get minted to the PositionManager is correct, they are in fact minted to the Pool because the call to _syncFeeGrowth passes the to value in _mint as `address(this):

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L637

however the mint itself will still occur in the given scenario because the calculation for rMintQty will be > 0:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L634C7

The additional 10 rTokens would then be in the Pool contract which as stated the user would try to redeem by first calling BasePositionManager::syncFeeGrowth which makes a call to Pool::tweakPosZeroLiq:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L228-L231

this calls _syncFeeGrowth again, however because the initial call had the updateReinvestLLast boolean set to false the tokens are again minted to the Pool, increasing the supply of rTokens in the Pool once again with rTokens that have already been accounted for:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L276

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L599

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L643

for the user to be able to redeem their rTokens the BasePositionManager::syncFeeGrowth then tries to update the value of pos.rTokenOwed in the call to _updateRTokenOwedAndFeeGrowth:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L233-L238

however, given that the feeGrowth hasn't changed the logic that increments pos.rTokenOwed is skipped:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

and therefore the original issue still holds as the pos.rTokenOwed = 0 from the initial call to BasePositionManager::burnRTokens

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

which triggers the require statement on L255 to revert:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L255

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

Escalate

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L637

however the mint itself will still occur in the given scenario because the calculation for rMintQty will be > 0:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L634C7

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L228-L231

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L276

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L599

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L643

for the user to be able to redeem their rTokens the BasePositionManager::syncFeeGrowth then tries to update the value of pos.rTokenOwed in the call to _updateRTokenOwedAndFeeGrowth:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L233-L238

however, given that the feeGrowth hasn't changed the logic that increments pos.rTokenOwed is skipped:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

and therefore the original issue still holds as the pos.rTokenOwed = 0 from the initial call to BasePositionManager::burnRTokens

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

which triggers the require statement on L255 to revert:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L255

Escalate

The part that the rTokens don't get minted to the PositionManager is correct, they are in fact minted to the Pool because the call to _syncFeeGrowth passes the to value in _mint as `address(this):

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L637

however the mint itself will still occur in the given scenario because the calculation for rMintQty will be > 0:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L634C7

The additional 10 rTokens would then be in the Pool contract which as stated the user would try to redeem by first calling BasePositionManager::syncFeeGrowth which makes a call to Pool::tweakPosZeroLiq:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L228-L231

this calls _syncFeeGrowth again, however because the initial call had the updateReinvestLLast boolean set to false the tokens are again minted to the Pool, increasing the supply of rTokens in the Pool once again with rTokens that have already been accounted for:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L276

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L599

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L629-L643

for the user to be able to redeem their rTokens the BasePositionManager::syncFeeGrowth then tries to update the value of pos.rTokenOwed in the call to _updateRTokenOwedAndFeeGrowth:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L233-L238

however, given that the feeGrowth hasn't changed the logic that increments pos.rTokenOwed is skipped:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

and therefore the original issue still holds as the pos.rTokenOwed = 0 from the initial call to BasePositionManager::burnRTokens

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L333-L350

which triggers the require statement on L255 to revert:

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/periphery/BasePositionManager.sol#L255

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

This escalation is invalid.

this calls _syncFeeGrowth again, however because the initial call had the updateReinvestLLast boolean set to false the tokens are again minted to the Pool, increasing the supply of rTokens in the Pool once again with rTokens that have already been accounted for:

=> This is invalid

In the burnRTokens function in Pool contract: Even though the boolean is set to false (i.e not update reinvestLLast in the _syncFeeGrowth function), it is updated in the burnRTokens right after calling _syncFeeGrowth (line 279 -> 282): https://github.com/sherlock-audit/2023-07-kyber-swap/blob/e0cb622e7dacf4e8603507f3ea1c1073f9445dbe/ks-elastic-sc/contracts/Pool.sol#L282

Reason: In the burnRTokens case, the reinvestL will be updated as burning, thus, it waits for reinvestL to be updated before updating reinvestLLast. In other cases, it can update reinvestLLast = reinvestL in the _syncFeeGrowth function as reinvestL is unchanged after that, so boolean is set to be true

So in general, the reinvestLLast is always updated to the latest reinvestL after syncing the growth fee. As the reinvestL and reinvestLLast are always updated correctly, the _syncFeeGrowth works as expected.

This causes other steps in the reporter's comment to be invalid.

sherlock-audit / 2023-07-kyber-swap-judging

nican0r - Loss of rTokens owed if a user is owed more rTokens than the Position Manager's balance #70

Loss of rTokens owed if a user is owed more rTokens than the Position Manager's balance

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation