external-price-feed Manual Review Findings

EXT-01M: Authoritative Definition of Prices

Type	Severity	Location
Centralization Concern		external-price-feed.sol:L74

Description:

The setPrice function permits an "oracle" implementation to set the prices for the system, however, no such oracle is defined in the repository and this role is assumed to be held by an Externally Owned Account (EOA), in which case the system is entirely centralized.

Example:

Copy
74function setPrice(uint256 price) external override onlyOracle {

Recommendation:

We advise the price definition mechanism to be revised as currently the system serves no real purpose apart from being a centralized data entry and prices could instead be entirely defined in a centralized manner if desired so.

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.

EXT-02M: Inexplicable Price Measurement Methodology

Type	Severity	Location
Mathematical Operations		external-price-feed.sol:L125-L130

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:

Copy
122/**
123 * @dev returns price based on moving average
124 */
125function price() external view override returns (uint256) {
126  uint256 startSnapshotIndex = shift + 1 == PRICE_AVERAGE_PERIOD ? 0 : shift + 1;
127  return
128    (priceCumulativeLast - snapshots[startSnapshotIndex].priceCumulative) /
129    (blockTimestampLast - snapshots[startSnapshotIndex].blockTimestamp);
130}

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.

EXT-03M: Unsafe Initialization Methodology

Type	Severity	Location
Logical Fault		external-price-feed.sol:L96-L108

Description:

The initialization methodology of the setPrice 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:

Copy
96if (isFirstUpdate) {
97  uint256 timeStep = 100;
98  uint256 emittedBlockTimestamp = blockTimestamp - (timeStep * (PRICE_AVERAGE_PERIOD - 1));
99  for (uint256 i = 1; i < snapshots.length; i++) {
100    snapshots[i].blockTimestamp = emittedBlockTimestamp;
101    snapshots[i].priceCumulative = i == 1 ? priceCumulative : snapshots[i - 1].priceCumulative + (price * timeStep);
102    emittedBlockTimestamp += timeStep;
103  }
104  priceCumulative = snapshots[PRICE_AVERAGE_PERIOD - 1].priceCumulative + (price * timeStep);
105
106  snapshots[shift].priceCumulative = priceCumulative;
107  priceCumulativeLast = priceCumulative;
108}

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.

EXT-04M: Unsafe Price Feed Implementation

Type	Severity	Location
Language Specific		external-price-feed.sol:L21-L24, L30-L32, L34-L36

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:

Copy
21struct CumulativePriceSnapshot {
22  uint256 blockTimestamp;
23  uint256 priceCumulative;
24}
25
26CumulativePriceSnapshot[PRICE_AVERAGE_PERIOD] public snapshots;
27// array index that always points to latest added snapshot
28uint256 public shift;
29
30uint256 public priceCumulativeLast;
31uint256 public blockTimestampLast;
32uint256 public priceAverage;
33
34uint256 public priceCumulativeLastTemp;
35uint256 public blockTimestampLastTemp;
36uint256 public priceAverageTemp;

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.