Lack of timestamp in setDirectPrice() and price freshness check in getTokenPrice() may cause a stale price to be used

[code423n4]

Lines of code

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

Vulnerability details

function setDirectPrice(address _token, uint256 _price) external onlyAdmin {
  emit DirectPriceUpdated(_token, assetPrices[_token], _price);
  assetPrices[_token] = _price;
}

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

 function getTokenPrice(address _tokenAddress) public view override returns (uint256) {
    address tokenAddress = _tokenAddress;
    if (_tokenAddress == address(0)) {
      tokenAddress = wrapped;
    }
    uint256 tokenPrice = assetPrices[tokenAddress];
    if (tokenPrice == 0) {
      tokenPrice = getPriceFromOracle(tokenAddress);
    }
    if (tokenPrice == 0) {
      tokenPrice = getPriceFromDex(tokenAddress);
    }
    if (tokenPrice == 0 && v1PriceOracle != address(0)) {
      tokenPrice = IPriceOracle(v1PriceOracle).getTokenPrice(tokenAddress);
    }
    return tokenPrice;
  }

setDirectPrice() can be called by the admin to set assetPrices directly, once set, it will become the primary source in getTokenPrice().

However, there is no timestamp assigned alongside with the price when setDirectPrice(), as a result, the price set by the admin can and tend to be stale.

Furthermore, without a timestamp in the calldata, when the network is congested, transactions sent a while ago with stale prices can be accepted as new/fresh prices.

Recommendation

tokenPrice should record not only the price but also the last updated time.

setDirectPrice should add a new parameter: _timestamp:

function setDirectPrice(address _token, uint256 _price, uint256 _timestamp) external onlyAdmin {
  require(_price > 0, "bad price");
  

  if (block.timestamp > _timestamp) {
    // reject stale price
    require(block.timestamp - _timestamp < validPeriod, "bad updatedAt");
  } else {
    // reject future timestamp (< 3s is allowed)
    require(_timestamp - block.timestamp < 3, "bad updatedAt");
    _timestamp = block.timestamp;
  }
  emit DirectPriceUpdated(_token, tokenPrice[_token].answer, _price);

  tokenPrice[_token].answer = _price;
  tokenPrice[_token].updatedAt = _timestamp;
}

getTokenPrice() should check for the freshness of directly set token price:

 function getTokenPrice(address _tokenAddress) public view override returns (uint256) {
    address tokenAddress = _tokenAddress;
    if (_tokenAddress == address(0)) {
      tokenAddress = wrapped;
    }
    uint256 tokenPrice = assetPrices[tokenAddress].answer;
    if (tokenPrice > 0 && ((block.timestamp - tokenPrice[_token].updatedAt) < validPeriod)) {
      return tokenPrice
    }
    // no valid direct price, try `getPriceFromOracle`
    tokenPrice = getPriceFromOracle(tokenAddress);
    if (tokenPrice == 0) {
      tokenPrice = getPriceFromDex(tokenAddress);
    }
    if (tokenPrice == 0 && v1PriceOracle != address(0)) {
      tokenPrice = IPriceOracle(v1PriceOracle).getTokenPrice(tokenAddress);
    }
    return tokenPrice;
  }

[LayneHaber]

Agree this is an issue, but disagree with the severity as there are no oracles used in the core flow that could allow for value to be removed from the protocol. Within the bridging flow, the only time oracles are used is within the SponsorVault, and they are only used to provide additive value to the user.

[LayneHaber]

connext/monorepo@59c1a73

[0xleastwood]

AFAIK, the only instance where an oracle is used in SponsorVault is in getRate(). I don't think the issue raised actually affects the behaviour of this function unless gasTokenOracle builds upon ConnextPriceOracle. Tagging in @LayneHaber for more context.

[0xleastwood]

Downgrading to QA because ConnextPriceOracle.sol is not currently used within the codebase but may be integrated again in the future.

[0xleastwood]

Merging with #203.