yAudit Timeless Bunni Oracle Review

The contracts of the Bunni Oracle Repo were reviewed over 8 days. The code review was performed by 2 auditors between July 3 and July 12, 2023. The review was limited to the latest commit at the start of the review. This was commit ca510d8ce96195d4eed732a449f04504ca6d0daf for the Bunni Oracle repo.

Scope

The scope of the review consisted of the contracts in the following directories:

src/BunniOracle.sol

After the findings were presented to the Timeless team, fixes were made and included in several PRs.

This review is a code review to identify potential vulnerabilities in the code. The reviewers did not investigate security practices or operational security and assumed that privileged accounts could be trusted. The reviewers did not evaluate the security of the code relative to a standard or specification. The review may not have identified all potential attack vectors or areas of vulnerability.

yAudit and the auditors make no warranties regarding the security of the code and do not warrant that the code is free from defects. yAudit and the auditors do not represent nor imply to third parties that the code has been audited nor that the code is free from defects. By deploying or using the code, Timeless and users of the contracts agree to use the code at their own risk.

Code Evaluation Matrix

Category	Mark	Description
Access Control	Good	The contract does not appear to have any explicit access control.
Mathematics	Good	The contract includes mathematical operations for computing token prices and liquidity values. The calculations seem to be appropriately implemented.
Complexity	Good	The complexity of the contract appears to be reasonable, with functions performing specific tasks and utilizing external libraries for certain calculations.
Libraries	Good	The contract imports various external libraries that are well-tested.
Decentralization	Good	The contract is working tastelessly.
Code stability	Good	Code was stable during the review.
Documentation	Low	The code is well documented.
Monitoring	Average	The contract does not have built-in monitoring capabilities. All the functions being view functions it's not changing state variables and do not need events.
Testing and verification	Low	We suggest adding a differential test by implementing the price calculation logic in a language like Python and fetching price from Coingecko. This will help in identifying precision issues.

Findings Explanation

Findings are broken down into sections by their respective impact:

Critical, High, Medium, Low impact
- These are findings that range from attacks that may cause loss of funds, impact control/ownership of the contracts, or cause any unintended consequences/actions that are outside the scope of the requirements
Gas savings
- Findings that can improve the gas efficiency of the contracts
Informational
- Findings including recommendations and best practices

Critical Findings

None.

High Findings

None.

Medium Findings

1. Medium - Atomic Bunni token price manipulation

It is feasible to manipulate the price of a Bunni token within a single transaction.

Technical Details

Through the process of depositing into the underlying uniswap v3 position of Bunni, one can manipulate the value of the Bunni token. As a result, the price of the Bunni token will experience an immediate increase.

Allowing the price of an asset to be manipulated is at risk when the token can be borrowed.

The token can be inflated to liquidate borrowers for profit.
The token inflation can be used to attack the lending protocol with an attack similar to the cream hack.

It's also possible for the price to be manipulated down. In the unlikely event of a single entity owning the total supply of the Bunni token, burning the tokens then redepositing into the hub will result in a price decrease.

Impact

Medium. Token price manipulation can potentially lead to attacks.

Recommendation

It is advised to exercise caution while integrating the Bunni token and ensure that it is not made available for borrowing.

Developer Response

Acknowledged.

2. Medium - Potential Uniswap v3 price oracle manipulation

A TWAP price manipulation is possible to exploit the uniswap v3 oracle.

Technical Details

The manipulation involves controlling a certain percentage of consecutive blocks within the desired TWAP timeframe. With block producers being centralized to a small number of entities, we think it's possible to manipulate the Uniswap price oracle. An assessment of TWAP Market Risk can be found here.

BunniOracle.sol#L529-L544

Impact

Medium. The longer the TWAP window, the harder the attack becomes. However, it comes with a trade-off of higher deviation from the actual price.

Recommendation

To mitigate manipulation risks, it's important to choose TWAP timeframe based on a pool TVL. The larger the pool, the greater the funds required to manipulate it.

Developer Response

Acknowledged.

Low Findings

1. Low - Compatibility with Arbitrum

The team may deploy BunniOracle.sol on Arbitrum in future. The current code needs to be updated before deploying on Arbitrum because of differences between the chains.

Technical Details

Solidity version 0.8.20 switches default target EVM version to Shanghai. This introduces a new opcode PUSH0 which is currently not supported on Arbitrum.
If the Arbitrum sequencer is down, the L2 rollup can only be accessed through L1 optimistic rollup contracts. If a transaction is created on an L2 rollup while the sequencer is down, the transaction would be added to a queue. Because the transaction timestamp is the time when the transaction was queued, by the time the sequencer comes back online to process the queue, the price data could be outdated.

Impact

Low.

If solidity version is updated to 0.8.20 with Shanghai EVM version, PUSH0 may be introduced in the bytecode making it incompatible with Arbitrum.
If Arbitrum sequencer goes down, outdated prices may be fetched.

Recommendation

Select the appropriate solidity or EVM version before deploying to a chain other than mainnet.
Modify _getPriceUSDFromFeed() to use the Chainlink example code to check sequencer uptime. You may decide to revert or use Uniswap V3 oracle if the sequencer is down.

Developer Response

Fixed at commit 1daca0f17dc1350be8b40fcefd40347d612ce840.

yAudit Response

Reviewed. Circle has since launched native USDC on more chains including Arbitrum. This means that USDC has two versions (bridged and native). Please ensure you are using the intended address.

2. Low - Chainlink doesn't necessarily use 8 decimals

Technical Details

8 decimals is the most common precision used by Chainlink price oracle denominated in USD, but not all Chainlink oracle use 8 decimals. We fetched all the USD aggregators registered with the feed registry and found that only AMPL/USD oracle has 18 decimals. This is the aggregator returned from Chainlink registry:

getFeed(0xD46bA6D942050d489DBd938a2C909A5d5039A161, address(840))

BunniOracle assumes a fixed decimal precision of 8 decimals from Chainlink in BunniOracle.sol#L38:

uint256 internal constant PRICE_BASE = 1e8; // chainlink uses 8 decimals for prices

The contract doesn't return the correct values for feeds with a different precision. However, AMPL is a rebasing token so Bunni oracle will likely not be used with this oracle.

Impact

Low. The contract doesn't return correct values for feeds with a different precision.

Recommendation

If AMPL/USD oracle is not supposed to be used with this contract, future oracle integrations or deployment on other chains can still invalidate this assumption. Consider calling decimals() on the Chainlink aggregator to fetch the correct decimals value.

Developer Response

Fixed at 5c40aa17bad00ef694fc8d5578ce6cf3e1e2b5d4.

yAudit Response

Chainlink feed decimals are hardcoded for popular tokens. For Ethereum mainnet and Arbitrum, these values are correct, but make sure to verify the correctness before deploying on new chains. You can add such a check in the deploy script.

3. Low - `_quoteUSD()` doesn't behave as expected

Technical Details

Some assumptions of the if clauses in _quoteUSD() function are not correct (audit comments are left with @audit tag):

if (address(feed0) != address(0) && address(feed1) != address(0)) {
    // both tokens have chainlink price

    // fetch prices from chainlink
    price0USD = _getPriceUSDFromFeed(feed0, token0, chainlinkPriceMaxAgeSecs);
    price1USD = _getPriceUSDFromFeed(feed1, token1, chainlinkPriceMaxAgeSecs);
} else if (uint160(address(feed0)) | uint160(address(feed1)) == uint160(address(feed0))) {
    // feed0 != 0, feed1 == 0                        <-- @audit feed0 == 0, feed1 == 0 also enters here.
    // token0 has chainlink price

    // fetch prices from chainlink
    price0USD = _getPriceUSDFromFeed(feed0, token0, chainlinkPriceMaxAgeSecs);

    // compute price1USD using Uniswap v3 TWAP oracle
    (int24 arithmeticMeanTick,) = OracleLibrary.consult(address(pool), uniV3OracleSecondsAgo);
    uint256 quoteAmount =
        OracleLibrary.getQuoteAtTick(arithmeticMeanTick, token1Base.safeCastTo128(), token1, token0); // price of token1 in token0
    price1USD = quoteAmount.mulDivDown(price0USD, token0Base);
} else if (uint160(address(feed0)) | uint160(address(feed1)) == uint160(address(feed1))) {
    // feed0 == 0, feed1 != 0
    // token1 has chainlink price

    // fetch prices from chainlink
    price1USD = _getPriceUSDFromFeed(feed1, token1, chainlinkPriceMaxAgeSecs);

    // compute price0USD using Uniswap v3 TWAP oracle
    (int24 arithmeticMeanTick,) = OracleLibrary.consult(address(pool), uniV3OracleSecondsAgo);
    uint256 quoteAmount =
        OracleLibrary.getQuoteAtTick(arithmeticMeanTick, token0Base.safeCastTo128(), token0, token1); // price of token0 in token1
    price0USD = quoteAmount.mulDivDown(price1USD, token1Base);
} else {  // <-- @audit can never enter this else clause since all possibilities are exhausted before.
    // neither token has chainlink price
    // cannot compute USD price in this case
    revert BunniOracle__NoChainlinkPriceAvailable();
}

Impact

Low. The UX is not the same as intended. Currently, if address(0) is passed for feed0 and feed1 and if chainlink is set to a valid chainlink feed registry, _quoteUSD() will consider the chainlink price for feed0. From the comments, it seems like the intention is to revert for this case.

Recommendation

Update the conditions as:

if (address(feed0) != address(0) && address(feed1) != address(0)) {
    ...
-} else if (uint160(address(feed0)) | uint160(address(feed1)) == uint160(address(feed0))) {
+} else if (address(feed0) != address(0)) {
    // feed0 != 0, feed1 == 0
    ....
-} else if (uint160(address(feed0)) | uint160(address(feed1)) == uint160(address(feed1))) {
+} else if (address(feed1) != address(0)) {
    // feed0 == 0, feed1 != 0
    ....
} else {
    // neither token has chainlink price
    // cannot compute USD price in this case
    revert BunniOracle__NoChainlinkPriceAvailable();
}

Developer Response

Fixed at b4ca3512aaa03e54c41d49883415c006e00b3cab.

Gas Saving Findings

1. Gas - Test contracts imported

BunniOracle.sol imports forge's test contracts which can be removed.

Technical Details

BunniOracle.sol#L6:

import "forge-std/Test.sol";

Impact

Gas savings.

Recommendation

Remove the import.

Developer Response

Fixed at 7fb63848b5dc6473231b98abdc2aafe0a3211e91.

Informational Findings

1. Informational - Do not declare return variables without using it

It is recommended to rename the variable to an unnamed one as it is neither assigned nor returned by name. If the optimizer is not enabled, keeping the variable as it is will also result in unnecessary gas spending.

Technical Details

BunniOracle.sol#L111 BunniOracle.sol#L170 BunniOracle.sol#L225 BunniOracle.sol#L282 BunniOracle.sol#L313 BunniOracle.sol#L370 BunniOracle.sol#L422 BunniOracle.sol#L475 BunniOracle.sol#L578 BunniOracle.sol#L614

Impact

Informational.

Recommendation

Remove the unused variables.

Developer Response

Acknowledged, won't fix. We do use the optimizer so naming the return variables helps code readability.

2. Informational - `_quoteUSD()`'s `feed0` and `feed1` may not be used to fetch price

Technical Details

_quoteUSD() takes feed0 and feed1 as the chainlink feed for the two tokens. It's not immediately clear to the users that they may not be used to fetch Chainlink price if chainlink is set. Additionally, the caller may pass any non-zero value to make the _quoteUSD() if/else checks pass, since ultimately, the feed values are not used to fetch prices.

Impact

Informational.

Recommendation

Add a NatSpec comment documenting this behavior. In case it's not expected, update the code accordingly.

Developer Response

Fixed at 089fb1ff45846b190cb152a8dc26ba2c5461eeaa.

3. Informational - `else` block unnecessary

One way to simplify the code and remove a level of nesting is by removing the else block and directly returning the values from the if block.

Technical Details

BunniOracle.sol#L584-L592 BunniOracle.sol#L645-L646 BunniOracle.sol#L665-L667

Impact

Informational.

Recommendation

Remove the else part of the block.

Developer Response

Acknowledged, won't fix.

Final remarks

The code review has identified several areas for improvement and potential issues in the provided code. It is recommended to address the medium-level vulnerabilities related to token price manipulation and potential oracle manipulation. Additionally, the low-level issues regarding Chainlink precision, unexpected behavior in _quoteUSD(), and unnecessary code imports should be resolved. Lastly, the informational recommendations regarding unused variables and code simplification can be implemented for better code clarity. Overall, addressing these recommendations will help enhance the security, reliability, and efficiency of the code.

To identify potential precision issues, we've also sent the developers a basic Python script which can be run against foundry tests.

yAudit Timeless Bunni Oracle Review

Table of Contents

Review Summary

Scope

Code Evaluation Matrix

Findings Explanation

Critical Findings

High Findings

Medium Findings

1. Medium - Atomic Bunni token price manipulation

Technical Details

Impact

Recommendation

Developer Response

2. Medium - Potential Uniswap v3 price oracle manipulation

Technical Details

Impact

Recommendation

Developer Response

Low Findings

1. Low - Compatibility with Arbitrum

Technical Details

Impact

Recommendation

Developer Response

yAudit Response

2. Low - Chainlink doesn't necessarily use 8 decimals

Technical Details

Impact

Recommendation

Developer Response

yAudit Response

3. Low - _quoteUSD() doesn't behave as expected

Technical Details

Impact

Recommendation

Developer Response

Gas Saving Findings

1. Gas - Test contracts imported

Technical Details

Impact

Recommendation

Developer Response

Informational Findings

1. Informational - Do not declare return variables without using it

Technical Details

Impact

Recommendation

Developer Response

2. Informational - _quoteUSD()'s feed0 and feed1 may not be used to fetch price

Technical Details

Impact

Recommendation

Developer Response

3. Informational - else block unnecessary

Technical Details

Impact

Recommendation

Developer Response

Final remarks

3. Low - `_quoteUSD()` doesn't behave as expected

2. Informational - `_quoteUSD()`'s `feed0` and `feed1` may not be used to fetch price

3. Informational - `else` block unnecessary

← Back to Reports

Bunni Oracle

↑ Back to Top