Platform: Code4rena
Start Date: 02/06/2023
Pot Size: $100,000 USDC
Total HM: 15
Participants: 75
Period: 7 days
Judge: Picodes
Total Solo HM: 5
Id: 249
League: ETH
Rank: 38/75
Findings: 1
Award: $41.33
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: Madalad
Also found by: Aymen0909, Bauchibred, Breeje, DadeKuma, Hama, LaScaloneta, Madalad, MohammedRizwan, bin2chen, dwward3n, erictee, etherhood, kutugu, peanuts, piyushshukla, rvierdiiev, saneryee, tallo, turvy_fuzz, whimints
41.334 USDC - $41.33
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L646 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L648
Chainlink's latestRoundData is used here to retrieve price feed data, however there is insufficient protection against price staleness.
Return arguments other than int256 answer are necessary to determine the validity of the returned price, as it is possible for an outdated price to be received. See here for reasons why a price feed might stop updating.
The return value updatedAt contains the timestamp at which the received price was last updated, and can be used to ensure that the price is not outdated. See more information about latestRoundID in the Chainlink docs. Inaccurate price data can lead to functions not working as expected and/or lost funds.
function getPORFeedData() internal view returns ( uint256, uint256, uint256 ) { (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy()) .latestRoundData(); (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy()) .latestRoundData(); return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number); }
Manual Review
Add a check for the updatedAt returned value from latestRoundData.
function getPORFeedData() internal view returns ( uint256, uint256, uint256 ) { - (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy()) + (, int256 totalETHBalanceInInt, , uint256 balanceUpdatedAt, ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy()) .latestRoundData(); + require(block.timestamp - balanceUpdatedAt <= MAX_DELAY, "stale price"); - (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy()) + (, int256 totalETHXSupplyInInt, , uint256 supplyUpdatedAt, ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy()) .latestRoundData(); + require(block.timestamp - supplyUpdatedAt <= MAX_DELAY, "stale price"); return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number); }
Oracle
#0 - c4-judge
2023-06-09T23:24:44Z
Picodes marked the issue as primary issue
#1 - c4-sponsor
2023-06-19T06:16:01Z
manoj9april marked the issue as sponsor acknowledged
#2 - manoj9april
2023-06-19T06:16:13Z
Solution with chainlink is not finalized.
#3 - c4-judge
2023-07-02T10:49:21Z
Picodes marked the issue as selected for report
🌟 Selected for report: Madalad
Also found by: Aymen0909, Bauchibred, Breeje, DadeKuma, Hama, LaScaloneta, Madalad, MohammedRizwan, bin2chen, dwward3n, erictee, etherhood, kutugu, peanuts, piyushshukla, rvierdiiev, saneryee, tallo, turvy_fuzz, whimints
41.334 USDC - $41.33
https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L646 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L648
Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset.
Chainlink's latestRoundData pulls the associated aggregator and requests round data from it. ChainlinkAggregators have minPrice and maxPrice circuit breakers built into them. This means that if the price of the asset drops below the minPrice, the protocol will continue to value the token at minPrice instead of it's actual value. This will allow users to exploit certain parts of the protocol.
This discrepency could cause major issues with accounting within the Stader protocol and potentially lead to loss of funds. This effect lead to the Venus protocol on BSC being exploited for $13.5m when LUNA imploded.
function getPORFeedData() internal view returns ( uint256, uint256, uint256 ) { (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy()) .latestRoundData(); (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy()) .latestRoundData(); // @audit missing price check return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number); }
Manual review
Add a check to revert if the price received from the oracle is out of bounds:
function getPORFeedData() internal view returns ( uint256, uint256, uint256 ) { (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy()) .latestRoundData(); (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy()) .latestRoundData(); + int192 minBalance = IChainLinkAggregator(staderConfig.getETHBalancePORFeedProxy()).minAnswer(); + int192 maxBalance = IChainLinkAggregator(staderConfig.getETHBalancePORFeedProxy()).maxAnswer(); + require(totalETHBalanceInInt > minBalance, "Lower price bound breached"); + require(totalETHBalanceInInt < maxBalance, "Upper price bound breached"); + int192 minSupply = IChainLinkAggregator(staderConfig.getETHXSupplyPORFeedProxy()).minAnswer(); + int192 maxSupply = IChainLinkAggregator(staderConfig.getETHXSupplyPORFeedProxy()).maxAnswer(); + require(totalETHXSupplyInInt > minSupply, "Lower price bound breached"); + require(totalETHXSupplyInInt < maxSupply, "Upper price bound breached"); return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number); }
Oracle
#0 - c4-judge
2023-06-09T23:25:42Z
Picodes marked the issue as duplicate of #15
#1 - c4-judge
2023-07-02T10:49:56Z
Picodes marked the issue as satisfactory
#2 - Madalad
2023-07-03T16:43:37Z
I disagree that this issue is a duplicate of #15 Chainlink's latestRoundData may return stale or incorrect result.
Firstly, the preconditions for each finding are different. For this issue (#16), the protocol is vulnerable against extreme price volatility, even if the oracle continues to behave as it is expected to. However issue #15 addresses the possibility of an oracle not updating the price for a non-standard amount of time, which can occur regardless of whether the underlying asset is experiencing price volatility.
Secondly, the proposed fixes are different. Implementing the recommended mitigation steps for issue #16 would not protect against the impact described in issue #15, and vice versa.
For these reasons I believe that these two issues address two different vulnerabilities and ought to be treated as separate.