ConnextPriceOracle.getPriceFromDex() is vulnerable to price manipulation

[code423n4]

Lines of code

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

Vulnerability details

Impact

Getting price data from an oracle can be tricky. There are a lot of pitfalls. One of them is to compute price data using the token balances of a DEX pair. It's very easy to manipulate the price since that approach relies on the price at a single moment. The attacker can take a flash loan, use it to prop up the price of an asset, and thus manipulate the price of the pair within the same transaction. Since they can do it within the same transaction there's virtually no risk for them

Instead of computing the price data through token balances, you should properly integrate the oracle of whatever DEX you want to use. In the case of Uniswap, you're supposed to use the observe() function. You get the price by observing the data over multiple blocks. That makes the price manipulation for the attacker way more difficult. They are exposed to other bots that will use the arbitrage opportunity which will put the price back to its original value.

Proof of Concept

ConnextPriceOracle computes the price through the token balances:

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

Tools Used

none

Recommended Mitigation Steps

Properly integrate the DEX oracle or remove it altogether and rely on Chainlink.

[itsmetechjay]

Warden submitted a help desk request asking that the following be added as a comment:

Just two hours ago the same issue was exploited on Inverse Finance, see https://twitter.com/peckshield/status/1537383690262589440

By computing the price of the asset using the actual balances of a pair, the attacker was able to manipulate it safely through a flash loan.

[jakekidd]

Duplicate of #13

[0xleastwood]

No instance of this function being used anywhere within the codebase. Downgrading to QA.

[0xleastwood]

Merging with #106.