Keepers can lose compensation fee

Summary

Vulnerability Detail

When the keeper fulfills the order, they receive a compensation fee from the order owner. The user specifies a maxFee in the order. The _handleKeeperFee function calculates the fee for the compensation, and the keeper receives the lesser of the calculated fee or the maxFee set by the user.

function _raiseKeeperFee(
        UFixed18 amount,
        bytes memory data
    ) internal virtual override returns (UFixed18) {
        (IMarket market, address account, UFixed6 maxFee) = abi.decode(data, (IMarket, address, UFixed6));
        UFixed6 raisedKeeperFee = UFixed6Lib.from(amount, true).min(maxFee);

        _marketWithdraw(market, account, raisedKeeperFee);

        return UFixed18Lib.from(raisedKeeperFee);
    }

The problem is that the user can front-run the keepers' tx and change the maxFee to 0 to grief the keeper.

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial-order/contracts/Manager.sol#L76-L78

function placeOrder(IMarket market, uint256 orderId, TriggerOrder calldata order) external {
        _placeOrder(market, msg.sender, orderId, order);

    }

The user can call the placeOrder function using the same orderId as before and modify the order with a lower maxFee amount.

Impact

The keeper can lose a fee.

Code Snippet

POC:

Manager_Arbitrum.ts

it('test maxFee', async () => {
      // userA places a 5k maker order
      // maxFee is 0.88e18
      const orderId = await placeOrder(userA, Side.MAKER, Compare.LTE, parse6decimal('3993.6'), parse6decimal('55'))
      expect(orderId).to.equal(BigNumber.from(501))

      const order = {
        side: Side.MAKER,
        comparison: Compare.LTE,
        price: parse6decimal('3993.6'),
        delta: parse6decimal('55'),
        maxFee: utils.parseEther('0'),
        isSpent: false,
        referrer: constants.AddressZero,
        ...NO_INTERFACE_FEE,
      }
      //before the keepers tx, userA changes the maxFee
      await expect(manager.connect(userA).placeOrder(market.address, orderId, order, TX_OVERRIDES))

      await commitPrice(parse6decimal('2800'))

      await executeOrder(userA, 501)
    })

Tool used

Manual Review

Recommendation

Add a minimum fee parameter to the executeOrder function to ensure that the compensation fee is not less than what keepers want.

sherlock-audit / 2024-08-perennial-v2-update-3-judging

Nyx - Keepers can lose compensation fee #79