Omniscia Bonq Audit

price-feed Manual Review Findings

price-feed Manual Review Findings

PRI-01M: Inexplicable Price Measurement Methodology

Description:

The price measurement utilized here appears incorrect as it will attempt to calculate the delta between the current period and the period located 14 slots before.

Example:

contracts/price-feed.sol
144/// @notice this will always return 0 before update has been called successfully for the first time.
145function price() external view override returns (uint256) {
146  uint256 startSnapshotIndex = shift + 1 == PRICE_AVERAGE_PERIOD ? 0 : shift + 1;
147  return
148    (price0CumulativeLast - snapshots[startSnapshotIndex].price0Cumulative) /
149    (blockTimestampLast - snapshots[startSnapshotIndex].blockTimestamp);
150}

Recommendation:

We advise this calculation method to be revised or explained via in-line documentation as it does not appear rational in its current state.

Alleviation:

The Bonq Protocol team has stated that they will not proceed with the price feed implementations that they currently have in place after the findings we have identified within the report. The price feeds they will instead utilize will be Chainlink and as such, we consider these exhibits nullified.

PRI-02M: Incorrect Price Measurements

Description:

The referenced price measurements factor into account the fees imposed by the Uniswap system whilst the price feed should not do so.

Impact:

Prices reported by the price-feed will be artificially lower than the actual price of the assets thus opening up a constant arbitrage opportunity for passive holders of both assets.

Example:

contracts/price-feed.sol
117function _getUpdatePrices(uint256 timeElapsed)
118  private
119  view
120  returns (
121    uint256 price0,
122    uint256 price1,
123    uint256 price0Cumulative,
124    uint256 price1Cumulative
125  )
126{
127  price0 = router.getAmountOut(DECIMAL_PRECISION, collateralToken, stableCoin);
128  price1 = router.getAmountOut(DECIMAL_PRECISION, stableCoin, collateralToken);
129

130  // ensure that at least one full period has passed since the last update
131  // require(timeElapsed >= UPDATE_PERIOD, "PriceFeed: update: update period not elapsed");
132

133  price0Cumulative = price0CumulativeLast + (price0 * timeElapsed);
134  price1Cumulative = price1CumulativeLast + (price1 * timeElapsed);
135}

Recommendation:

We advise the calculations to be performed directly, assuming a Uniswap-V2 like Router system the reserve values can be utilized directly in fractional format.

Alleviation:

The Bonq Protocol team has stated that they will not proceed with the price feed implementations that they currently have in place after the findings we have identified within the report. The price feeds they will instead utilize will be Chainlink and as such, we consider these exhibits nullified.

PRI-03M: Selective Storage of Price Feed Measurements

Description:

The update function allows the owner to selectively store price measurements from the Time-Weighted Price Average (TWAP) calculator, a trait that is highly undesirable for a decentralized system.

Impact:

As the snapshots are not always stored, it is possible for the owner of the contract to skew the system towards a price point they desire to favour any loans they may have procured themselves.

Example:

contracts/price-feed.sol
58function update(bool savePrevious) external override onlyOwner {
59  uint256 blockTimestamp = block.timestamp;
60  uint256 timeElapsed = blockTimestamp - blockTimestampLast;
61  bool isFirstUpdate = shift == 0 && snapshots[shift + 1].blockTimestamp == 0;
62

63  (uint256 price0, uint256 price1, uint256 price0Cumulative, uint256 price1Cumulative) = _getUpdatePrices(
64    timeElapsed
65  );
66

67  if (savePrevious && !isFirstUpdate) {
68    shift = shift + 1 == PRICE_AVERAGE_PERIOD ? 0 : shift + 1;
69

70    price0Average = price0AverageTemp;
71    price1Average = price1AverageTemp;
72

73    blockTimestampLast = blockTimestampLastTemp;
74    price0CumulativeLast = price0CumulativeLastTemp;
75    price1CumulativeLast = price1CumulativeLastTemp;
76

77    snapshots[shift].blockTimestamp = blockTimestampLastTemp;
78    snapshots[shift].price0Cumulative = price0CumulativeLastTemp;
79    snapshots[shift].price1Cumulative = price1CumulativeLastTemp;
80  }
81

82  price0AverageTemp = (price0Cumulative - price0CumulativeLast) / timeElapsed;
83  price1AverageTemp = (price1Cumulative - price1CumulativeLast) / timeElapsed;
84

85  blockTimestampLastTemp = blockTimestamp;
86  price0CumulativeLastTemp = price0Cumulative;
87  price1CumulativeLastTemp = price1Cumulative;
88

89  // filling price points with hipotetical values
90  if (isFirstUpdate) {
91    uint256 timeStep = 100;
92    uint256 emittedBlockTimestamp = blockTimestamp - (timeStep * (PRICE_AVERAGE_PERIOD));
93    for (uint256 i = 1; i < snapshots.length; i++) {
94      snapshots[i].blockTimestamp = emittedBlockTimestamp;
95      snapshots[i].price0Cumulative = i == 1
96        ? price0Cumulative
97        : snapshots[i - 1].price0Cumulative + (price0 * timeStep);
98      snapshots[i].price1Cumulative = i == 1
99        ? price1Cumulative
100        : snapshots[i - 1].price1Cumulative + (price1 * timeStep);
101      emittedBlockTimestamp += timeStep;
102    }
103    price0Cumulative = snapshots[PRICE_AVERAGE_PERIOD - 1].price0Cumulative + (price0 * timeStep);
104    price1Cumulative = snapshots[PRICE_AVERAGE_PERIOD - 1].price1Cumulative + (price1 * timeStep);
105

106    snapshots[shift].price0Cumulative = price0Cumulative;
107    snapshots[shift].price1Cumulative = price1Cumulative;
108    snapshots[shift].blockTimestamp = blockTimestamp - timeStep;
109    price0CumulativeLast = price0Cumulative;
110    price1CumulativeLast = price1Cumulative;
111    price0CumulativeLastTemp = price0Cumulative + (price0 * timeStep);
112    price1CumulativeLastTemp = price1Cumulative + (price1 * timeStep);
113    blockTimestampLast = blockTimestamp - timeStep;
114  }
115}

Recommendation:

We advise all periods to be properly stored as otherwise the system is completely susceptible to artificial price fluctuations.

Alleviation:

The Bonq Protocol team has stated that they will not proceed with the price feed implementations that they currently have in place after the findings we have identified within the report. The price feeds they will instead utilize will be Chainlink and as such, we consider these exhibits nullified.

PRI-04M: Incorrect Order of Snapshot Update

Description:

The snapshot update system is in incorrect order as the timeElapsed calculated at the beginning of the function is incorrect in case the snapshot is stored as the blockTimestamp - blockTimestampLast delta would be different after the snapshot storage logic block.

Impact:

Incorrect price measurements would yield a price that is weighted across a significantly bigger time period than expected.

Example:

contracts/price-feed.sol
58function update(bool savePrevious) external override onlyOwner {
59  uint256 blockTimestamp = block.timestamp;
60  uint256 timeElapsed = blockTimestamp - blockTimestampLast;
61  bool isFirstUpdate = shift == 0 && snapshots[shift + 1].blockTimestamp == 0;
62

63  (uint256 price0, uint256 price1, uint256 price0Cumulative, uint256 price1Cumulative) = _getUpdatePrices(
64    timeElapsed
65  );
66

67  if (savePrevious && !isFirstUpdate) {
68    shift = shift + 1 == PRICE_AVERAGE_PERIOD ? 0 : shift + 1;
69

70    price0Average = price0AverageTemp;
71    price1Average = price1AverageTemp;
72

73    blockTimestampLast = blockTimestampLastTemp;
74    price0CumulativeLast = price0CumulativeLastTemp;
75    price1CumulativeLast = price1CumulativeLastTemp;
76

77    snapshots[shift].blockTimestamp = blockTimestampLastTemp;
78    snapshots[shift].price0Cumulative = price0CumulativeLastTemp;
79    snapshots[shift].price1Cumulative = price1CumulativeLastTemp;
80  }

Recommendation:

We advise the snapshot logic to be relocated at the very top of the function to ensure the calculations within price the correct time period instead of using the old time measurement.

Alleviation:

The Bonq Protocol team has stated that they will not proceed with the price feed implementations that they currently have in place after the findings we have identified within the report. The price feeds they will instead utilize will be Chainlink and as such, we consider these exhibits nullified.

PRI-05M: Unsafe Initialization Methodology

Description:

The initialization methodology of the update function will fill in all 15 values of the snapshots array with hypothetical values which is incorrect, especially for a sensitive component such as a price feed.

Impact:

The initial price points reported by the price feed cannot be deemed as safe to use for any purpose.

Example:

contracts/price-feed.sol
89// filling price points with hipotetical values
90if (isFirstUpdate) {
91  uint256 timeStep = 100;
92  uint256 emittedBlockTimestamp = blockTimestamp - (timeStep * (PRICE_AVERAGE_PERIOD));
93  for (uint256 i = 1; i < snapshots.length; i++) {
94    snapshots[i].blockTimestamp = emittedBlockTimestamp;
95    snapshots[i].price0Cumulative = i == 1
96      ? price0Cumulative
97      : snapshots[i - 1].price0Cumulative + (price0 * timeStep);
98    snapshots[i].price1Cumulative = i == 1
99      ? price1Cumulative
100      : snapshots[i - 1].price1Cumulative + (price1 * timeStep);
101    emittedBlockTimestamp += timeStep;
102  }
103  price0Cumulative = snapshots[PRICE_AVERAGE_PERIOD - 1].price0Cumulative + (price0 * timeStep);
104  price1Cumulative = snapshots[PRICE_AVERAGE_PERIOD - 1].price1Cumulative + (price1 * timeStep);
105

106  snapshots[shift].price0Cumulative = price0Cumulative;
107  snapshots[shift].price1Cumulative = price1Cumulative;
108  snapshots[shift].blockTimestamp = blockTimestamp - timeStep;
109  price0CumulativeLast = price0Cumulative;
110  price1CumulativeLast = price1Cumulative;
111  price0CumulativeLastTemp = price0Cumulative + (price0 * timeStep);
112  price1CumulativeLastTemp = price1Cumulative + (price1 * timeStep);
113  blockTimestampLast = blockTimestamp - timeStep;
114}

Recommendation:

We advise all 15 measurements to be carried out properly before the price feed is enabled for use as otherwise it can be susceptible to incorrect price measurements.

Alleviation:

The Bonq Protocol team has stated that they will not proceed with the price feed implementations that they currently have in place after the findings we have identified within the report. The price feeds they will instead utilize will be Chainlink and as such, we consider these exhibits nullified.

PRI-06M: Unsafe Price Feed Implementation

Description:

The price feed implementation that the Bonq system utilizes is unsafe as TWAP mechanisms will be insecure following the Ethereum staking hard-fork as it will be trivial to affect the price of a pair multiple blocks in advance.

Impact:

Any TWAP implementation in post-staking-merge Ethereum will be ill-advised and susceptible to multi-block price manipulation attacks unless a significantly large TWAP period is selected in which case the TWAP would become unusable.

Example:

contracts/price-feed.sol
21struct CumulativePriceSnapshot {
22  uint256 blockTimestamp;
23  uint256 price0Cumulative;
24  uint256 price1Cumulative;
25}
26

27CumulativePriceSnapshot[PRICE_AVERAGE_PERIOD] public snapshots;
28// array index that always points to latest added snapshot
29uint256 public shift;
30

31uint256 public price0CumulativeLast;
32uint256 public price1CumulativeLast;
33uint256 public blockTimestampLast;
34uint256 public price0Average;
35uint256 public price1Average;
36

37uint256 public price0CumulativeLastTemp;
38uint256 public price1CumulativeLastTemp;
39uint256 public blockTimestampLastTemp;
40uint256 public price0AverageTemp;
41uint256 public price1AverageTemp;

Recommendation:

We advise the TWAP system to be re-evaluated in light of this and external price feeds to be used by the borrowing system instead such as Chainlink oracles.

Alleviation:

The Bonq Protocol team has stated that they will not proceed with the price feed implementations that they currently have in place after the findings we have identified within the report. The price feeds they will instead utilize will be Chainlink and as such, we consider these exhibits nullified.

Prev

mintable-token.sol (MIN-M)

Next

stability-pool.sol (STA-M)

On this page