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-04 [Oval] Updating Parameters Inside the BaseController Contract May Lead to Undesired Outcomes #19

Conversation

Reinis-FRP
Copy link
Collaborator

Addresses audit issue: M-04 [Oval] Updating Parameters Inside the BaseController Contract May Lead to Undesired Outcomes

The maxAge() parameter has been introduced to controllers in order to limit the staleness of
the data returned by the adapters. Stale data could possibly be returned if no "unlock"
transaction happens during the lockWindow() period. In such a case, the latest available
price data for block.timestamp - lockWindow() is returned. The maxAge() parameter
specifies the maximum age of such price data and, in case when the price is too old, the most
recent price data is returned. Inside both ImmutableController and
MutableUnlockersController , the maxAge() parameter is immutable and cannot be
changed, but it is possible to change it inside the BaseController contract.

However, changing this parameter may lead to undesired results. One of the problems which
may happen after such a change arises when the price data from an oracle is not unlocked for
the lockWindow() time and the timestamp of the most recent price older than the
lockWindow() is older than what the maxAge() parameter allows. In such a case, the
newest price data is returned in order to avoid returning too stale prices. However, if
maxAge() is then changed to a higher value, such that the most recent price as of
block.timestamp - lockWindow() is not older than the maxAge() , that older price will
be returned. It means that in the same block, the oracle adapter may return the most recent
data and then the stale data, which is an unexpected and incorrect behavior.

The opposite scenario is also possible when the maxAge() parameter is decreased. In such a
case, the most recent data will be unexpectedly returned which will take away the possibility of
extracting OEV for the newest price for Oval by unlocking the price. It should be noted that
these are just two of many different scenarios that may happen if the maxAge() parameter is
changed. Similar problems may also arise when the unlockWindow() and
maxTraversal() parameters are modified, even if maxAge() stays the same.

Consider setting the maxAge() , unlockWindow() , and maxTraversal() parameters as
immutable inside the BaseController in order to avoid returning multiple different prices for
the same timestamp.

This addresses the issue by adding validation checks when changing BaseController parameters.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP Reinis-FRP marked this pull request as draft June 17, 2024 11:51
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP Reinis-FRP marked this pull request as ready for review June 17, 2024 12:29
@@ -101,4 +113,10 @@ abstract contract BaseController is Ownable, Oval {
function maxAge() public view override returns (uint256) {
return maxAge_;
}

// Helper function to ensure that changing controller parameters does not change the returned data.
function _checkDataNotChanged(int256 currentAnswer, uint256 currentTimestamp) internal view {
Copy link
Member

Choose a reason for hiding this comment

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

nice. good structure here.

@Reinis-FRP Reinis-FRP changed the title fix[oval-audit-m-04]: validate base controller parameters M-04 [Oval] Updating Parameters Inside the BaseController Contract May Lead to Undesired Outcomes Jun 17, 2024
Copy link
Collaborator

@md0x md0x left a comment

Choose a reason for hiding this comment

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

Looks good!

@Reinis-FRP Reinis-FRP merged commit 0e94235 into master Jun 18, 2024
3 checks passed
@Reinis-FRP Reinis-FRP deleted the reinis/uma-2649-m-04-oval-updating-parameters-inside-the-basecontroller branch June 18, 2024 15:05
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.

None yet

3 participants