roguereddwarf - ChainlinkAggregator: binary search for roundId does not work correctly and Oracle can even end up temporarily DOSed

[sherlock-admin]

roguereddwarf

high

ChainlinkAggregator: binary search for roundId does not work correctly and Oracle can even end up temporarily DOSed

Summary

When a phase switchover occurs, it can be necessary that phases need to be searched for a roundId with a timestamp as close as possible but bigger than targetTimestamp.

Finding the roundId with the closest possible timestamp is necessary according to the sponsor to minimize the delay of position changes:

The binary search algorithm is not able to find this best roundId which thereby causes unintended position changes.

Also it can occur that the ChainlinkAggregator library is unable to find a valid roundId at all (as opposed to only not finding the "best").

This would cause the Oracle to be temporarily DOSed until there are more valid rounds.

Vulnerability Detail

Let's look at the binary search algorithm:
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-oracle/contracts/types/ChainlinkAggregator.sol#L123-L156

The part that we are particularly interested in is:
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-oracle/contracts/types/ChainlinkAggregator.sol#L139-L149

Let's say in a phase there's only one valid round (roundId=1) and the timestamp for this round is greater than targetTimestamp

We would expect the roundId that the binary search finds to be roundId=1.

The binary search loop is executed with minRoundId=1 and maxRoundId=1001.

All the above conditions can easily occur in reality, they represent the basic scenario under which this algorithm executes.

minRoundId and maxRoundId change like this in the iterations of the loop:

minRoundId=1
maxRoundId=1001

-> 

minRoundId=1
maxRoundId=501

-> 

minRoundId=1
maxRoundId=251

-> 

minRoundId=1
maxRoundId=126

-> 

minRoundId=1
maxRoundId=63

-> 

minRoundId=1
maxRoundId=32

-> 

minRoundId=1
maxRoundId=16

-> 

minRoundId=1
maxRoundId=8

-> 

minRoundId=1
maxRoundId=4

-> 

minRoundId=1
maxRoundId=2

Now the loop terminates because
minRoundId + 1 !< maxRoundId

Since we assumed that roundId=2 is invalid, the function returns 0 (maxTimestamp=type(uint256).max):

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-oracle/contracts/types/ChainlinkAggregator.sol#L153-L155

In the case that latestRound.roundId is equal to the roundId=1 (i.e. same phase and same round id which could not be found) there would be no other valid rounds that the ChainlinkAggregator can find which causes a temporary DOS.

Impact

As explained above this would result in sub-optimal and unintended position changes in the best case.
In the worst-case the Oracle can be temporarily DOSed, unable to find a valid roundId.

This means that users cannot interact with the perennial protocol because the Oracle cannot be synced.
So they cannot close losing trades which is a loss of funds.

The DOS can occur since the while loop searching the phases does not terminate:
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-oracle/contracts/types/ChainlinkAggregator.sol#L88-L91

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-oracle/contracts/types/ChainlinkAggregator.sol#L123-L149

Tool used

Manual Review

Recommendation

I recommend to add a check if minRoundId is a valid solution for the binary search.
If it is, minRoundId should be used to return the result instead of maxRoundId:

         // If the found timestamp is not greater than target timestamp or no max was found, then the desired round does
         // not exist in this phase
-        if (maxTimestamp <= targetTimestamp || maxTimestamp == type(uint256).max) return 0;
+        if ((minTimestamp <= targetTimestamp || minTimestamp == type(uint256).max) && (maxTimestamp <= targetTimestamp || maxTimestamp == type(uint256).max)) return 0;
 
+        if (minTimestamp > targetTimestamp) {
+            return _aggregatorRoundIdToProxyRoundId(phaseId, uint80(minRoundId));
+        }
         return _aggregatorRoundIdToProxyRoundId(phaseId, uint80(maxRoundId));
     }

After applying the changes, the binary search only returns 0 if both minRoundId and maxRoundId are not a valid result.

If this line is passed we know that either of both is valid and we can use minRoundId if it is the better result.

[roguereddwarf]

Escalate for 10 USDC

I think this should be a "High" severity finding.
The binary search lies at the core of the protocol.
All functionality for users to open / close trades and liquidations relies on the Chainlink oracle.

By not finding a valid roundId obviously many users are put at risk of losing funds (-> not being able to close trades).

Similarly when an unintended (i.e. sub-optimal) roundId is found this leads to a similar scenario where settlements / liquidations occur at unintended prices.

In summary, the fact that the binary search algorithm lies at the core of the protocol and there is a very direct loss of funds makes me think this should be "High" severity.

[sherlock-admin]

Escalate for 10 USDC

I think this should be a "High" severity finding.
The binary search lies at the core of the protocol.
All functionality for users to open / close trades and liquidations relies on the Chainlink oracle.

By not finding a valid roundId obviously many users are put at risk of losing funds (-> not being able to close trades).

Similarly when an unintended (i.e. sub-optimal) roundId is found this leads to a similar scenario where settlements / liquidations occur at unintended prices.

In summary, the fact that the binary search algorithm lies at the core of the protocol and there is a very direct loss of funds makes me think this should be "High" severity.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

Comment from watson:
Chainlink: ETH/USD has been deployed for 3 years, and the current phaseID is only 6. The binary search is only triggered when this condition is met, that is, when a new phaseID is generated. This is infrequent.
This is from the chainlink docs: phaseId is incremented each time the underlying aggregator implementation is updated.

[KenzoAgada]

The issue is dependent upon a Chainlink changing of phase (which is quite infrequent) and the new phase having only 1 round.
The impact as stated in the finding is that in those conditions, the product flywheel is jammed until further rounds are issued. (Then, the algorithm will correctly return round 2.)
So the impact is only very temporary and rare DOS (which can impact user funds).

I think escalation is invalid and medium severity is appropriate.

[arjun-io]

To add here - the binary search is also a backup solution to simply checking if roundId + 1 exists in the previous phase (code). So additionally the last seen round in the previous phase has to be the very last roundID in that phase

[jacksanford1]

Agree with Medium due to Kenzo's reasons (unlikely and temporary situation which only indirectly could result in user funds loss due to liquidations). cc @roguereddwarf

[roguereddwarf]

Agreed

[jacksanford1]

Result:
Medium
Unique
Unlikely and temporary situation which only indirectly could result in user funds loss due to liquidations is part of the reason why this is not being upgraded to a High.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• roguereddwarf: rejected

[arjun-io]

Fixed in: equilibria-xyz/perennial-mono#207 as per the recommended fix