Liquidators might mistakenly transfer additional fund to users when liquidating

Summary

Liquidators might mistakenly transfer additional fund to users when liquidating.

Vulnerability Detail

Because liquidation logic does not check if collateral amount is bigger than 0, liquidators who mistakenly input positive value of liquidation fee via update() function will end up transferring their token to liquidated balance instead of claiming token as liquidation fee. Consider this scenario:

An user trusts this protocol, so they decide to approve max on erc20 so that thet could make an update on position fast and more efficient without having to approve transfer for every update.
They see a liquidation opportunity and decide to go for it. However , they mistakenly input a positive value of liquidation fee.
Now they transfer that value of token to liquidated account instead of claiming liquidation fee.
Impact

Liquidators will lose fund if they input positive amount of liquidation fee.

Code Snippet

My test (add it under 'long' test suite in Market.test.ts)

it('myTest', async () => {
              oracle.at.whenCalledWith(ORACLE_VERSION_2.timestamp).returns(ORACLE_VERSION_2)
              oracle.status.returns([ORACLE_VERSION_2, ORACLE_VERSION_3.timestamp])
              oracle.request.returns()

              await settle(market, user)
              await settle(market, userB)

              const EXPECTED_PNL = parse6decimal('27').mul(5)
              const EXPECTED_LIQUIDATION_FEE = parse6decimal('2000')

              const oracleVersionLowerPrice = {
                price: parse6decimal('96'),
                timestamp: TIMESTAMP + 7200,
                valid: true,
              }
              oracle.at.whenCalledWith(oracleVersionLowerPrice.timestamp).returns(oracleVersionLowerPrice)
              oracle.status.returns([oracleVersionLowerPrice, ORACLE_VERSION_4.timestamp])
              oracle.request.returns()

              await settle(market, userB)
              dsu.transferFrom.whenCalledWith(liquidator.address, market.address, utils.parseEther('2000')).returns(true)
              dsu.transfer.whenCalledWith(liquidator.address, EXPECTED_LIQUIDATION_FEE.mul(1e12)).returns(true)
              dsu.balanceOf.whenCalledWith(market.address).returns(COLLATERAL.mul(1e12))

              await expect(
                market.connect(liquidator).update(user.address, 0 ,0, 0, EXPECTED_LIQUIDATION_FEE, true),
              )
                .to.emit(market, 'Updated')
                .withArgs(user.address, ORACLE_VERSION_4.timestamp, 0, 0, 0, EXPECTED_LIQUIDATION_FEE, true)

              oracle.at.whenCalledWith(ORACLE_VERSION_4.timestamp).returns(ORACLE_VERSION_4)
              oracle.status.returns([ORACLE_VERSION_4, ORACLE_VERSION_5.timestamp])
              oracle.request.returns()

              await settle(market, user)
              await settle(market, userB)

              const oracleVersionLowerPrice2 = {
                price: parse6decimal('96'),
                timestamp: TIMESTAMP + 14400,
                valid: true,
              }
              oracle.at.whenCalledWith(oracleVersionLowerPrice2.timestamp).returns(oracleVersionLowerPrice2)
              oracle.status.returns([oracleVersionLowerPrice2, oracleVersionLowerPrice2.timestamp + 3600])
              oracle.request.returns()

              await settle(market, user)
              await settle(market, userB)

            })

Result: already initialized

update

    long position
      liquidation
        long
          ✔ myTest (8261ms)

1 passing (12s)

Tool used

Manual Review

Recommendation

The liquidation logic should revert if liquidation fee's value is bigger than 0

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

 if (protected && (
            !collateral <=0 ||
            !context.currentPosition.local.magnitude().isZero() ||
            context.latestPosition.local.collateralized(
                context.latestVersion,
                context.riskParameter,
                collateralAfterFees.sub(collateral)
            ) ||
            collateral.lt(Fixed6Lib.from(-1, _liquidationFee(context)))
        )) revert MarketInvalidProtectionError();

sherlock-audit / 2023-07-perennial-judging

tvdung94 - Liquidators might mistakenly transfer additional fund to users when liquidating #118