Chainlink AmpleforthKeeper

In AmpleforthKeeper, the pushReport method on the MedianOracle is only called when the new value differs from the current one:

function performUpkeep(bytes calldata) external override {
    (bool hasNewAnswer, int256 latestAnswer) = super.updateAnswer();
    // Dedaub: pushReport does not go through if there is no new answer
    require(hasNewAnswer, "Feed has not changed");
    require(latestAnswer >= 0, "Invalid feed answer");
    ampleforthOracle.pushReport(uint256(latestAnswer));
}

The MedianOracle works by calculating the median price of a sequence of reported values, within a time window. With this in mind, we’ve identified potential scenarios where the above update policy could be problematic, when the values returned from the Chainlink feed have been the same for some time:

• The median calculation could be affected, as the median operation is sensitive to duplicate values. For example, assuming the Chainlink feed reports the values (in chronological order) [1, 2, 3, 3, 3, 3], then only the first three performUpkeeps would go through (due to the check in the snippet), and as such, the reported value will be median([1, 2, 3]) = 2. Had it not been for this check, all reports would go through and the median calculation would instead be median([1, 2, 3, 3, 3, 3]) = 3.

• Assuming the Chainlink feed has been reporting the same value for a time period greater than the time window of the MedianOracle, price reporting will fail due to the following check in the MedianOracle:

  function computeMedian(uint256[] array, uint256 size)
      internal
      pure
      returns (uint256)
  {
      // Dedaub: Median calculation reverts on empty arrays
      require(size > 0 && array.length >= size);
      for (uint256 i = 1; i < size; i++) {
          for (uint256 j = i; j > 0 && array[j-1]  > array[j]; j--) {
              uint256 tmp = array[j];
              array[j] = array[j-1];
              array[j-1] = tmp;
          }
      }
      if (size % 2 == 1) {
          return array[size / 2];
      } else {
          return array[size / 2].add(array[size / 2 - 1]) / 2;
      }
  }
  

It is recommended that this logic be refactored, and take into account the timestamp returned from the Chainlink feed instead of the value itself, as a measure of avoiding the same datapoint being pushed.

In contract LastUpdater.sol function

function checkUpkeep(bytes calldata) external view virtual override returns (bool upkeepNeeded, bytes memory) {}

declares two return variables. The second one is an unnamed variable of type bytes memory, which is never assigned nor returned. This may be confusing when developing contracts which intend to interact with LastUpdater but could also be error-prone regarding the contract’s maintenance and future updates. We recommend omitting the second return variable.

Floating pragmas (^0.8, ^0.8.7) are used in the contracts, allowing them to be compiled with the (0.8 - 0.8.12, 0.8.7 - 0.8.12) versions of the Solidity compiler. Although the differences between these versions are small, floating pragmas should be avoided and the pragma should be fixed to the version that will be used for the contracts’ deployment.

Contract AmpleforthKeeper was compiled with the Solidity compiler v0.8.7 which, at the time of writing, has a known bug (SignedImmutables). We believe that it does not affect the code: no immutable signed integer variables are declared. Contract LastUpdater was compiled with Solidity compiler v0.8 which, at the time of writing, has some known bugs. We believe that these bugs do not affect the code.