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

`LibChainlinkOracle#getTwap()` could revert for valid TWAP queries, due to the assumption that `latestRoundData` and `getRoundData` should be treated similarly by protocol

bauchibred

Summary

The LibChainlinkOracle#getTwap() function in the Beanstalk protocol would revert for valid TWAP queries due to the incorrect assumption that latestRoundData and getRoundData should be treated equally. This results in potential reverts when calculating TWAP using historical data in valid ranges that are not supposed to revert.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/Oracle/LibChainlinkOracle.sol#L95-L164

​
function getTwap(
address priceAggregatorAddress,
uint256 maxTimeout,
uint256 lookback
) internal view returns (uint256 price) {
IChainlinkAggregator priceAggregator = IChainlinkAggregator(priceAggregatorAddress);
// First, try to get current decimal precision:
uint8 decimals;
try priceAggregator.decimals() returns (uint8 _decimals) {
// If call to Chainlink succeeds, record the current decimal precision
decimals = _decimals;
} catch {
// If call to Chainlink aggregator reverts, return a price of 0 indicating failure
return 0;
}
​
// Secondly, try to get latest price data:
try priceAggregator.latestRoundData() returns (
uint80 roundId,
int256 answer,
uint256 /* startedAt */,
uint256 timestamp,
uint80 /* answeredInRound */
) {
// Check for an invalid roundId that is 0
if (roundId == 0) return 0;
if (checkForInvalidTimestampOrAnswer(timestamp, answer, block.timestamp, maxTimeout)) {
return 0;
}
​
TwapVariables memory t;
​
t.endTimestamp = block.timestamp.sub(lookback);
// Check if last round was more than `lookback` ago.
if (timestamp <= t.endTimestamp) {
return uint256(answer).mul(PRECISION).div(10 ** decimals);
} else {
t.lastTimestamp = block.timestamp;
// Loop through previous rounds and compute cumulative sum until
// a round at least `lookback` seconds ago is reached.
while (timestamp > t.endTimestamp) {
t.cumulativePrice = t.cumulativePrice.add(
uint256(answer).mul(t.lastTimestamp.sub(timestamp))
);
roundId -= 1;
t.lastTimestamp = timestamp;
//@audit
(answer, timestamp) = getRoundData(priceAggregator, roundId);
if (
checkForInvalidTimestampOrAnswer(
timestamp,
answer,
t.lastTimestamp,
maxTimeout
)
) {
return 0;
}
}
t.cumulativePrice = t.cumulativePrice.add(
uint256(answer).mul(t.lastTimestamp.sub(t.endTimestamp))
);
return t.cumulativePrice.mul(PRECISION).div(10 ** decimals).div(lookback);
}
} catch {
// If call to Chainlink aggregator reverts, return a price of 0 indicating failure
return 0;
}
}
​

This function is used to returns the TWAP price from the Chainlink Oracle over the past lookback seconds, issue as hinted by the @audit tag it makes a query to getRoundData to get the specific data for that round, and the implementation of this function is here https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/Oracle/LibChainlinkOracle.sol#L165-L180

function getRoundData(
IChainlinkAggregator priceAggregator,
uint80 roundId
) private view returns (int256, uint256) {
try priceAggregator.getRoundData(roundId) returns (
uint80 /* roundId */,
int256 _answer,
uint256 /* startedAt */,
uint256 _timestamp,
uint80 /* answeredInRound */
) {
return (_answer, _timestamp);
} catch {
return (-1, 0);
}
}

Now, going to the official documentation by Chainlink, we can see that these are the Return values of the getRoundData call :

roundId: The round ID
answer: The answer for this round
startedAt: Timestamp of when the round started
updatedAt: Timestamp of when the round was updated
answeredInRound: Deprecated - Previously used when answers could take multiple rounds to be

Now would be key to note that the logic here unlike with latestRoundData where as the name suggests we should expect the updatedAt to not be far in the past, in the case of getRoundData for previous rounds, the updatedAt parameter can only be the last timestamp of when the round was updated.

Problem however with this integration is the fact that when checking both the call to latestRoundData & getRoundData the checkForInvalidTimestampOrAnswer functionality is being queried, as can be seen here, here & here, evidently, all these snippets are attaching the same maxTimeout to the calls, now where as that would be valid for instantaneous attempt at getting prices, for example let's use the LibEthUsdOracle as an example, we can see that from here the maxTimeour is set as the FOUR_HOUR_TIMEOUT https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/Oracle/LibEthUsdOracle.sol#L39-L45

function getEthUsdPrice() internal view returns (uint256) {
return
LibChainlinkOracle.getPrice(
C.ETH_USD_CHAINLINK_PRICE_AGGREGATOR,
LibChainlinkOracle.FOUR_HOUR_TIMEOUT
);
}

This would be correct considering it's an attempt to get the instantaneous price, however from the same LibEthUsdOracle, we can see that this is how the twap price is being gotten https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/Oracle/LibEthUsdOracle.sol#L53-L65

function getEthUsdPrice(uint256 lookback) internal view returns (uint256) {
return
lookback > 0
? LibChainlinkOracle.getTwap(
C.ETH_USD_CHAINLINK_PRICE_AGGREGATOR,
LibChainlinkOracle.FOUR_HOUR_TIMEOUT,
lookback
)
: LibChainlinkOracle.getPrice(
C.ETH_USD_CHAINLINK_PRICE_AGGREGATOR,
LibChainlinkOracle.FOUR_HOUR_TIMEOUT
);
}

Evidently, even if lookback > 0 the maxTimeout is the FOUR_HOUR_TIMEOUT but this would cause reverts in the @audit hinted part of the code snippet below, considering in the case of getting twap prices the updatedAT for these past rounds are also timestamps in the past
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/Oracle/LibChainlinkOracle.sol#L182-L196

function checkForInvalidTimestampOrAnswer(
uint256 timestamp,
int256 answer,
uint256 currentTimestamp,
uint256 maxTimeout
) 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
//@audit for previous rounds there is a high potential this reverts considering the
if (currentTimestamp.sub(timestamp) > maxTimeout) return true;
// Check for non-positive price
if (answer <= 0) return true;
​
return false;
}

Impact

LibChainlinkOracle#getTwap() would be broken in some cases since the same timeout value ism used for data received from the previous rounds, which would have their updatedAt timestamps in the past and as such have this block if (currentTimestamp.sub(timestamp) > maxTimeout) return true; triggered.

NB: This part is the direct snippet that has the last updated at time for the round to be checked with the current timestamp: https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/Oracle/LibChainlinkOracle.sol#L132-L158.

Tools Used

Manual review

Recommendations

Consider not having the same timeout check for the data gotten from the rounds and also latestRoundData . Instead, depending on how far back lookback is, increment the timeout value too.

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Instantaneous and TWAP prices should be checked with the same `checkForInvalidTimestampOrAnswer`

Appeal created

bauchibred Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Instantaneous and TWAP prices should be checked with the same `checkForInvalidTimestampOrAnswer`

Support

FAQs

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