Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

`LibUsdOracle` assumes every token from a Well is whitelisted with a oracle implementation set

holydevoti0n

Summary

The LibUsdOracle component in the protocol incorrectly assumes all tokens used within a Well that leverages the Generalized Oracle are registered/whitelisted. When whitelisting a token, we can see that the value of the oracleImplementationis set here:

// LibWhitelist -> whitelistToken
s.sys.oracleImplementation[token] = oracleImplementation;

Notice that the LibUsdOraclerelies on the variable above to fetch the prices from a generalized oracle.

// LibUsdOracle
function getTokenPriceFromExternal(
address token,
uint256 lookback
) internal view returns (uint256 tokenPrice) {
AppStorage storage s = LibAppStorage.diamondStorage();
@> Implementation memory oracleImpl = s.sys.oracleImplementation[token];
....
}

Vulnerability Details

If a Well has one of the tokens let's say WBTC, and this token is not whitelisted, LibUsdOracle returns a price of zero due to missing oracleImplementation. However, the protocol assumes that whenever it fetches a price for a token from the well that is not Bean, it will be able to fetch the correct price, when in fact it will return zero if this token is not whitelisted with a proper oracleImplementation set.

// LibWell
function getWellTwaUsdLiquidityFromReserves(
address well,
uint256[] memory twaReserves
) internal view returns (uint256 usdLiquidity) {
uint256 tokenUsd = getUsdTokenPriceForWell(well);
(address token, uint256 j) = getNonBeanTokenAndIndexFromWell(well);
if (tokenUsd > 1) {
return twaReserves[j].mul(1e18).div(tokenUsd);
}
​
// if tokenUsd == 0, then the beanstalk could not compute a valid eth price,
// and should return 0. if s.sys.twaReserves[C.BEAN_ETH_WELL].reserve1 is 0, the previous if block will return 0.
if (tokenUsd == 0) {
return 0;
}
​
// if the function reaches here, then this is called outside the sunrise function
// (i.e, seasonGetterFacet.getLiquidityToSupplyRatio()).We use LibUsdOracle
// to get the price. This should never be reached during sunrise and thus
// should not impact gas.
@> return LibUsdOracle.getTokenPrice(token).mul(twaReserves[j]).div(1e6);// @audit - LibUsdOracle issue
}
// SeasonGettersFacet
function getTwaLiquidityForWell(address well) public view returns (uint256) {
(address token, ) = LibWell.getNonBeanTokenAndIndexFromWell(well);
@> return LibWell.getTwaLiquidityFromPump(well, LibUsdOracle.getTokenPrice(token));//@audit LibUsdOracle issue
}

Another hint that enhances the assumption that the tokens from the well are automatically whitelisted as external tokens is on the RessedWhitelist. It whitelist the internal(Silo) tokens, but there is no use for the function whitelistTokenWithExternalImplementation for external tokens.

contract ReseedWhitelist {
AppStorage internal s;
​
/**
* @notice Whitelists Silo assets
*/
function init(address[] calldata tokens, AssetSettings[] calldata asset) external {
for (uint i; i < tokens.length; i++) {
@> LibWhitelist.whitelistToken( // @audit - missing external tokens from the wells.
tokens[i],
asset[i].selector,
asset[i].stalkIssuedPerBdv,
asset[i].stalkEarnedPerSeason,
asset[i].encodeType,
asset[i].gaugePointImplementation.selector,
asset[i].liquidityWeightImplementation.selector,
asset[i].gaugePoints,
asset[i].optimalPercentDepositedBdv,
asset[i].oracleImplementation
);
}
}
}

Additionally, a potential DoS vulnerability exists:

In scenarios where the oracleImplementation has only the selectorset but not the target, will lead the library to assign the oracleImpl target to address(this).

// LibUsdOracle -> getTokenPriceFromExternal
address target = oracleImpl.target;
@> if (target == address(0)) target = address(this);
​
(bool success, bytes memory data) = target.staticcall(
abi.encodeWithSelector(oracleImpl.selector, lookback)
);
​
if (!success) return 0;
assembly {
tokenPrice := mload(add(data, add(0x20, 0)))
}

The problem here is that if the current contract has a function(oracleImpl.selector) that relies on LibUsdOracle, it will lead the system to a DoS. Because all the paths for an external token price will lead to getTokenPriceFromExternal.

// LibusdOracle -> getTokenPriceFromExternal
(bool success, bytes memory data) = target.staticcall(
@> abi.encodeWithSelector(oracleImpl.selector, lookback) // @audit DoS - recursive call.
);
​
if (!success) return 0;

One example of a vulnerable facet that will lead to DoS in this scenario: OracleFacet. It consumes all the methods from LibUsdOraclewithout a reentrancy guard.

contract OracleFacet is Invariable, ReentrancyGuard {
function getUsdPrice(address token) external view returns (uint256) {
return LibUsdOracle.getUsdPrice(token, 0);
}
​
function getUsdPriceWithLookback(
address token,
uint256 lookback
) external view returns (uint256) {
return LibUsdOracle.getUsdPrice(token, lookback);
}
​
function getTokenPrice(address token) external view returns (uint256) {
return LibUsdOracle.getTokenPrice(token, 0);
}
​
function getTokenPrice(address token, uint256 lookback) external view returns (uint256) {
return LibUsdOracle.getTokenPrice(token, lookback);
}
​
function getTokenPriceFromExternal(
address token,
uint256 lookback
) external view returns (uint256 tokenPrice) {
return LibUsdOracle.getTokenPriceFromExternal(token, lookback);
}
}
​

Impact

  • The protocol cannot correctly determine the USD price of non-Bean tokens from a Well if they are not whitelisted, returning 0.

  • The protocol will DoS whenever the selector from the Generalised Oracle relies on LibUsdOracle.

Tools Used

Manual Review & Foundry

Recommendations

Fix for whitelisting external tokens:

  • Add a new function i.e: whitelistWellWithExternalTokens that combines both whitelistTokenand whitelistTokenWithExternalImplementation. Whenever adding a new well that needs to have its tokens whitelisted automatically, this function should be used to avoid adding a Well without support for fetching the price of the Well's token.

  • (Additional) On getTokenPriceFromExternal check whether oracleImplementationis properly set otherwise return 0.

Fix for the DoS issue:

  • (Recommended) Add the reentrancy modifier for the contracts that use LibUsdOracle. Notice that in OracleFacet for instance, the ReentrancyGuard is inherited but the modifier nonReentrant is not used.

  • (Alternative) Input sanitization can be done by ensuring the selectors of a specific facet, i.e: BDVFacetor a NatSpec could be added to the whitelist function to avoid having a selector that relies on LibUsdOraclefor external tokens.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.