Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M-03 [Oval] Newest Prices May Be Reported Ignoring the lockWindow() Constraint #17

Conversation

Reinis-FRP
Copy link
Collaborator

Addresses audit issue: M-03 [Oval] Newest Prices May Be Reported Ignoring the lockWindow() Constraint

The _tryLatestRoundDataAt function is invoked after the tryLatestDataAt function is
called inside the internalLatestData function of the Oval contract. Its timestamp
argument is equal to the maximum of the last unlock time and block.timestamp -
lockWindow() . Before the introduction of the maxAge() parameter, it used to be that in
cases where the price data had been unlocked inside the lockWindow() period, the price
data would be returned. In the opposite case, the most recent price as of block.timestamp
- lockWindow() timestamp would be returned.

Now, the maxAge() parameter has been introduced to limit the staleness of the data returned
by the adapters. If the data to be returned by the _tryLatestRoundDataAt function is older
than the maxAge() , the most recent price data is returned instead. This means that if the
maxAge() is lower than the lockWindow() and the last price is unlocked within the
lockWindow() , but its timestamp is older than the maxAge() parameter, the newest price
will be returned by the internalLatestData function of the Oval contract.

However, this is not the desired behavior as the lock window mechanism has been introduced
in order to guarantee that the "unlock" transactions unlock the most recent price that has not
yet been used by anyone. As a consequence, the OEV opportunity for Oval will be lost since
the newest price will be available for use by anyone before the "unlock" transaction takes
place. A similar scenario may happen if the maxTraversal parameter is set to 0. In such a
case, uninitialized data is returned by the _searchRoundDataAt functions inside the
adapters and hence the check against the maxAge() value will never pass. As a result, the
newest price data will always be returned.

Consider carefully validating the parameters with which the Oval contracts are deployed,
especially ensuring that the max age is always bigger than the lock window and that the
maxTraversal parameter is set to a reasonable value.

This implements the recommended fix by validating Oval controller constructor parameters.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP Reinis-FRP requested review from chrismaree and md0x June 17, 2024 11:32
@chrismaree
Copy link
Member

The title of this PR was wrong. I updated it.

@chrismaree chrismaree changed the title fix[oval-audit-m-03]: validate controller deployment parameters M-03 [Oval] Newest Prices May Be Reported Ignoring the lockWindow() Constraint Jun 17, 2024
Copy link
Member

@chrismaree chrismaree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. there is an important requirement here that we discuss some of the risks and failure modes within the documentation. it would be a useful exercise for us to carefully define the boundaries of these parameters and under what situations the oracle can return inconsistent/unexpected values. it would be useful, as well, to have some unit tests for these.

@Reinis-FRP Reinis-FRP merged commit 2aa2c7d into master Jun 18, 2024
3 checks passed
@Reinis-FRP Reinis-FRP deleted the reinis/uma-2648-m-03-oval-newest-prices-may-be-reported-ignoring-the branch June 18, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants