removeWhitelistStatus function Ignores updating milestoneSeason variableChainlink oracle returns stale price due to CHAINLINK_TIMEOUT variable in LibChainlinkOracle being set to 4 hoursSubmitted by Zealynx, LordOfTerra. Selected submission by: Zealynx.
The defaultGaugePointFunction in the smart contract does not explicitly handle the scenario where the percentage of the Base Deposited Value (BDV) equals the optimal percentage (optimalPercentDepositedBdv), resulting in an unintended reduction of gauge points to 0 instead of maintaining their current value.
This behavior can lead to an undesired decrease in incentives for contract participants, potentially affecting participation and reward accumulation within the contract's ecosystem. Users may lose gauge points and, consequently, rewards due to a technical flaw rather than their actions.
The testnew_GaugePointAdjustment() test demonstrated this flaw by providing inputs where currentGaugePoints = 1189, optimalPercentDepositedBdv = 64, and percentOfDepositedBdv = 64, expecting newGaugePoints to equal currentGaugePoints. However, the outcome was newGaugePoints = 0, indicating an unexpected reduction to zero.
function testnew_GaugePointAdjustment() public {
uint256 currentGaugePoints = 1189;
uint256 optimalPercentDepositedBdv = 64;
uint256 percentOfDepositedBdv = 64;
uint256 newGaugePoints = gaugePointFacet.defaultGaugePointFunction(
currentGaugePoints,
optimalPercentDepositedBdv,
percentOfDepositedBdv
);
assertTrue(newGaugePoints <= MAX_GAUGE_POINTS, "New gauge points exceed the maximum allowed");
assertEq(newGaugePoints, currentGaugePoints, "Gauge points adjustment does not match expected outcome");
}
Implement Explicit Returns: Ensure the defaultGaugePointFunction has an explicit return for the case where gauge points should not be adjusted. This can be achieved by adding a final return statement that simply returns currentGaugePoints if neither condition for incrementing nor decrementing is met, as shown below:
else {
return currentGaugePoints;
}
Submitted by InAllHonesty.
According to the documentation there are certain conditions that need to be met for a token to be whitelisted:
Additional tokens may be added to the Deposit Whitelist via Beanstalk governance. In order for a token to be added to the Deposit Whitelist, Beanstalk requires:
1. The token address;
2. A function to calculate the Bean Denominated Value (BDV) of the token (see Section 14.2 of the whitepaper for complete formulas); and
3. The number of Stalk and Seeds per BDV received upon Deposit.
Thus if the community proposes any kind of Fee-on-Transfer or rebasing tokens like (PAXG or stETH) and the Beanstalk governance approves it, then the protocol needs to integrate them into the system. But as it is now the system is definitely not compatible with such tokens.
deposit, depositWithBDV, addDepositToAccount, removeDepositFromAccount and any other silo accounting related functions perform operations using inputed/recorded amounts. They don't query the existing balance of tokens before or after receiving/sending in order to properly account for tokens that shift balance when received (FoT) or shift balance over time (rebasing).
Likelyhood - low/medium - At the moment of writing lido has over 31% of the ETH staked which makes stETH a very popular token. There's a strong chance that stakeholder would want to have stETH inside the silo.
Impact - High - It simply won't work.
Overall severity is medium.
Manual review
Clearly state in the docs that weird tokens won't be implemented via Governance Vote or adjust the code to check the token.balanceOf() before and after doing any operation related to the silo.
removeWhitelistStatus function Ignores updating milestoneSeason variableSubmitted by 0xBeastBoy, holydevoti0n. Selected submission by: 0xBeastBoy.
The issue in the LibWhitelistedTokens:removeWhitelistStatus function is that it removes the Whitelist status of a token without considering the impact on other related variables, such as the milestoneSeason variable.
milestoneSeason Is used in many functions for checking whether a token is whitelisted or not i.e.
require(s.ss[token].milestoneSeason == 0, "Whitelist: Token already whitelisted");
If the milestoneSeason variable is not updated or cleared when removing the Whitelist status, it may lead to incorrect behavior in subsequent checks or operations that rely on this variable.
Removing the Whitelist status of a token without updating related variables can lead to inconsistencies in the data stored in the contract. The milestoneSeason variable, used for checking whitelist status in many functions, may still hold outdated or incorrect information after removing the status, potentially leading to unexpected behavior or vulnerabilities.
Manual Review
To address this issue, ensure that related variables, such as milestoneSeason, are appropriately updated or cleared when removing the Whitelist status of a token. If the milestoneSeason variable is no longer relevant after removing the Whitelist status, it should be updated or cleared to maintain data integrity.
Submitted by holydevoti0n.
When user calls gm and the call for the chainlink oracle fails, it will return 0 for the deltaB value and this will cause a cascade effect, making the calculation of caseId = 3 and using the incorrect caseId to set up the new temperature on Weather.sol
function updateTemperature(int8 bT, uint256 caseId) private {
uint256 t = s.w.t;
if (bT < 0) {
if (t <= uint256(-bT)) {
// if (change < 0 && t <= uint32(-change)),
// then 0 <= t <= type(int8).max because change is an int8.
// Thus, downcasting t to an int8 will not cause overflow.
bT = 1 - int8(t);
s.w.t = 1;
} else {
s.w.t = uint32(t - uint256(-bT));
}
} else {
s.w.t = uint32(t + uint256(bT));
}
emit TemperatureChange(s.season.current, caseId, bT);
}
Every consumer of the temperature on the protocol will be affected like:
LibDibbler.morningTemperatureLibDibbler.beansToPodsLibDibbler.remainingPodsSun.setSoilAbovePegSun.stepSunFieldFacet.maxTemperatureFieldFacet.totalSoilFieldFacet._totalSoilAndTemperaturegm function uses the incorrect deltaB(0) to calculate the caseId which is then used to set the temperature.
function gm(address account, LibTransfer.To mode) public payable returns (uint256) {
int256 deltaB = stepOracle(); // @audit here if oracle failed, we update the season.timestamp and return deltaB zero here
uint256 caseId = calcCaseIdandUpdate(deltaB); // @audit caseId will be 3 here if deltaB is zero
LibGerminate.endTotalGermination(season, LibWhitelistedTokens.getWhitelistedTokens());
LibGauge.stepGauge();
stepSun(deltaB, caseId); // @audit wrong deltaB and caseId used here, setting the abovePeg to false and soil to zero
}
The interest rate will be wrongly decreased to 1, compromising the protocol peg mechanism when it needs to be maintained with a high interest rate/ temperature.
Sow will be calculated with the lowest temperature, also compromising the peg mechanism due to the wrong exchange of Beans -> Sow -> Pods
Remaining pods function will return zero and users will have an inaccurate number representing their actual pods.
Prepare the environment to work with Foundry + Updated Mocks https://gist.github.com/h0lydev/fcdb00c797adfdf8e4816031e095fd6c
Make sure to have the mainnet forked through Anvil: anvil --fork-url https://rpc.ankr.com/eth
Create the SeasonTemperature.t.sol file under the folder foundry and paste the code below. Then run forge test --match-contract SeasonTemperatureTest -vv.
// SPDX-License-Identifier: MIT
pragma solidity =0.7.6;
pragma abicoder v2;
import { Sun } from "contracts/beanstalk/sun/SeasonFacet/Sun.sol";
import { MockSeasonFacet } from "contracts/mocks/mockFacets/MockSeasonFacet.sol";
import { MockSiloFacet } from "contracts/mocks/mockFacets/MockSiloFacet.sol";
import { MockFieldFacet } from "contracts/mocks/mockFacets/MockFieldFacet.sol";
import { MockWhitelistFacet } from "contracts/mocks/mockFacets/MockWhitelistFacet.sol";
import {LibWhitelist} from "contracts/libraries/Silo/LibWhitelist.sol";
import { Utils } from "./utils/Utils.sol";
import "./utils/TestHelper.sol";
import "contracts/libraries/LibSafeMath32.sol";
import "contracts/C.sol";
contract SeasonTemperatureTest is MockSeasonFacet, TestHelper {
using SafeMath for uint256;
using LibSafeMath32 for uint32;
bool oracleFailed;
function setUp() public {
console.log("diamondSetup");
vm.createSelectFork('local');
oracleFailed = false;
setupDiamond();
dewhitelistCurvePool();
mintUnripeLPToUser1();
mintUnripeBeanToUser1();
setOraclePrices(false, 1000e6, 1000e6, 1000e6);
_setReservesForWell(1000000e6, 1000e18);
// user / tokens
mintTokenForUsers();
setTokenApprovalForUsers();
enableFertilizerAndMintActiveFertilizers();
callSunriseForUser1();
}
//////////// Setup functions ////////////
function setTokenApprovalForUsers() internal {
approveTokensForUser(deployer);
approveTokensForUser(user1);
approveTokensForUser(user2);
approveTokensForUser(user3);
approveTokensForUser(user4);
approveTokensForUser(user5);
}
function mintTokenForUsers() internal {
mintWETHtoUser(deployer);
mintWETHtoUser(user1);
mintWETHtoUser(user2);
mintWETHtoUser(user3);
mintWETHtoUser(user4);
mintWETHtoUser(user5);
// mint C.bean() to users
C.bean().mint(deployer, 10e6);
C.bean().mint(user1, 10e6);
C.bean().mint(user2, 10e6);
C.bean().mint(user3, 10e6);
C.bean().mint(user4, 10e6);
C.bean().mint(user5, 10e6);
}
function approveTokensForUser(address user) prank(user) internal {
mockWETH.approve(address(diamond), type(uint256).max);
unripeLP.approve(address(diamond), type(uint256).max);
unripeBean.approve(address(diamond), type(uint256).max);
well.approve(address(diamond), type(uint256).max);
C.bean().approve(address(field), type(uint256).max);
C.bean().approve(address(field), type(uint256).max);
}
function dewhitelistCurvePool() public {
vm.prank(deployer);
whitelist.dewhitelistToken(C.CURVE_BEAN_METAPOOL);
}
function mintWETHtoUser(address user) prank(user) internal {
mockWETH.mint(user, 1000e18);
}
function mintUnripeLPToUser1() internal {
unripeLP.mint(user1, 1000e6);
}
function mintUnripeBeanToUser1() internal {
unripeBean.mint(user1, 1000e6);
}
function enableFertilizerAndMintActiveFertilizers() internal {
// second parameter is the unfertilizedIndex
fertilizer.setFertilizerE(true, 10000e6);
vm.prank(deployer);
fertilizer.addFertilizerOwner(7500, 1e11, 99);
vm.prank(deployer);
fertilizer.addFertilizerOwner(6200, 1e11, 99);
addUnripeTokensToFacet();
}
function addUnripeTokensToFacet() prank(deployer) internal {
unripe.addUnripeToken(C.UNRIPE_BEAN, C.BEAN, bytes32(0));
unripe.addUnripeToken(C.UNRIPE_LP, C.BEAN_ETH_WELL, bytes32(0));
}
function callSunriseForUser1() prank(user1) internal {
_ensurePreConditions();
_advanceInTime(2 hours);
season.sunrise();
}
function setOraclePrices(bool makeOracleFail, int256 chainlinkPrice, uint256 ethUsdtPrice, uint256 ethUsdcPrice) internal {
if (makeOracleFail) {
_addEthUsdPriceChainlink(0);
oracleFailed = true;
} else {
oracleFailed = false;
_addEthUsdPriceChainlink(chainlinkPrice);
_setEthUsdtPrice(ethUsdtPrice);
_setEthUsdcPrice(ethUsdcPrice);
}
}
////////////////////////////////////////// TESTS //////////////////////////////////////////
function testWrongCalcId_whenOracleFails() public {
_prepareForAbovePeg();
_advanceInTime(1 hours);
uint256 _snapId = vm.snapshot();
// When sunrise succeeds
vm.prank(user4);
season.sunrise();
// Then print results
_printProtocolState();
assertEq(season.getT(), 5, "when succeeds t should be 5");
// Then revert it to prepare for the season that will fail
vm.revertTo(_snapId);
// Prepare for the season that will fail
setOraclePrices(true, 0, 0, 0);
// When sunrise fails
vm.prank(user4);
season.sunrise();
console.log("Oracle failed, see results");
_printProtocolState();
assertEq(season.getT(), 1, "when succeeds t should be 1");
}
function _printProtocolState() internal {
console.log("-------------- Results --------------");
console.log("");
console.log("thisSowTime: ", season.thisSowTime());
console.log("lastSowTime: ", season.lastSowTime());
console.log("getUsdTokenPrice: ", season.getUsdTokenPrice());
console.log("getReserve0: ", season.getReserve0());
console.log("getReserve1: ", season.getReserve1());
console.log("getAbovePeg: ", season.getAbovePeg());
console.log("getSoil: ", season.getSoil());
console.log("lastDSoil: ", season.lastDSoil());
console.log("s.w.t: ", season.getT());
console.log("remaining pods: ", field.remainingPods());
}
function _prepareForAbovePeg() internal {
season.mockSetSopWell(address(well));
season.captureWellE(address(well));
season.setYieldE(5); // s.w.t
setOraclePrices(false, 1000e6, 1000e6, 1000e6);
season.setLastSowTimeE(1);
season.setNextSowTimeE(10);
season.calcCaseIdE(1e18, 1);
season.setAbovePegE(true);
}
////////////////////////////////////////// HELPERS //////////////////////////////////////////
function _ensurePreConditions() internal {
assertTrue(season.thisSowTime() == type(uint32).max, "thisSowTime should be max");
assertTrue(season.lastSowTime() == type(uint32).max, "thisLastSowTime should be max");
assertEq(season.getIsFarm(), 1, "isFarm should be 1");
assertEq(season.getUsdTokenPrice(), 1, "usdTokenPrice should be 1");
assertEq(season.getReserve0(), 1, "reserve0 should be 1");
assertEq(season.getReserve1(), 1, "reserve1 should be 1");
assertFalse(season.getAbovePeg(), "pre - abovePeg should be false");
assertEq(season.getSoil(), 0, "soil should be == 0");
}
}
Output:
handleRain caseId: 0
-------------- Results --------------
thisSowTime: 4294967295
lastSowTime: 4294967295
getUsdTokenPrice: 1
getReserve0: 1
getReserve1: 1
getAbovePeg: false
getSoil: 462832752243
lastDSoil: 0
s.w.t: 5
remaining pods: 467461079765
handleRain caseId: 3
Oracle failed, see results
-------------- Results --------------
thisSowTime: 4294967295
lastSowTime: 4294967295
getUsdTokenPrice: 1
getReserve0: 1
getReserve1: 1
getAbovePeg: false
getSoil: 0
lastDSoil: 0
s.w.t: 1
remaining pods: 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 29.45s (3.32ms CPU time)
ps: a console.log was added to the handleRain function to print the caseId.
Result: In a normal scenario the temperature would have remained at the value 5 but in this case was set to 1 and remaining pods/soil are also set to zero when in fact they should not.
Manual Review & Foundry
It is noticed that the developers have the intention of never reverting the sunrise function to decrease the risk of depeg and breaking the incentive for users calling it. But at the same time, those state variables shouldn't be updated as if the system is working correctly because they will impact the next season as stated in this finding.
It is tricky to propose a simple fix to the problem without impacting the system as a whole. Here are a few ideas that could be used:
getMintFertilizerOut), getting the well price(getRatiosAndBeanIndex), and getConstantProductWell. As the oracle is used along the protocol in many places, the latest successful price would be often up-to-date and within the limit time defined to use the previous price when the chainlink oracle fails.deltaB and abovePeg variables, as these disrupt the peg mechanism logic.Chainlink oracle returns stale price due to CHAINLINK_TIMEOUT variable in LibChainlinkOracle being set to 4 hoursSubmitted by Igdbaxe, Bube, golanger85. Selected submission by: Bube.
The LibChainlinkOracle library utilizes a CHAINLINK_TIMEOUT constant set to 14400 seconds (4 hours). This duration is four times longer than the Chainlink heartbeat that is 3600 seconds (1 hour), potentially introducing a significant delay in recognizing stale or outdated price data.
The LibChainlinkOracle::checkForInvalidTimestampOrAnswer function accepts three input arguments: timestamp, answer and currentTimestamp and check if the return answer from Chainlinlink Oracle or the timestamp is invalid:
function checkForInvalidTimestampOrAnswer(
uint256 timestamp,
int256 answer,
uint256 currentTimestamp
) private pure returns (bool) {
// Check for an invalid timeStamp that is 0, or in the future
if (timestamp == 0 || timestamp > currentTimestamp) return true;
// Check if Chainlink's price feed has timed out
if (currentTimestamp.sub(timestamp) > CHAINLINK_TIMEOUT) return true;
// Check for non-positive price
if (answer <= 0) return true;
}
The function also checks if the difference between the currentTimestamp and the timestamp is greater then CHAINLINK_TIMEOUT. The CHAINLINK_TIMEOUT is defined to be 4 hours:
uint256 public constant CHAINLINK_TIMEOUT = 14400; // 4 hours: 60 * 60 * 4
The Chainlink heartbeat indicates the expected frequency of updates from the oracle. The Chainlink heartbeat on Ethereum for Eth/Usd is 3600 seconds (1 hour).
But the defined CHAINLINK_TIMEOUT in the LibChainlinkOracle is 14400 seconds (4 hours).
A CHAINLINK_TIMEOUT that is significantly longer than the heartbeat can lead to scenarios where the LibChainlinkOracle library accepts data that may no longer reflect current market conditions. Also, in volatile markets, a 4-hour window leads to accepting outdated prices, increasing the risk of price slippage.
Manual Review
Consider reducing the CHAINLINK_TIMEOUT to align more closely with the Chainlink heartbeat on Ethereum, enhancing the relevance of the price data.
Submitted by golanger85.
LibChainlinkOracle is not fully compatible with Chainlink's data model due to the lack of support for phaseId and aggregatorRoundId. Chainlink's roundID is a composite number combining a phaseID and an aggregatorRoundID.
The phaseID changes whenever there is an upgrade to the underlying aggregator, and this change causes
a significant jump in the roundID values due to the bit-shifting operation described in the documentation.
ref here: https://docs.chain.link/data-feeds/historical-data#solidity
The Beanstalk LibChainlinkOracle misinterprets the progression of roundID as sequential, overlooking Chainlink's unique bundling of phaseId and aggregatorRoundId. With the advancement of phaseID, there's an exponential increase in roundID by 2^64, leading to a temporal suspension until a new interval commences. This will instigate a denial-of-service scenario. The getEthUsdTwap and getEthUsdPrice functions are particularly susceptible to this vulnerability, as they rely on accurate TWAP values for their computations, which effects for example any calls reliant on Oracle data.
function getRoundData(uint80 _roundId)
public
view
virtual
override
returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
)
{
(uint16 phaseId, uint64 aggregatorRoundId) = parseIds(_roundId);
(
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 ansIn
) = phaseAggregators[phaseId].getRoundData(aggregatorRoundId);
return addPhaseIds(roundId, answer, startedAt, updatedAt, ansIn, phaseId);
}
function latestRoundData()
public
view
virtual
override
returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
)
{
Phase memory current = currentPhase; // cache storage reads
(
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 ansIn
) = current.aggregator.latestRoundData();
return addPhaseIds(roundId, answer, startedAt, updatedAt, ansIn, current.id);
}
https://etherscan.io/address/0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419#code
The code segment extracted from the ETH/USD Chainlink aggregator above highlights the composite structure of roundId,
integrating both phaseId and aggregatorRoundId. As highlighted, an increment in phaseId leads to a substantial
leap in roundId by 2^64, thereby bypassing a number of "rounds." Consequently, any attempt to query
currentRound - 1 post-upgrade encounters a non-existent round, triggering a revert. This condition could persist up to 24 hours based on configuration, impacting the timely execution of getEthUsdTwap and getEthUsdPrice.
These functions, once operational again, might utilize altered TWAP values for computations, diverging from expected outcomes
If a phaseID increment occurs, it results in a jump in ``````roundID values, creating a gap in the sequence.
When there are attempts to access round data for roundIDs within this gap, it will encounter inaccurate
rounds, potentially causing the function to fail or return incorrect data, considering when the phaseID is incremented the roundID increases by 2 ** 64. This discrepancy can lead to a denial-of-servicein any calls to the oracle.
Manual Review
Check return values of roundId. If the roundID is a nonzero value and is reverting then the oracle needs to try again with a lower phaseId.
minAnswer, impacting fertilizer mintingSubmitted by 0xTheBlackPanther, 0xBeastBoy, Igdbaxe. Selected submission by: 0xTheBlackPanther.
The Chainlink aggregator utilized in the LibEthUsdOracle contract lacks a mechanism to detect and handle scenarios where the price of an asset falls outside of a predetermined price band. This limitation can result in the oracle returning the minPrice instead of the actual price of the asset during extreme market events, such as a significant drop in value. Consequently, users may continue to interact with the system, such as minting fertilizer tokens, using inaccurate price data. similar case happened with Venus on BSC when LUNA imploded
More Refs for similar issues like this:
The Chainlink aggregator can lead to potential exploitation of price discrepancies during extreme market conditions. For instance, if the price of an asset experiences a sudden crash, the oracle may continue to provide the minPrice, allowing users to conduct transactions at incorrect prices. This could result in financial losses for users and undermine the integrity of the system.
In our scenario, the mintFertilizer function within the FertilizerFacet contract, although it falls out of our immediate scope, relies on the LibEthUsdOracle.getEthUsdPrice() function (within our scope) to fetch the ETH/USD price from the Chainlink oracle. This price is crucial for calculating the amount of Fertilizer tokens that can be acquired with the provided wethAmountIn of WETH. However, if this function returns the minPrice during extreme market events, it would not reflect the actual price of the asset. Consequently, users could continue to mint fertilizer tokens using this inaccurate price data, leading to transactions occurring at incorrect prices.
It is recommended to enhance the Chainlink oracle (LibEthUsdOracle) by implementing a mechanism to check the returned answer against predefined minPrice and maxPrice bounds. If the answer falls outside of these bounds, the oracle should revert the transaction, indicating that the price data is not reliable due to market conditions.
percentBeansRecapped & percentLPRecappedSubmitted by 0xBeastBoy.
LibUnripe:percentBeansRecapped & LibUnripe:percentLPRecapped functions calculate the percentage of Unripe Beans and Unripe LPs that have been recapitalized, respectively. These percentages are calculated based on the underlying balance of the Unripe Tokens and their total supply. There is no check if the totalSupply is zero which is used as division in the calculation.
See the following code for both the functions:
/**
* @notice Returns the percentage that Unripe Beans have been recapitalized.
*/
function percentBeansRecapped() internal view returns (uint256 percent) {
AppStorage storage s = LibAppStorage.diamondStorage();
return s.u[C.UNRIPE_BEAN].balanceOfUnderlying.mul(DECIMALS).div(C.unripeBean().totalSupply());
}
/**
* @notice Returns the percentage that Unripe LP have been recapitalized.
*/
function percentLPRecapped() internal view returns (uint256 percent) {
AppStorage storage s = LibAppStorage.diamondStorage();
return C.unripeLPPerDollar().mul(s.recapitalized).div(C.unripeLP().totalSupply());
}
If the totalSupply in these two functions becomes zero, the calculation of the percentage of recapitalized Unripe Beans or LP tokens would result in a division by zero error. This is because of the denominator in the calculation. When the total supply is zero, dividing by zero is not defined in Solidity, and the contract would revert with an error.
These functions are used widely across the different contracts at crucial places. So they will effect a lot of functionalities.
Manual Review
To handle this scenario, appropriate checks should be added to ensure that the totalSupply of Unripe Beans or LP tokens is non-zero before performing the division operation.