Skip to main content
Dedaub

Vesper One Oracle

Smart Contract Security Assessment

December 14, 2022

Vesper

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit of several smart contract modules of Vesper One Oracle protocol.

This audit includes newly developed contracts and changes for One Oracle protocol under the at-the-time private repository bloqpriv/one-oracle, at commit hash a1464d91eaac5f58fe66c5c89b098f4029ff8da0.

Two auditors worked on the codebase on the following contracts:

contracts/
  • access/
    • Governable.sol
  • core/
    • AddressProvider.sol
    • ChainlinkBscPriceProvider.sol
    • ChainlinkFeedPriceProvider.sol
    • PriceProvider.sol
    • PriceProvidersAggregator.sol
    • StableCoinProvider.sol
    • UniswapV2LikePriceProvider.sol
    • UniswapV3PriceProvider.sol
  • features/
    • UsingStalePeriod.sol
  • periphery/
    • ChainlinkAndFallbacksOracle.sol
    • ChainlinkOracle.sol
    • MasterOracle.sol
    • VTokenFraxLendOracle.sol
    • tokens/
      • ATokenOracle.sol
      • AlusdTokenMainnetOracle.sol
      • BTCPeggedTokenOracle.sol
      • CTokenOracle.sol
      • CurveFactoryLpTokenOracle.sol
      • CurveLpTokenOracle.sol
      • EllipsisLpTokenOracle.sol
      • IbBtcTokenOracle.sol
      • MStableTokenOracle.sol
      • USDPeggedTokenOracle.sol
      • VPoolTokenOracle.sol
      • VspMainnetOracle.sol
      • YEarnTokenOracle.sol
  • swapper/
    • CurveExchange.sol
    • RoutedSwapper.sol
    • Swapper.sol
    • UniswapV2LikeExchange.sol

SETTING & CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. 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 affecting 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” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

[No high severity issues]


MEDIUM SEVERITY

[No medium severity issues]


LOW SEVERITY

L1

Unclear relationship between the governor and the proxy admin

L1LOW

Unclear relationship between the governor and the proxy admin
acknowledged

The governor stored in AddressProvider is of crucial importance since he controls the whole One Oracle protocol. The Governable abstract contract uses an AddressProvider at address 0xfbA0816A81bcAbBf3829bED28618177a2bf0e82A, which is a TransparentUpgradeableProxy. Currently both the governor and the proxy’s admin are set to the same address .

(0x76d266DFD3754f090488ae12F6Bd115cD7E77eBD)

The relationship between the two, however, is not enforced in the code. AddressProvider defines a specific procedure for updating the governor, by first proposing a new governor who has to explicitly accept it (acceptGovernorship). The new governor, however, will not necessarily be the admin of the TransparentUpgradeableProxy, and if the admin is not modified as well, the old governor still has full control over the protocol. Moreover, the “first-propose-then-accept” procedure is only followed to update the governor, not the proxy admin.

Due to its importance, we recommend having a clear procedure for updating the governor, and preferably enforcing that the same address is both the governor and the proxy admin.

L2

getBestAmountOut amount calculation imprecise for fee-on-transfer tokens

L2LOW

getBestAmountOut amount calculation imprecise for fee-on-transfer tokens
resolved

Swapper::swapExactInput has been modified to support fee-on-transfer tokens, by swapping the amount actually transferred (which might be less than amountIn). However, Swapper::swapExactInput still uses the original amountIn_ when calling getBestAmountOut to compute the best routing. It would be more precise to use the amount actually transferred also in this computation.

In most cases the small difference in the amount should make no difference in the resulting routing, but there could be edge cases when it does.

A theoretical example: imagine a 1-1 swap of TokenA to TokenB. User transfers 1.1 TokenA, but only 0.99 is transferred due to fee-on-transfer:

  • Exchange 1 gives 1-1 rate but takes a fixed fee of 0.1 TokenA.
  • Exchange 2 gives 1-1 rate but takes 10% fee.

Then:

  • When 1 .1TokenA is swapped, Exchange 1 gives higher output.
  • When 0.99 TokenA are swapped, Exchange 2 gives higher output.

(and it could be the case that only one of the two exchanges meets the _amountOutMin at the current moment).

L3

Small risk of losing funds in _swapExactInput

L3LOW

Small risk of losing funds in _swapExactInput
resolved

Swapper::_swapExactInput first transfers the incoming amount to the exchange, then computes the amount actually transferred and then calls the exchange’s swap function.

This procedure has a small risk of losing funds for the following reasons:

    • The exchange contracts (eg UniswapV2LikeExchange) should never hold balance in any token since they contain public functions that can trivially be used to extract any such balance. Hence, a transfer to the exchange must be immediately followed by a swap.
    • However, safeTransferFrom is an external function call to a third-party contract and such calls could potentially transfer the execution to some untrusted contract, for instance via an ERC777-type hook.

    Hence, the following attack scenario could theoretically take place:

      • ContractA asks for a swap by calling Swapper::swapExactInput
      • Swapper::_swapExactInput calls safeTransferFrom to transfer the funds to the exchange
      • A sender hook is called on ContractA because of the transfer, and that hook transfers execution to some untrusted ContractB.
      • ContractB swaps part of the funds by calling some swap* method of the exchange
      • Control returns to Swapper::_swapExactInput which computes the amount being transferred, and finds it to be smaller than what ContractA intended (since ContractB swapped part of the funds), leading to a loss of funds.

      Although the probability of such an attack is relatively small, it is useful to keep it in mind for future changes in the protocol. To avoid such scenarios, it might be useful to restrict access to the exchange’s methods, instead of having them public.



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

Extensive use of external calls

A1ADVISORY

Extensive use of external calls
info

The introduction of AddressProvider is very useful for organizing the codebase, but has the downside that all calls to governor providersAggregator and stableCoinProvider are external. External calls are orders of magnitude more expensive than internal ones, so in functions performing multiple such calls (eg ChainlinkAndFallbacksOracle::quoteUsdToToken) the overhead might be substantial.

If the gas overhead is considered important, optimizations could be employed to reduce it. For instance, one could create a wrapper function inside Governable, that fetches all information from addressProvider in a single call. Moreover, such a wrapper could cache this information locally, to avoid multiple calls in the same transaction.

Having a wrapper might a good practice for code readability anyway, instead of calling addressProvider.providersAggregator() in multiple places.

A2

Remove pragma abicoder v2 from CurveExchange contract

A2ADVISORY

Remove pragma abicoder v2 from CurveExchange contract
resolved

Since you are using solidity v0.8.9, the ABIEncoderV2 is enabled by default by the compiler, so you can remove this declaration from the CurveExchange contract of One Oracle protocol. Moreover, no other contracts contain this declaration.

A3

Inconsistent use of interfaces

A3ADVISORY

Inconsistent use of interfaces
resolved

MasterOracle declares the contents of the oracles mapping as IOracle, however the periphery oracles that will be used by MasterOracle implement ITokenOracle, which declares only getPriceInUsd (and not the other functions of IOracle). Indeed MasterOracle only calls getPriceInUsd, not the other functions. To improve code readability and avoid future issues with calling methods that are not actually implemented, we recommend declaring all variables with the proper interface used in the corresponding implementation.

A4

Compiler bugs

A4ADVISORY

Compiler bugs
info

The code is compiled with Solidity 0.8.9. Version 0.8.9, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.



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