Protocol fee rate can be arbitrarily modified by the owner and the new rate will apply to all existing orders

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L132-L146

Vulnerability details

function matchOneToOneOrders(
    OrderTypes.MakerOrder[] calldata makerOrders1,
    OrderTypes.MakerOrder[] calldata makerOrders2
  ) external {
    uint256 startGas = gasleft();
    uint256 numMakerOrders = makerOrders1.length;
    require(msg.sender == MATCH_EXECUTOR, 'OME');
    require(numMakerOrders == makerOrders2.length, 'mismatched lengths');

    // the below 3 variables are copied to memory once to save on gas
    // an SLOAD costs minimum 100 gas where an MLOAD only costs minimum 3 gas
    // since these values won't change during function execution, we can save on gas by copying them to memory once
    // instead of SLOADing once for each loop iteration
    uint16 protocolFeeBps = PROTOCOL_FEE_BPS;
    uint32 wethTransferGasUnits = WETH_TRANSFER_GAS_UNITS;

Per the comment:

Transfer fees. Fees are always transferred from buyer to the seller and the exchange although seller is the one that actually 'pays' the fees

And the code:

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725-L729

    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
    uint256 remainingAmount = execPrice - protocolFee;
    _transferMultipleNFTs(sell.signer, buy.signer, sell.nfts);
    // transfer final amount (post-fees) to seller
    IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, sell.signer, remainingAmount);

In the current design/implementation, the protocol fee is paid from the buyer's wallet, regardless of whether the buyer is the taker or the maker. And the protocol fee will be deducted from the execPrice, only the remainingAmount will be sent to the seller.

This is unconventional as if the buyer placed a limit order, say to sell 1 Punk for 100 ETH, it means that the seller is expected to receive 100 ETH. And now the seller must consider the fee rate and if they expect 100 ETH, the price must be set to 101 ETH.

While this is unconventional and a little inconvenience, it's still acceptable IF the protocol fee rate is fixed, OR the seller is the taker so that they can do the math and agrees to the protocol fee when they choose to fulfill the counterparty's maker order.

However, that's not always the case with the current implementation: the protocol can be changed, effective immediately, and applies to all existing orders.

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1265-L1269

    /// @dev updates exchange fees
  function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
    PROTOCOL_FEE_BPS = _protocolFeeBps;
    emit NewProtocolFee(_protocolFeeBps);
  }

Plus, when the protocol fee rate updated to a higher rate, say from 5% to 50%, an maker order placed before this fee rate update can be fulfilled by a buyer, while the buyer still pays the same amount, the seller (maker) will receive 45% less than the initial sell order amount.

Recommendation

1. Consider making the protocol fee rate a constant, ie, can not be changed;
2. Or, consider changing to the protocol fee always be paid by the taker; while matching the maker buy and maker sell orders, the protocol fee must be paid from the price difference between the buy price and sell price;
3. Or, consider changing to the new protocol fee only applies to the orders created after the rate updated.

[nneverlander]

This was a design decision. Initially we were fetching the protocol fee from the complication but decided not to make external contract calls for this to save on gas. The other option was to make the protocol fee a part of the maker order but that comes with its own attack surface. So we implemented a compromise: https://github.com/infinitydotxyz/exchange-contracts-v2/commit/793ee814d86030477470c81c4f6fda353967a42a

As such the severity of the bug can be classified as low since this assumes malicious intent on part of the protocol admin.

[nneverlander]

https://github.com/code-423n4/2022-06-infinity-findings/issues/180

[HardlyDifficult]

Maker sell orders are charged the fee set at the time an order is filled and not when the order was created.

I'm not sure that I agree this concern is limited to malicious intent. With the ability to change fee, it's safe to assume at some point the admin may choose to increase the fee. At that point, all outstanding maker sells are subject to a higher fee than expected. Some users may be more sensitive to this than others. The warden's recommendations seems to address that concern and the fix the sponsor posted mitigates it by setting a max fee that may apply.

I think this is a Medium risk issue - an unexpected bump in fee impacting users who interacted with the system previous to that change is a form of value leak.