`getPriceFromDex()` Using `balanceOf` to get price from dex can easily be manipulated

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L99-L115

Vulnerability details

function getPriceFromDex(address _tokenAddress) public view returns (uint256) {
    PriceInfo storage priceInfo = priceRecords[_tokenAddress];
    if (priceInfo.active) {
      uint256 rawTokenAmount = IERC20Extended(priceInfo.token).balanceOf(priceInfo.lpToken);
      uint256 tokenDecimalDelta = 18 - uint256(IERC20Extended(priceInfo.token).decimals());
      uint256 tokenAmount = rawTokenAmount.mul(10**tokenDecimalDelta);
      uint256 rawBaseTokenAmount = IERC20Extended(priceInfo.baseToken).balanceOf(priceInfo.lpToken);
      uint256 baseTokenDecimalDelta = 18 - uint256(IERC20Extended(priceInfo.baseToken).decimals());
      uint256 baseTokenAmount = rawBaseTokenAmount.mul(10**baseTokenDecimalDelta);
      uint256 baseTokenPrice = getTokenPrice(priceInfo.baseToken);
      uint256 tokenPrice = baseTokenPrice.mul(baseTokenAmount).div(tokenAmount);

      return tokenPrice;
    } else {
      return 0;
    }
  }

This is a very well known wrong implementation of price oracle based on Uniswap v2.

Both token0.balanceOf(pair) and token1.balanceOf(pair) can be easily manipulated almost for free, by sending tokens to the pair, call the oracle to update the price and then call pair.skim() to clawback the funds used for manipulation.

Recommendation

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

And https://github.com/Uniswap/v2-periphery/blob/master/contracts/examples/ExampleOracleSimple.sol

[ecmendenhall]

Duplicate of #13

[LayneHaber]

Agree this is an issue, however this is not used within the core protocol (no oracles in the core bridging flow), so I would disagree with the severity.

[jakekidd]

Duplicate of https://github.com/code-423n4/2022-06-connext-findings/issues/13

[0xleastwood]

This contract is not being actively used in the codebase. I'm downgrading this to QA because it may be integrated in the future.

[0xleastwood]

Merging with #203.