search

sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

WATCHPUG - `OracleVersion latestVersion` of `Oracle.status()` may go backwards when updating to a new oracle provider and result in wrong settlement in `_processPositionLocal()`. #145

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

WATCHPUG

high

OracleVersion latestVersion of Oracle.status() may go backwards when updating to a new oracle provider and result in wrong settlement in _processPositionLocal().

Summary

Vulnerability Detail

This is because when Oracle.update(newProvider) is called, there is no requirement that newProvider.latest().timestamp > oldProvider.latest().timestamp.

During the processLocal, encountering a non-existing version will result in using 0 as the makerValue, longValue, and shortValue to settle PNL, causing the user's collateral to be deducted incorrectly.

This is because L350 is skipped (as the global has been settled to a newer timestamp), and L356 enters the if branch.

PoC

Given:

When:

Impact

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L106-L117

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L327-L364

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L390-L423

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L430-L457

Tool used

Manual Review

Recommendation

Consider requireing newProvider.latest().timestamp > oldProvider.latest().timestamp in Oracle.update(newProvider).

sherlock-admin commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

141345 commented:

d

panprog commented:

invalid because this is owner error who is trusted, also there is no real impact (update will simply revert if the time goes backward)

panprog commented 11 months ago

This should be valid medium, even though not escalated. POC:

      it('latest.timestamp moving back in time', async () => {

        function setupOracle(price: string, timestamp : number, nextTimestamp : number) {
          const oracleVersion = {
            price: parse6decimal(price),
            timestamp: timestamp,
            valid: true,
          }
          oracle.at.whenCalledWith(oracleVersion.timestamp).returns(oracleVersion)
          oracle.status.returns([oracleVersion, nextTimestamp])
          oracle.request.returns()
        }

        var marketParameter = {
          fundingFee: parse6decimal('0.1'),
          interestFee: parse6decimal('0.1'),
          oracleFee: parse6decimal('0.1'),
          riskFee: parse6decimal('0.1'),
          positionFee: parse6decimal('0.1'),
          maxPendingGlobal: 5,
          maxPendingLocal: 3,
          settlementFee: parse6decimal('0'),
          makerRewardRate: parse6decimal('0.0'),
          longRewardRate: parse6decimal('0.0'),
          shortRewardRate: parse6decimal('0.0'),
          makerCloseAlways: false,
          takerCloseAlways: false,
          closed: false,
        }

        await market.connect(owner).updateParameter(marketParameter);

        setupOracle('100', TIMESTAMP, TIMESTAMP + 100);

        var collateral = parse6decimal('1000')
        dsu.transferFrom.whenCalledWith(userB.address, market.address, collateral.mul(1e12)).returns(true)
        await market.connect(userB).update(userB.address, parse6decimal('10.000'), 0, 0, collateral, false)

        var collateral = parse6decimal('120')
        dsu.transferFrom.whenCalledWith(user.address, market.address, collateral.mul(1e12)).returns(true)
        await market.connect(user).update(user.address, 0, parse6decimal('1.000'), 0, collateral, false)

        // open position
        setupOracle('100', TIMESTAMP + 100, TIMESTAMP + 200);
        await market.connect(user).update(user.address, 0, parse6decimal('1.000'), 0, 0, false)

        var info = await market.locals(user.address);
        var pos = await market.positions(user.address);
        console.log("after open (price=100): user collateral = " + info.collateral + " long = " + pos.long);

        // accumulate some pnl
        setupOracle('90', TIMESTAMP + 200, TIMESTAMP + 300);
        await market.connect(user).update(user.address, 0, parse6decimal('1.000'), 0, 0, false)

        var info = await market.locals(user.address);
        var pos = await market.positions(user.address);
        var ver = await market.versions(TIMESTAMP + 200);
        console.log("after settle pnl (price=90): user collateral = " + info.collateral + " long = " + pos.long + " ver_longValue: " + ver.longValue + " ver_makerValue: " + ver.makerValue);

        // add collateral only
        setupOracle('90', TIMESTAMP + 300, TIMESTAMP + 400);
        dsu.transferFrom.whenCalledWith(userB.address, market.address, collateral.mul(1e12)).returns(true)
        await market.connect(userB).update(userB.address, parse6decimal('10.000'), 0, 0, collateral, false)

        // oracle.latest moves back in time
        setupOracle('89', TIMESTAMP + 290, TIMESTAMP + 400);
        await market.connect(user).update(user.address, 0, parse6decimal('1.000'), 0, 0, false)

        var info = await market.locals(user.address);
        var pos = await market.positions(user.address);
        console.log("after move back in time (price=89): user collateral = " + info.collateral + " long = " + pos.long);

        setupOracle('89', TIMESTAMP + 400, TIMESTAMP + 500);
        await market.connect(user).update(user.address, 0, parse6decimal('1.000'), 0, 0, false)
        setupOracle('89', TIMESTAMP + 500, TIMESTAMP + 600);
        await market.connect(user).update(user.address, 0, parse6decimal('1.000'), 0, 0, false)

        var info = await market.locals(user.address);
        var pos = await market.positions(user.address);
        console.log("User settled (price=89): collateral = " + info.collateral + " long = " + pos.long);

      })

Console output:

after open (price=100): user collateral = 120000000 long = 1000000
after settle pnl (price=90): user collateral = 109999994 long = 1000000 ver_longValue: -10000006 ver_makerValue: 1000000
after move back in time (price=89): user collateral = 120000000 long = 1000000
User settled (price=89): collateral = 108999975 long = 1000000
panprog commented 11 months ago

Medium, not high, because:

  1. Can only happen during oracle provider switch (which is a rare admin event by itself)
  2. Very specific condition must be met: previous provider must be commited unrequested past the last requested AND new provider must be commited unrequested with newProvider.latest.timestamp < previousProvider.latest.timestamp AND there should be a user who has pending position at a timestamp > previousProvider.latest.timestamp (no position change to not create new request).
  3. Only possible if accumulated reward is 0, meaning the market must have all reward rates = 0 for the entire market lifetime (otherwise it will revert when trying to accumulate reward, which is unsigned).
  4. The effect is only temporary because the "0" version is invalid and is discarded once a valid version appears (any commit is done for new provider)
hrishibhat commented 11 months ago

Considering this issue a Unique Medium based on the above comments

kbrizzle commented 11 months ago

Looks like we missed this one since it was reopened last minute. After an initial review we think this is a false-positive, but please check our reasoning and let us know if you still see an issue.

The actual potential bug here is an incorrect implementation of the IOracleProvider interface in Oracle. The extra information about what happens inside the Market if latest can be out of order is not relevant, as many things can go wrong if that invariant is not upheld in the implementation.

So what we'd be looking for here for a valid bug is a specific case where an Oracle updating from one sub-oracle to another sub-oracle would cause the latest() to return a value lower than it previously did.

I ran through the case that I think you've outlined here (though I may have translate it incorrectly) and I don't see a way for it to bypass the _latestStale check, which is specifically designed to handle this case.

Case

oracle 0 -> current: 330, latest: 330
oracle 1 -> current: 330, latest: 320

Assuming Oracle.update() was called at 330, once in the state above this check will pass, but this check will still fail. This keeps the Oracle pointed at oracle 0 until the latest clears as expected.

Let us know if there's an example here you can show us that does in fact bypass the _latestStale function.

MLON33 commented 11 months ago

From WatchPug,

No PR attached.

panprog commented 11 months ago

Let us know if there's an example here you can show us that does in fact bypass the _latestStale function.

oracles[global.latest].timestamp - is the last time oracle.request() was called for a provider. Since provider can commit unrequested, provider1.latest.timestamp can be greater than oracles[global.latest].timestamp. So the following values can happen:

To cause these values, the following actions might happen: t=320: Oracle.request() called [oracles[global.latest].timestamp = 320] t=340: provider1.commitRequested() called [provider1.latest.timestamp = 320] t=350: provider1.commit(330) called [provider1.latest.timestamp = 330] t=350: provider2.commit(325) called [provider2.latest.timestamp = 325] t=350: Oracle.update(provider2) called

In such situations both checks in _latestStale() will pass:

  1. 320 > 330: condition not satisfied
  2. 320 >= 325: condition not satisfied So, the provider2 will be set immediately, and so Oracle.lastest() will return provider2.latest() which is earlier than provider1.latest()
kbrizzle commented 11 months ago

Thanks @panprog, this is exactly what we needed 🙏 will get back on a fix.

arjun-io commented 11 months ago

Fixed: https://github.com/equilibria-xyz/perennial-v2/pull/99