Skip to main content
Dedaub

Chainlink AmpleforthKeeper

Smart Contract Security Assessment

18.02.2021

Chainlink

SUMMARY


ABSTRACT

Dedaub was commissioned to perform an audit of the AmplefortheKeeper and LastUpdated contracts at commit hash a6b9c174245d9401b234e346763d05f9868fd4c1.

The audit also examined any other functionality highly related to the project, i.e. the MedianOracle contract with which the audited contracts interact.


SETTING & CAVEATS

The audited codebase is of a small size at ~120LoC. The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

[No high severity issues]


MEDIUM SEVERITY

M1

Upkeep only goes through when new value is different than current

M1MEDIUM

Upkeep only goes through when new value is different than current
resolved

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.



LOW SEVERITY

L1

Unused return value

L1LOW

Unused return value
dismissed

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.



OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Floating pragma

A1ADVISORY

Floating pragma
dismissed

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.

A2

Compiler known issues

A2ADVISORY

Compiler known issues
info

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.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub