Beanstalk: The Finale

Summary

The protocol overall functionality and performance strongly relies on proper communication with the oracles it uses in order to obtain token information. It also depends on the correct handling of the data obtained. Mishandling of this data can seriously compromise the expected results of a specific internal process or a thread thereof.

Tools such as the ternary conditional operator are useful for handling simple value assignments given two possible cases of a condition. However, it is easy to make mistakes when using it because of the compactness of its syntax.

Vulnerability Details

In order to fetch the manipulation resistant USD price of different tokens, the protocol uses oracles that are implemented within Beanstalk, as well as external oracles. For the case of tokens that use the default Chainlink oracle implementation or a custom oracle implementation, Beanstalk uses the contracts/libraries/Oracle/LibUsdOracle.sol::getTokenPriceFromExternal function, where given the token address and a desired lookback, the token price is returned.

The function LibUsdOracle::getTokenPriceFromExternal in turn makes use of the contracts/libraries/Oracle/LibChainlinkOracle.sol::getTokenPrice function, which receives the desired token address, a timeout and the lookback to calculate the TWA or the instantaneous price of the token.

On the @dev field of the LibChainlinkOracle::getTokenPrice function it can be read:

@dev Returns the TOKEN/USD price with the option of using a TWA lookback. Use lookback = 0 for the instantaneous price. lookback > 0 for a TWAP.

However, in the ternary operation to get the Token/Usd price, the previous statement is not accomplished.

When lookback is greater than zero, the getPrice function is called without taking into account the desired lookback. Otherwise, getTwap function is called with a lookback equal to zero.

The ternary conditional operator used here makes the lookback value useless, and hence the desired time weighted average functionality is missing.

Furthermore, in the LibUsdOracle::getTokenPriceFromExternal function there is a code section where it is assumed that a dollar stablecoin is passed in. After obtaining the address of the first token in the pool (token0), and store it in the chainlinkToken variable, a ternary operator is used to compare it with the previously known token address:

For later use, both addresses are required to be different, however, it is evident that if the condition of the ternary operator is false, chainlinkToken variable will be assigned with the same value of the token variable, making both containing the same address after the execution of the line. In addition, both variables are passed as parameters for the LibUniswapOracle::getTwap function, resulting in wrong calculations.

Impact

Impact: High

Since the time weighted average price functionality is missing, and the usd stablecoins price is not properly obtained, calculations and processes that rely on it will not work as expected, compromising their accuracy and the stability of the protocol.

Likelihood: High

Span of the finding is as broad as the LibUsdOracle::getTokenPriceFromExternal function is used throughout the protocol. LibUsdOracle::getUsdPrice and LibUsdOracle::getTokenPrice functions uses it directly to work properly.

The list of contracts and functions that use LibUsdOracle::getUsdPrice is given below.

• FertilizerFacet::_getMintFertilizerOut

• OracleFacet::getUsdPriceWithLookback

• UsdOracle::getUsdTokenPrice

• UsdOracle::getUsdTokenTwap

• WellPrice::getConstantProductWell

• LibWell::getRatiosAndBeanIndex

The list of contracts and functions that use LibUsdOracle::getTokenPrice is given below.

• OracleFacet::getTokenPrice

• SeasonGettersFacet::getTwaLiquidityForWell

• UsdOracle::getTokenUsdPrice

• UsdOracle::getTokenUsdTwap

• LibFertilizer::beginBarnRaiseMigration

• LibWell::getWellTwaUsdLiquidityFromReserves

Every time the LibUsdOracle::getTokenPriceFromExternal function is called, it is very likely to obtain a wrong result because of the incorrect implementations of the ternary conditional operator, breaking the intended behavior of the function and the thread of functions that called it.

Tools Used

Manual Review

Recommendations

In the LibChainlinkOracle::getTokenPrice function, exchange the place of true and false cases will solve the issue:

Moreover, changing the condition of the ternary operator is also a valid solution:

For the case of usd stablecoins in LibUsdOracle::getTokenPriceFromExternal function, fixing the statement for when the condition is false will solve the issue.

In addition, instead of using a ternary operator, an if statement is also a valid solution: