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

Risk of stale price consumption in YieldOracle #126

Open
hats-bug-reporter bot opened this issue Nov 15, 2024 · 3 comments
Open

Risk of stale price consumption in YieldOracle #126

hats-bug-reporter bot opened this issue Nov 15, 2024 · 3 comments
Labels
invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: @@Tri-pathi
Twitter username: --
Submission hash (on-chain): 0x994391875e9a4768c7b80ecae87ebb7d7b845d79a1e72adbb1266ba9a9a444d5
Severity: medium

Description:
Description

The setCommitDelay() function in the YieldOracle contract presents a potential risk of utilizing stale prices due to the way it handles the update of the commitDelay variable. If the delay is set to a value greater than the current commitDelay , it can block the commitment of the price for a duration equal to delay - commitDelay. This will lead to situations where the price remains stale, even if the oracle is functioning correctly.

Proof of Concept

The current implementation of the setCommitDelay function does not commit prrice if commitDelay has been passed before updating the commitDelay state variable

    /**
     * @notice Updates the commit delay.
     * @param delay The new commit delay.
     */
    function setCommitDelay(uint256 delay) external onlyOwner {
        require(delay <= updateDelay, "Delay out of bounds");

        commitDelay = delay;
    }

Example

  1. updateDelay is 1 day and commitDelay is 1 hr
  2. oracle calls updatePrice() and set the nextPrice
  3. 1 hour 1 min is passed and setCommitDelay is called for updating the commitDelay to 2 hours
  4. Now protocol will consume stale price for next 59 mins since commitPrice can't be called

Impact

protocol will consume stale price

Recommendation

    function setCommitDelay1(uint256 delay) external onlyOwner {
        require(delay <= updateDelay, "Delay out of bounds");
        if ((nextPrice != NO_PRICE) && (lastUpdate + commitDelay < block.timestamp) && (nextPrice - currentPrice >= 0))
        {
            previousPrice = currentPrice;
            currentPrice = nextPrice;
            nextPrice = NO_PRICE;

            emit PriceCommitted(currentPrice);
        }
        commitDelay = delay;
    }
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Nov 15, 2024
@AndreiMVP
Copy link

That's the intended design

@Tri-pathi
Copy link

@AndreiMVP
I can't see any reason to have such design, which will lead to stale price. Even you try to commit price in 'updatePrice()' if nextPrice is not used.

I believe its valid issue or please elaborate more about this design choice?

@geaxed geaxed added invalid This doesn't seem right and removed bug Something isn't working labels Nov 18, 2024
@AndreiMVP
Copy link

It's an update mirroring the RWA backing. The period between updates could be shortened but the price remains stale inbetween according to current version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants