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

`LibChainlinkOracle#getTwap()` is broken due to the assumption around Chainlink feed roundIds it implements

bauchibred

Summary

Beanstalk's LibChainlinkOracle#getTwap() is broken due to incorrect assumptions about Chainlink feed round IDs. This assumption causes the function to potentially query non-existent rounds, leading to reverts and incorrect TWAP calculations.

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;//@audit issue here with the assu
t.lastTimestamp = timestamp;
(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 here is the fact that protocol erroneously assumes the roundId for chainlink feeds are always incremented by 1, however that's not the case.

This is because Chainlink's roundId is made up of 2 packed components, the phaseId and aggregatorRoundId, and whenever the phaseId is incremented 2 ** 64 "rounds" will be skipped, which means that in that instance the roundId does not increase by one... to confirm this check out any feed's aggregator, we can use this, NB this is from the ETH/USD feed on AVAX...

Now we can see that in the LibChainlinkOracle#getTwap()function's execution there is an attempt to loop through previous rounds and compute cumulative sum until a round at least lookback seconds ago is reached, while doing this the roundId is directly decremented by 1, however since during an increment/change in rounds by Chainlink the ID is not always incremented by 1. This would then mean that there is going to be an attempt to query data from a round that never existed which would prompt a failure and break the functionality.

The functionality here is broken, because the getRoundData() would return (-1,0) for a round that doesn't exist, which would then be perceived as an invalid timestamp in the query to checkForInvalidTimestampOrAnswer, here.

Impact

LibChainlinkOracle#getTwap() would be broken in some cases since it would attempt to query data from roundIds that don't exist for a Chainlink feed.

As explained under Vulnerability Details, the current implementation would then cause 0 to be returned from here in getTwap(), which would make the execution to erroneously assume the Chainlink's price feed is broken or frozen where as it isn't, the assumption has been documented here, but in our case even for feeds that are not broken/frozen 0 could be returned.

Tools Used

Manual review

Recommendations

Consider correctly querying past roundIds, use a try/catch on the attempt to get the data from roundId - 1 and if it fails, then reroute the logic to correctly query the previous rounds taking into account that there are two packed components that makes up the roundId and that the Ids do not always montonically increase.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Known - Bean Part 1

Support

FAQs

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