sherlock-admin commented 1 year ago

0x73696d616f

high

Drained oracle fees from market by depositing and withdrawing immediately without triggering settlement fees

Summary

The oracle fee can be drained, as requests can be made without paying fees by depositing and withdrawing immediately, leading to theft of yield to the keeper and potentially a DoS in the system. The DoS happens because the oracle version must be increased to trigger the settlements (global and local), such that a keeper amount must be available prior to the new version oracle request. This keeper amount would not be available as attackers would have drained the fees before any settlement occurs.

Vulnerability Detail

Market advances in the id of the Global and Local states by fetching latestVersion and currentTimestamp from the oracle, increasing it if there is an update.

When a new position is updated by calling update(), if the order is not empty (when there is a modification in the maker , long or short amounts), it requests a new version from the oracle. This means that users can trigger a request with the smallest position possible (1), not paying any fees.

Fetching a price in the oracle is expensive, thus Perennial attributes an oracleFee to the oracle, which is then fed to the keeper for commiting prices in the oracle. Notice that anyone can be the keeper, the only requirement is to submit a valid price to the oracle.

In the oracle, the incentive is only paid if there was a previous request, most likely from Market (can be any authorized entity). As the oracleFee is only increased on settlements, an oracle request can be triggered at any block (only 1 request per block is allowed) by depositing and withdrawing in the same block, without providing any settlement fee.

Thus, this mechanism can be exploited to give maximum profit to the keeper, which would force the protocol to manually add fees to the oracle or be DoSed (could be both).

Impact

Theft of yield and protocol funds by abusing the keeper role and DoS of the Market.

Code Snippet

Added the following test to Market.test.ts, proving that the request can be triggered without paying any fees. The attacker would then proceed to commit a price to the oracle and get the fees (possible at every block), until there are no more fees in the Market.

it.only('POC opens and closes the position to trigger an oracle request without paying any fee', async () => { 
  const dustCollateral = parse6decimal("100");
  const dustPosition = parse6decimal ("0.000001");
  dsu.transferFrom.whenCalledWith(user.address, market.address, dustCollateral.mul(1e12)).returns(true)

  await expect(market.connect(user).update(user.address, dustPosition, 0, 0, dustCollateral, false))
    .to.emit(market, 'Updated')
    .withArgs(user.address, ORACLE_VERSION_2.timestamp, dustPosition, 0, 0, dustCollateral, false)

  expectLocalEq(await market.locals(user.address), {
    currentId: 1,
    latestId: 0,
    collateral: dustCollateral,
    reward: 0,
    protection: 0,
  })
  expectPositionEq(await market.positions(user.address), {
    ...DEFAULT_POSITION,
    timestamp: ORACLE_VERSION_1.timestamp,
  })
  expectPositionEq(await market.pendingPositions(user.address, 1), {
    ...DEFAULT_POSITION,
    timestamp: ORACLE_VERSION_2.timestamp,
    maker: dustPosition,
    delta: dustCollateral,
  })
  expectGlobalEq(await market.global(), {
    currentId: 1,
    latestId: 0,
    protocolFee: 0,
    oracleFee: 0,
    riskFee: 0,
    donation: 0,
  })
  expectPositionEq(await market.position(), {
    ...DEFAULT_POSITION,
    timestamp: ORACLE_VERSION_1.timestamp,
  })
  expectPositionEq(await market.pendingPosition(1), {
    ...DEFAULT_POSITION,
    timestamp: ORACLE_VERSION_2.timestamp,
    maker: dustPosition,
  })
  expectVersionEq(await market.versions(ORACLE_VERSION_1.timestamp), {
    makerValue: { _value: 0 },
    longValue: { _value: 0 },
    shortValue: { _value: 0 },
    makerReward: { _value: 0 },
    longReward: { _value: 0 },
    shortReward: { _value: 0 },
  })

  dsu.transfer.whenCalledWith(user.address, dustCollateral.mul(1e12)).returns(true)
  await expect(market.connect(user).update(user.address, 0, 0, 0, dustCollateral.mul(-1), false))
    .to.emit(market, 'Updated')
    .withArgs(user.address, ORACLE_VERSION_2.timestamp, 0, 0, 0, dustCollateral.mul(-1), false)

    expect(oracle.request).to.have.been.calledWith(user.address)
})

Tool used

Vscode, Hardhat, Manual Review

Recommendation

Whitelist the keeper role to prevent malicious users from figuring out ways to profit from the incentive mechanism. Additionally, the whitelisted keppers could skip oracle requests if they don't contribute to settlements (when there are no orders to settle), to ensure that funds are always available.

sherlock-admin commented 1 year ago

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

141345 commented:

x

panprog commented:

invalid because deposit will add pending keeper fees and minCollateral will ensure that at least minCollateral can not be withdrawn before position is closed

0x73696d616f commented 12 months ago

Escalate As long as there are no new positions to settle and the oracle version/timestamp is not updated, it's possible to keep opening and closing positions at each block, triggering consecutive requests, without paying any fees to the protocol.

Then, after a few requests are triggered or someone else either opens a position or commits a new price, the attacker can commit all the previous requests. The only requirement is that the publish time of the requests are within the MIN_VALID_TIME_AFTER_VERSION and MAX_VALID_TIME_AFTER_VERSION window.

This would pay the attacker the incentives and the profit depends on the specific math (considering the gas fees). Additionally, over a long enough period, it could drain the protocol of keeper fees, which would lead to DoS due to lack of fees to commit new prices.

I tweaked the POC a bit to show how several requests can be made consecutively without paying any fees to the protocol. The positions are opened and closed in the loop (incrementing the block.timestamp at each iteration), triggering several requests. The next step would be to commit these requests in the oracle, but that should not require a POC.

it.only('POC opens and closes the position to trigger an oracle request without paying any fee', async () => { 
  const dustCollateral = parse6decimal("100");
  const dustPosition = parse6decimal ("0.000001");

  for (let i = 0; i < 5; i++) {
    dsu.transferFrom.whenCalledWith(user.address, market.address, dustCollateral.mul(1e12)).returns(true)

    await expect(market.connect(user).update(user.address, dustPosition, 0, 0, dustCollateral, false))
      .to.emit(market, 'Updated')
      .withArgs(user.address, ORACLE_VERSION_2.timestamp, dustPosition, 0, 0, dustCollateral, false)

    expectLocalEq(await market.locals(user.address), {
      currentId: 1,
      latestId: 0,
      collateral: dustCollateral,
      reward: 0,
      protection: 0,
    })
    expectPositionEq(await market.positions(user.address), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_1.timestamp,
    })
    expectPositionEq(await market.pendingPositions(user.address, 1), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_2.timestamp,
      maker: dustPosition,
      delta: dustCollateral,
    })
    expectGlobalEq(await market.global(), {
      currentId: 1,
      latestId: 0,
      protocolFee: 0,
      oracleFee: 0,
      riskFee: 0,
      donation: 0,
    })
    expectPositionEq(await market.position(), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_1.timestamp,
    })
    expectPositionEq(await market.pendingPosition(1), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_2.timestamp,
      maker: dustPosition,
    })
    expectVersionEq(await market.versions(ORACLE_VERSION_1.timestamp), {
      makerValue: { _value: 0 },
      longValue: { _value: 0 },
      shortValue: { _value: 0 },
      makerReward: { _value: 0 },
      longReward: { _value: 0 },
      shortReward: { _value: 0 },
    })

    dsu.transfer.whenCalledWith(user.address, dustCollateral.mul(1e12)).returns(true)
    await expect(market.connect(user).update(user.address, 0, 0, 0, dustCollateral.mul(-1), false))
      .to.emit(market, 'Updated')
      .withArgs(user.address, ORACLE_VERSION_2.timestamp, 0, 0, 0, dustCollateral.mul(-1), false)

      expect(oracle.request).to.have.been.calledWith(user.address)

    await time.increase(1)
  }
})

sherlock-admin2 commented 12 months ago

Escalate As long as there are no new positions to settle and the oracle version/timestamp is not updated, it's possible to keep opening and closing positions at each block, triggering consecutive requests, without paying any fees to the protocol.

Then, after a few requests are triggered or someone else either opens a position or commits a new price, the attacker can commit all the previous requests. The only requirement is that the publish time of the requests are within the MIN_VALID_TIME_AFTER_VERSION and MAX_VALID_TIME_AFTER_VERSION window.

This would pay the attacker the incentives and the profit depends on the specific math (considering the gas fees). Additionally, over a long enough period, it could drain the protocol of keeper fees, which would lead to DoS due to lack of fees to commit new prices.

I tweaked the POC a bit to show how several requests can be made consecutively without paying any fees to the protocol. The positions are opened and closed in the loop (incrementing the block.timestamp at each iteration), triggering several requests. The next step would be to commit these requests in the oracle, but that should not require a POC.
it.only('POC opens and closes the position to trigger an oracle request without paying any fee', async () => { 
  const dustCollateral = parse6decimal("100");
  const dustPosition = parse6decimal ("0.000001");

  for (let i = 0; i < 5; i++) {
    dsu.transferFrom.whenCalledWith(user.address, market.address, dustCollateral.mul(1e12)).returns(true)

    await expect(market.connect(user).update(user.address, dustPosition, 0, 0, dustCollateral, false))
      .to.emit(market, 'Updated')
      .withArgs(user.address, ORACLE_VERSION_2.timestamp, dustPosition, 0, 0, dustCollateral, false)

    expectLocalEq(await market.locals(user.address), {
      currentId: 1,
      latestId: 0,
      collateral: dustCollateral,
      reward: 0,
      protection: 0,
    })
    expectPositionEq(await market.positions(user.address), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_1.timestamp,
    })
    expectPositionEq(await market.pendingPositions(user.address, 1), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_2.timestamp,
      maker: dustPosition,
      delta: dustCollateral,
    })
    expectGlobalEq(await market.global(), {
      currentId: 1,
      latestId: 0,
      protocolFee: 0,
      oracleFee: 0,
      riskFee: 0,
      donation: 0,
    })
    expectPositionEq(await market.position(), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_1.timestamp,
    })
    expectPositionEq(await market.pendingPosition(1), {
      ...DEFAULT_POSITION,
      timestamp: ORACLE_VERSION_2.timestamp,
      maker: dustPosition,
    })
    expectVersionEq(await market.versions(ORACLE_VERSION_1.timestamp), {
      makerValue: { _value: 0 },
      longValue: { _value: 0 },
      shortValue: { _value: 0 },
      makerReward: { _value: 0 },
      longReward: { _value: 0 },
      shortReward: { _value: 0 },
    })

    dsu.transfer.whenCalledWith(user.address, dustCollateral.mul(1e12)).returns(true)
    await expect(market.connect(user).update(user.address, 0, 0, 0, dustCollateral.mul(-1), false))
      .to.emit(market, 'Updated')
      .withArgs(user.address, ORACLE_VERSION_2.timestamp, 0, 0, 0, dustCollateral.mul(-1), false)

      expect(oracle.request).to.have.been.calledWith(user.address)

    await time.increase(1)
  }
})

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

arjun-io commented 12 months ago

This is a great find - while the keeper and position fees are correctly accounted for in most cases, this single version open and close does not correctly debit these fees and require the collateral balance to be higher than the fee amount. Thank you for the thorough test case as well. We will fix this

nevillehuang commented 12 months ago

Escalate

Confirmed by sponsor above, fees are not correctly debited for single version open and close.

sherlock-admin2 commented 12 months ago

Escalate

Confirmed by sponsor above, fees are not correctly debited for single version open and close.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

141345 commented 11 months ago

The severity of medium seems more appropriate.

Because according to sherlock's HM criteria:

Medium: viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future.

High: This vulnerability would result in a material loss of funds, and the cost of the attack is low.

Here:

The loss is on oracle fee.
Each time the loss is capped by one time keeper fee, not significant compared to the trading volume.
And there is cost to fetch valid price data for the attacker.

In summary, the loss is limited with cost, medium might be suffice.

arjun-io commented 11 months ago

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