Velodrome LP Share calculation wont work for all velodrome pools #24

hats-bug-reporter · 2023-06-19T23:11:43Z

Github username: @abhishekvispute
Submission hash (on-chain): 0x6c095881eb903abb853914dd88ca7962750a9620cae6633e1c70e0a869790d8b
Severity: high severity

Description:

Description

VMEX intends to allow Velo LP tokens as an asset for their lending protocol.
The value of Velo LP token is derived inside VelodromeOracle library, and does take reserve's spot manipulation into consideration.
The formula used by VMEX team is following.

Source: https://blog.alphaventuredao.io/fair-lp-token-pricing/

As expected this derivation of LP share price sustains any spot reserve's manipulation since the product of r0 * r1 (k) remains the same.
However, there is a case where this formula doesn't work.

Velodrome supports two types of pools, stable pools, and variable pools.
Stable pools are designed for stable pairs like USDC- DAI, and variable pools are for more volatile assets.
Both pools operate from same pair contract and have almost all logic same (mint, burn).
However differ in calculation of K, and swapped amount.
https://github.com/velodrome-finance/contracts/blob/de6b2a19b5174013112ad41f07cf98352bfe1f24/contracts/Pair.sol#L437
Variable pools, follow uniswap's xy = k curve, and stable pools follow solidly's curve x^3 * y + y^3 * x = k.
This difference is not ignorable like VMEX incorrectly assumes here

//some minor differences to univ2 pairs, but mostly the same

It's not "mostly same", please check following to see how the above formula fails for the stable pools.

Consider following as the initial stage of the Pool

Reserve 0	1000000
Reserve 1	1000000
Price 0	  1
Price 1	  1
Total Supply	1000
Calculated Price Of LP Share	2000

Source

Please check above spreadsheet for all derivations

Now consider that someone does a huge swap for velodrome's variable pool in hopes of manipulating the lp share price

Reserve 0	5000
Reserve 1	200000000
Price 0	  1
Price 1	  1
Total Supply	1000
Calculated Price Of LP Share	2000

Source

Here as you can see even though reserves are manipluated, our calculated LP share price didn't change.

Now let's consider the same swap for velodrome's stable pool

Reserve 0	5000
Reserve 1	7368062.997
Price 0	  1
Price 1	  1
Total Supply	1000
Calculated Price Of LP Share	383.8766207

Source

Here as you can see, there is drop in our calculated share price.
The reason for this drop is the K (r0* r1) parameter of the formula is not constrained to remain same with movement of reserves for stable pool, unlike variable pools.

Attack scenario:

Swap X -> Y
Liquidate all positions or Borrow LP shares
Swap Y -> X
Profit = Liquidation Fees - Swap Fees
Note that you dont need to swap such a huge amount to cause deviation, enough deviation could be achieved using smaller numbers too.

Reasoning Behind High Severity

Likelihood: There is no barrier of entry, anyone can execute it.
Impact: Irrecoverable loss Of funds

Recommendation

Consider fetching K from the velodrome pool itself rather than assuming it to be x*y always, then consider choosing different formula for stable pools.
This decision could be made on the basis of metadata since its returns 1 for the stable pool and 0 for the variable pool.

The text was updated successfully, but these errors were encountered:

mel0ndev · 2023-06-20T14:23:38Z

Hi, I'm from the VMEX team. Do you have a discord or tg where we can discuss further?

0xcuriousapple · 2023-06-20T14:27:35Z

yeah sure
curiousapple

for both discord and tg

0xcuriousapple · 2023-06-20T21:04:58Z

As requested by @mel0ndev, please find POC below.

Instructions

Copy the mock velodrome oracle contract inside packages/contracts/contracts/mocks/oracle/MockVelodromeOracle.sol
Copy the test inside packages/contracts/localhost_tests_OP/test-velo-stable-pools-poc.spec.ts
Fork Optimism vmex:node:fork:optimism
Deploy vmex:deploy:fork:optimism
Run vmex:OPfork:unit-tests

Overview

POC is on a forked instance of optimism mainnet.
It has two parts.
The first part is for variable pools, where it proves that even if a big swap occurs, there is no noticeable deviation in LP share price, as expected. (0 deviation in this case)
The second part is for stable pools, where it presents a significant deviation of 80% for the same swapped amount of the first part.

It is important to note that the cause of this issue is not reliance on spot reserves, the cause is the wrong consideration of the K value for stable pools.

Both tests mimic the data presented from the spreadsheet and scenario mentioned in the issue.

MockVelodromeOracle

// SPDX-License-Identifier: agpl-3.0
pragma solidity 0.8.19;

import {VelodromeOracle} from "../../protocol/oracles/VelodromeOracle.sol";
contract MockVelodromeOracle  {
  
    function getVeloPrice(
        address asset,
        uint256 [] memory prices
    ) external view returns (uint256 price) {  
        price = VelodromeOracle.get_lp_price(asset, prices);
        return price;
    }

}

test-velo-stable-pools-poc.spec.ts

import { ethers } from "ethers";
const chai = require("chai");
const { expect } = chai;
import { makeSuite } from "../test-suites/test-aave/helpers/make-suite";
import { DRE } from "../helpers/misc-utils";

import { BigNumber, utils } from "ethers";
import { eOptimismNetwork, IChainlinkInternal, ICommonConfiguration, ProtocolErrors } from '../helpers/types';
import {getCurvePrice} from "./helpers/curve-calculation";
import {UserAccountData} from "./interfaces/index";
import {almostEqualOrEqual} from "./helpers/almostEqual";
import {calculateExpectedInterest, calculateUserStake, calculateAdminInterest} from "./helpers/strategy-interest";

import OptimismConfig from "../markets/optimism";
import { getParamPerNetwork } from "../helpers/contracts-helpers";
import { getPairsTokenAggregator } from "../helpers/contracts-getters";
const oracleAbi = require("../artifacts/contracts/protocol/oracles/VMEXOracle.sol/VMEXOracle.json")

chai.use(function (chai: any, utils: any) {
  chai.Assertion.overwriteMethod(
    "almostEqualOrEqual",
    function (original: any) {
      return function (this: any, expected: UserAccountData) {
        const actual = <UserAccountData>this._obj;

        almostEqualOrEqual.apply(this, [expected, actual]);
      };
    }
  );
});


makeSuite(
    "Velodrome Stable Pools POC",
    () => {
    const { VL_COLLATERAL_CANNOT_COVER_NEW_BORROW } = ProtocolErrors;
    const fs = require('fs');
    const contractGetters = require('../helpers/contracts-getters.ts');

     const VeloAbi = [
        "function allowance(address owner, address spender) external view returns (uint256 remaining)",
        "function approve(address spender, uint256 value) external returns (bool success)",
        "function balanceOf(address owner) external view returns (uint256 balance)",
        "function decimals() external view returns (uint8 decimalPlaces)",
        "function name() external view returns (string memory tokenName)",
        "function symbol() external view returns (string memory tokenSymbol)",
        "function totalSupply() external view returns (uint256 totalTokensIssued)",
        "function transfer(address to, uint256 value) external returns (bool success)",
        "function transferFrom(address from, address to, uint256 value) external returns (bool success)",
        "function tokens() external returns (address, address)",
        "function metadata() external view returns (uint dec0, uint dec1, uint r0, uint r1, bool st, address t0, address t1)",
        "function mint(address to) external returns (uint liquidity)",
        "function sync() external",
        "function getReserves() external view returns (uint _reserve0, uint _reserve1, uint _blockTimestampLast)",
        "function swap(uint amount0Out, uint amount1Out, address to, bytes calldata data) external", 
        "function getAmountOut(uint amountIn, address tokenIn) external view returns (uint)"
    ];

    const VeloFactoryABI = [
        "function createPair(address tokenA, address tokenB, bool stable) external returns (address pair)",
        "function getPair(address tokenA, address token, bool stable) external view returns (address)"
    ];

    it("Variable Pool", async () => {

      /*//////////////////////////////////////////////////////////////
                                 SETUP
      //////////////////////////////////////////////////////////////*/

      var signer = await contractGetters.getFirstSigner();
      var veloFactoryAddress = "0x25cbddb98b35ab1ff77413456b31ec81a6b6b746"; 
      const velFactory = new DRE.ethers.Contract(veloFactoryAddress, VeloFactoryABI);

      const Erc20 = await DRE.ethers.getContractFactory("MintableERC20");
      const erc20_1 = await Erc20.deploy("token0", "token0", 18);
      const erc20_0 = await Erc20.deploy("token1", "token1", 18);

      const VelodromeOracle = await DRE.ethers.getContractFactory("MockVelodromeOracle");
      const velodromeOracle = await VelodromeOracle.deploy();

      await velFactory.connect(signer).createPair(erc20_0.address, erc20_1.address, false);
      const veloPairAddress = await velFactory.connect(signer).getPair(erc20_0.address, erc20_1.address, false); // variable pool

      const veloStablePool = new DRE.ethers.Contract(veloPairAddress, VeloAbi);

      
      /*//////////////////////////////////////////////////////////////
                                 INITIAL STAGE
      //////////////////////////////////////////////////////////////*/

      await erc20_0.connect(signer).mint(DRE.ethers.utils.parseUnits('1000000', 18));
      await erc20_1.connect(signer).mint(DRE.ethers.utils.parseUnits('1000000', 18));
      await erc20_0.connect(signer).transfer(veloPairAddress,  DRE.ethers.utils.parseUnits('1000', 18));
      await erc20_1.connect(signer).transfer(veloPairAddress, DRE.ethers.utils.parseUnits('1000', 18));
      await veloStablePool.connect(signer).mint(signer.address);
      console.log(await veloStablePool.connect(signer).totalSupply()); 
      await erc20_0.connect(signer).transfer(veloPairAddress,  DRE.ethers.utils.parseUnits('999000', 18));
      await erc20_1.connect(signer).transfer(veloPairAddress, DRE.ethers.utils.parseUnits('999000', 18));

      await veloStablePool.connect(signer).sync(); // this is not necessary step, just doing this to get state = example mentioned in the spreadsheet
      
      // Data from spreadsheet
      // Reserve 0	1000000
      // Reserve 1	1000000
      // Price 0	1
      // Price 1	1
      // Total Supply	1000
      // Calculated Price Of LP Share	2000

      // Exact Match
      
      /*//////////////////////////////////////////////////////////////
                                 HUGE SWAP
      //////////////////////////////////////////////////////////////*/
      
      expect(await veloStablePool.connect(signer).totalSupply(), DRE.ethers.utils.parseUnits('1000', 18)); 
      expect(await veloStablePool.connect(signer).getReserves()._reserve0, DRE.ethers.utils.parseUnits('1000000', 18)); 
      expect(await veloStablePool.connect(signer).getReserves()._reserve1, DRE.ethers.utils.parseUnits('1000000', 18)); 
      const prices = [DRE.ethers.utils.parseUnits('1', 18), DRE.ethers.utils.parseUnits('1', 18)];
      const oldLPPrice = await velodromeOracle.connect(signer).getVeloPrice(veloPairAddress, prices);;
      expect(oldLPPrice, DRE.ethers.utils.parseUnits('2000', 18));
     
      console.log("Initial LP Price", oldLPPrice);


      // Case 1 : Huge Swap In case of Variable Pool (X* Y = K)	
      // Reserve 0	5000
      // Reserve 1	~200000000 // Deviation occurs due to fees = 200400250
      // Price 0	1
      // Price 1	1
      // Total Supply	1000
      // Calculated Price Of LP Share	~2000 // Small deviation occurs due to fees (2002 to be exact)

      await erc20_1.connect(signer).mint(DRE.ethers.utils.parseUnits('199500000', 18)); 
      // 199000000 should be the swaped amount as per spreadsheet, however increaseing it to include fees
      // spreadsheet numbers dont consider any fees
      await erc20_1.connect(signer).transfer(veloPairAddress,  DRE.ethers.utils.parseUnits('199500000', 18)); // 199000000
      await veloStablePool.connect(signer).swap(DRE.ethers.utils.parseUnits('995000', 18), DRE.ethers.utils.parseUnits('0', 18), signer.address, "0x");
      const newLPPrice = await velodromeOracle.connect(signer).getVeloPrice(veloPairAddress, prices);;
      const reserves = await veloStablePool.connect(signer).getReserves();
      console.log("Reserve 0", reserves._reserve0);
      console.log("Reserve 1", reserves._reserve1);

      console.log("Final LP Price", newLPPrice);
      var PercentageChange = (newLPPrice.sub(oldLPPrice)).mul(100).div(oldLPPrice).toString();
      console.log("Percentage Change", PercentageChange);
      if(PercentageChange< 0) PercentageChange = PercentageChange * -1;
      expect(BigNumber.from(PercentageChange)).to.be.within(0, 2); // 0 deviation in this case
        
    });

    it("Stable Pool", async () => {
      /*//////////////////////////////////////////////////////////////
                                 SETUP
      //////////////////////////////////////////////////////////////*/

      var signer = await contractGetters.getFirstSigner();
      var veloFactoryAddress = "0x25cbddb98b35ab1ff77413456b31ec81a6b6b746"; 
      const velFactory = new DRE.ethers.Contract(veloFactoryAddress, VeloFactoryABI);

      const Erc20 = await DRE.ethers.getContractFactory("MintableERC20");
      const erc20_1 = await Erc20.deploy("token0", "token0", 18);
      const erc20_0 = await Erc20.deploy("token1", "token1", 18);

      const VelodromeOracle = await DRE.ethers.getContractFactory("MockVelodromeOracle");
      const velodromeOracle = await VelodromeOracle.deploy();

      await velFactory.connect(signer).createPair(erc20_0.address, erc20_1.address, true);
      const veloPairAddress = await velFactory.connect(signer).getPair(erc20_0.address, erc20_1.address, true);

      const veloStablePool = new DRE.ethers.Contract(veloPairAddress, VeloAbi);

      await erc20_0.connect(signer).mint(DRE.ethers.utils.parseUnits('1000000', 18));
      await erc20_1.connect(signer).mint(DRE.ethers.utils.parseUnits('1000000', 18));


         
      /*//////////////////////////////////////////////////////////////
                                 INITIAL STAGE
      //////////////////////////////////////////////////////////////*/

      await erc20_0.connect(signer).transfer(veloPairAddress,  DRE.ethers.utils.parseUnits('1000', 18));
      await erc20_1.connect(signer).transfer(veloPairAddress, DRE.ethers.utils.parseUnits('1000', 18));
      await veloStablePool.connect(signer).mint(signer.address);
      console.log(await veloStablePool.connect(signer).totalSupply()); 
      await erc20_0.connect(signer).transfer(veloPairAddress,  DRE.ethers.utils.parseUnits('999000', 18));
      await erc20_1.connect(signer).transfer(veloPairAddress, DRE.ethers.utils.parseUnits('999000', 18));

      await veloStablePool.connect(signer).sync(); // this is not necessary step, just doing this to get state = example mentioned in the spreadsheet

      expect(await veloStablePool.connect(signer).totalSupply(), DRE.ethers.utils.parseUnits('1000', 18)); 
      expect(await veloStablePool.connect(signer).getReserves()._reserve0, DRE.ethers.utils.parseUnits('1000000', 18)); 
      expect(await veloStablePool.connect(signer).getReserves()._reserve1, DRE.ethers.utils.parseUnits('1000000', 18)); 
      const prices = [DRE.ethers.utils.parseUnits('1', 18), DRE.ethers.utils.parseUnits('1', 18)];
      const oldLPPrice = await velodromeOracle.connect(signer).getVeloPrice(veloPairAddress, prices);;
      expect(oldLPPrice, DRE.ethers.utils.parseUnits('2000', 18));
      console.log("Initial LP Price", oldLPPrice);
      
      // Data from spreadsheet
      // Reserve 0	1000000
      // Reserve 1	1000000
      // Price 0	1
      // Price 1	1
      // Total Supply	1000
      // Calculated Price Of LP Share	2000

      // Exact Match
     
      /*//////////////////////////////////////////////////////////////
                                 HUGE SWAP
      //////////////////////////////////////////////////////////////*/

      // Data from spreadsheet
      // Reserve 0	5000
      // Reserve 1	~7368062.997 //  Deviation due to fees = 7374872.969
      // Price 0	1
      // Price 1	1
      // Total Supply	1000
      // Calculated Price Of LP Share	~383.8766207 //  Deviation due to fees = 384.053979773677127299
      

      await erc20_1.connect(signer).mint(DRE.ethers.utils.parseUnits('6378062', 18)); 
      // 6368062.997 should be the swaped amount as per spreadsheet, however increaseing it to include fees
      // spreadsheet numbers dont consider any fees

      await erc20_1.connect(signer).transfer(veloPairAddress,  DRE.ethers.utils.parseUnits('6378062', 18));
      await veloStablePool.connect(signer).swap(DRE.ethers.utils.parseUnits('995000', 18), DRE.ethers.utils.parseUnits('0', 18), signer.address, "0x");

      const newLPPrice = await velodromeOracle.connect(signer).getVeloPrice(veloPairAddress, prices);
      const reserves = await veloStablePool.connect(signer).getReserves();
      console.log("Reserve 0", reserves._reserve0);
      console.log("Reserve 1", reserves._reserve1);

      console.log("Final LP Price", newLPPrice);
      var PercentageChange = (newLPPrice.sub(oldLPPrice)).mul(100).div(oldLPPrice).toString();
      console.log("Percentage Change", PercentageChange);
      if(PercentageChange< 0) PercentageChange = PercentageChange * -1;
      expect(BigNumber.from(PercentageChange)).to.be.within(0, 2); // 80 deviation in this case
    });

    }
)

Output

Velodrome Stable Pools POC
BigNumber { value: "1000000000000000000000" }
Initial LP Price BigNumber { value: "2000000000000000000000" }
Reserve 0 BigNumber { value: "5000000000000000000000" }
Reserve 1 BigNumber { value: "200400250000000000000000000" }
Final LP Price BigNumber { value: "2002000249750234172033" }
Percentage Change 0
    ✓ Variable Pool (6074768 gas)
BigNumber { value: "1000000000000000000000" }
Initial LP Price BigNumber { value: "2000000000000000000000" }
Reserve 0 BigNumber { value: "5000000000000000000000" }
Reserve 1 BigNumber { value: "7374872969000000000000000" }
Final LP Price BigNumber { value: "384053979773677127299" }
Percentage Change -80

   1) Stable Pool
AssertionError: Expected "80" to be within [0,2]

0xcuriousapple · 2023-06-21T17:03:01Z

The case for this issue becomes more strong if you consider the initial assets VMEX team intended to allow.
As you can see there are many velo stable pool pairs there

https://docs.google.com/spreadsheets/d/1TBKpQuhRW1ikfFTrF93hCj6C-GpX2mCR7QrFygIugEk/edit#gid=646476569

hats-bug-reporter bot added the bug Something isn't working label Jun 19, 2023

ksyao2002 added the high label Jun 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Velodrome LP Share calculation wont work for all velodrome pools #24

Velodrome LP Share calculation wont work for all velodrome pools #24

hats-bug-reporter bot commented Jun 19, 2023

mel0ndev commented Jun 20, 2023

0xcuriousapple commented Jun 20, 2023 •

edited

0xcuriousapple commented Jun 20, 2023 •

edited

0xcuriousapple commented Jun 21, 2023 •

edited

Velodrome LP Share calculation wont work for all velodrome pools #24

Velodrome LP Share calculation wont work for all velodrome pools #24

Comments

hats-bug-reporter bot commented Jun 19, 2023

Description

Attack scenario:

Reasoning Behind High Severity

Recommendation

mel0ndev commented Jun 20, 2023

0xcuriousapple commented Jun 20, 2023 • edited

0xcuriousapple commented Jun 20, 2023 • edited

Instructions

Overview

MockVelodromeOracle

test-velo-stable-pools-poc.spec.ts

0xcuriousapple commented Jun 21, 2023 • edited

0xcuriousapple commented Jun 20, 2023 •

edited

0xcuriousapple commented Jun 20, 2023 •

edited

0xcuriousapple commented Jun 21, 2023 •

edited