Vesper One Oracle

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.

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).

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.

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.

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.

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.

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.