Olympus DAO contest - okkothejawa's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L165-L171

Vulnerability details

Impact

Price oracle may fail and revert due to the inconsistency in the staleness checks.

Proof of Concept

In the getCurrentPrice() of PRICE.sol, Chainlink oracles are used to get the price of OHM against a reserve token, and a staleness check is used to make sure the price oracles are reporting fresh data. Yet the freshness requirements are inconsistent, for OHM, updatedAt should be lower than current timestamp minus three times the observation frequency, while for the reserve price, it is required that updatedAt should be lower than current timestamp minus the observation frequency. Our understanding is that that frequency is multiplied by 3 so that there can be some meaningful room where price data is accepted, as the time frame of only observation frequency (multiplied by 1) may not be enough for the oracle to realistically update its data. (In other words, the frequency of new price information might be lower than the observation frequency, which is probably the case as third multiple is used for the OHM price). If this is the case, this inconsistency may lead to the getCurrentPrice() reverting as while third multiple of the observation frequency might give enough space for the first oracle, second oracle's first multiple of frequency time frame might not be enough and it couldn't pass the staleness check due to unrealistic expectation of freshness.

Tools Used

Manual review, talking with devs

Recommended Mitigation Steps

Change the line 171 to

            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))

like line 165.

LOW RISK

1) lastBeat might lag

The variable lastBeat is used to store the timestamp of the last beat, yet it is incremented only by frequency(). While this would work perfectly in the scenario that keeper bots are consistently calling this function with a frequency of frequency() for rewards, lastBeat may start lagging behind the actual time in the case there are some update made to the PRICE contract (updating observation frequency or the moving average duration) and thus PRICE contract is back in the initialized == false state which would result in the call to updateMovingAverage() failing as the function would revert when PRICE is not in the initialized state. This scenario would result in any calls made to the beat reverting, and if the PRICE contract is not initialized immediately (so there is some delay) after the updates, lastBeat would lag behind as it would start getting incremented from the old value up to old value + multiples of frequency(), never reaching the actual timestamp of beats. This can be solved with by making a call to resetBeat or by making sure the updates and initializations are done in an atomic fashion operationally, and thus we think this creates only a low risk.

NON-CRITICAL

Open TODOs

There are three open TODOs in this function, and according to our understanding, this function revokePolicyApprovals does not require a check to see if the address supplied is a deactivated policy, as TRSRY should be only called by policies, and thus only policies would require approvals and revoking the approval of something that is not a policy would not do anything. So, these TODOs can be removed. (https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/TreasuryCustodian.sol#L56)

TYPO

There is a typo in this line (https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L126), numbe should be number.