Canto Dex Oracle contest - hickuphh3's results

Lines of code

https://github.com/code-423n4/2022-09-canto/blob/main/src/Swap/BaseV1-periphery.sol#L501-L508

Vulnerability details

Description

The prices of USDC and USDT, which (I assume) are the underlying tokens of cUSDC and cUSDT, have been hardcoded to parity. Such practices are highly discouraged because while the likelihood of either stablecoin de-pegging is low, it is not zero.

Because of the UST debacle, the price of USDT dropped to $0.95 before making a recovery.

Impact

Here is an example of how a lending protocol on Fantom was affected by such a depeg event because they hardcoded the value.

To quote philosopher George Santayana, “Those who cannot remember the past are condemned to repeat it.”

Recommended Mitigation Steps

Consider using a price feed by trusted and established oracle providers like Chainlink, Band Protocol or Flux. The USDC/NOTE or USDT/NOTE price feed may be used as well, but NOTE has its own volatility concerns.

Lines of code

https://github.com/code-423n4/2022-09-canto/blob/main/src/Swap/BaseV1-periphery.sol#L582

Vulnerability details

Details & Impact

uint token0TVL = assetReserves[i] * (prices[i] / decimals);

Because of the brackets, the division of prices[i] / decimals is executed before multiplication, causing token0TVL to potentially be zero.

Proof of Concept

Add the following test in oracle.test.ts. Note: getPriceLP() should have its visibility changed from internal to public as the test relies on it.

To summarise what the test is doing, a stablecoin of 24 decimals is deployed, whose address will be greater than the note address so that token0 = note. It will enter the following case:

if (pair.stable()) { // stable pairs will be priced in terms of Note
  if (token0 == note) { //token0 is the unit, token1 will be priced with respect to this asset initially
      decimals = 10 ** (erc20(token1).decimals()); // we must normalize the price of token1 to 18 decimals
      prices = pair.sample(token1, decimals, 8, 1);
      (unitReserves, assetReserves) = pair.sampleReserves(8, 1);

such that the prices’s denomination is smaller than the stablecoin’s decimals of 24.

To see the difference in test results, apply the recommended fix after running the test once. In essence, the LP’s price will double from 999500000001499 to 1999999998838589, which is expected since the LP token should be worth the combined value of both stablecoins.

it.only("will have 0 token0TVL", async () => {
  // NOTE: change getPriceLP() from internal to public so that function can be called
  let tokenFactory = await ethers.getContractFactory("ERC20", dep)
  let stablecoin = await tokenFactory.deploy("STABLE","STABLE",ethers.utils.parseUnits("100000", "24"), 24)
  await stablecoin.deployed()
  // we want note to be token0
  // redeploy till it is
  while (stablecoin.address < note.address) {
      stablecoin = await tokenFactory.deploy("STABLE","STABLE",ethers.utils.parseUnits("100000", "24"), 24)
      await stablecoin.deployed()
  }
  // give token approvals to router
  let noteIn = ethers.utils.parseUnits("10000", "18")
  let stableIn = ethers.utils.parseUnits("10000", "24")
  await (await note.approve(router.address, ethers.constants.MaxUint256)).wait()
  await (await stablecoin.approve(router.address, ethers.constants.MaxUint256)).wait()

  // borrow note
  await (await comptroller._supportMarket(cUsdc.address)).wait()
  // set collateral factors for cCanto 
  await (await comptroller._setCollateralFactor(cUsdc.address, ethers.utils.parseUnits("0.9", "18"))).wait()
  // borrow note against usdc 
  await (await comptroller.enterMarkets([cUsdc.address, cNote.address])).wait()
  await (await usdc.approve(cUsdc.address, ethers.utils.parseUnits("1000"))).wait()
  // supply usdc
  await (await cUsdc.mint(ethers.utils.parseUnits("100000000", "6"))).wait()
  // borrow note
  await (await cNote.borrow(ethers.utils.parseUnits("9000000", "18"))).wait()

  // add liquidity
  await (await router.addLiquidity(
      note.address,
      stablecoin.address,
      true,
      noteIn,
      stableIn,
      0,
      0,
      dep.address,
      9999999999,
      )).wait()
  // get pair address
  let pairAddr = await factory.getPair(note.address, stablecoin.address, true)
  pair = await ethers.getContractAt("BaseV1Pair", pairAddr)

  //set period size to zero for instant observations
  await (await factory.setPeriodSize(0)).wait()

  // swap 10 times for price observations
  for(var i = 0; i < 10; i++) {
      if (i % 2) {
          //swap 0.01 note for stable
          await (await router.swapExactTokensForTokensSimple(
              ethers.utils.parseUnits("10", "18"),
              0,
              note.address,
              stablecoin.address,
              true,
              dep.address,
              9999999999999
          )).wait()
      } else {
          //swap stable for note
          await (await router.swapExactTokensForTokensSimple(
              ethers.utils.parseUnits("10", "24"),
              0,
              stablecoin.address,
              note.address,
              true,
              dep.address,
              9999999999999
          )).wait()
      }
  }
  // check lpToken price
  // Actual price calculated is 999500000001499
  // But expected price (after removing brackets) is 1999999998838589
  console.log((await router.getPriceLP(pairAddr)).toString());
});

Recommended Mitigation Steps

- uint token0TVL = assetReserves[i] * (prices[i] / decimals);
+ uint token0TVL = assetReserves[i] * prices[i] / decimals;

Low Severity Findings

L01: Misleading variable names token0TVL and token1TVL

Line References

https://github.com/code-423n4/2022-09-canto/blob/main/src/Swap/BaseV1-periphery.sol#L582-L583

Description

The variables token0TVL and token1TVL are misleading because assetReserves and unitReserves could be either token0 or token1 amounts, depending on whether token0 is the NOTE or CANTO token.

uint token0TVL = assetReserves[i] * (prices[i] / decimals);
uint token1TVL = unitReserves[i]; // price of the unit asset is always 1

Recommended Mitigation Steps

Consider better names indicating that they are referring to the TVL of the asset and unit tokens. Something like assetTokenTVL and unitTokenTVL.

Non-Critical Findings

NC01: Geometric mean / More established oracle services should be used

Line References

https://github.com/code-423n4/2022-09-canto/blob/main/src/Swap/BaseV1-periphery.sol#L558-L586

Description

The LP token’s value is determined by a 30-minutes arithmetic mean TWAP (am-TWAP) of the past 8 observations.

Hayden from Uniswap tweeted that *“gmTWAP (geometric mean TWAP) is equally expensive to manipulate in both directions under geometric brownish motion, whereas amTWAP overweights upwards movements a huge amount relative to downward movements (making it far easier to manipulate if you know what you’re doing)”.

This is supported by a research paper on TWAP Oracle attacks. I reproduced a portion of the “results” section below.

We compare the three different TWAP manipulation attacks we have seen so far. We again use the example where we want to double the price of an asset which uses a 30-minute TWAP that uses a pair with $2,000,000 of total liquidity reserves. The cost of the MMEV single-block attack with 1.5% hash rate is $160,000. Based on equation 1, the cost of the singleblock attack is C1(135 · 1) = $9,750,000 and the cost of the multi-block attack is 135 · C1(1) = $16,200,000. All attacks ignore the rather small AMM trading fee of around $35,000.

It goes on to present the geometric mean as a solution to the oracle manipulation.

However, calculation of the geometric mean for the current 8 observations is non-trivial because the multiplication of the values will likely lead to overflow. A naiive method would be to square root each observation thrice, then multiply them together. This however might lead to potential rounding errors, with significant gas usage as well.

A more viable solution would be to push for established oracle companies like Chainlink, Band Protocol and Flux to deploy on Canto for usage.

NC02: Redundancies

Line References

https://github.com/code-423n4/2022-09-canto/blob/main/src/Swap/BaseV1-periphery.sol#L492

https://github.com/code-423n4/2022-09-canto/blob/main/src/Swap/BaseV1-periphery.sol#L558-L586

Description

• Redundant underlying assignment

if (compareStrings(symbol, "cCANTO")) {
  underlying = address(wcanto); // redundant
  return getPriceNote(address(wcanto), false);

The assignment of wcanto to the underlying variable is redundant since it is not used in this branch.

• Redundant else case

else {
  if (isStable[underlying]) {
    return getPriceNote(underlying, true); // value has already been scaled
  }

  return getPriceCanto(underlying) * getPriceNote(address(wcanto), false) / 1e18;
}

The else case here is redundant and thus can be dropped because the if case returns early.

NC03: Have isPriceOracle indicator

Line References

https://github.com/Canto-Network/clm/blob/main/src/PriceOracle.sol#L7-L8

Description

There is an isPriceOracle constant that is present in the PriceOracle contract. Consider having it included in the periphery contract as well, even though the Comptroller doesn’t check it.