Malicios users can grief admin calls to pushProtocolFeeBipsUpdates causing tranasctions to revert

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/HooksFactory.sol#L553
https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L171

Vulnerability details

Impact

The pushProtocolFeeBipsUpdates function is callable by anyone, meaning anyone can push a fee update. However, whenever a market has already been updated, the setProtocolFeeBips() function will revert when the update is pushed again. As a result malicios users can frontrun batch calls to pushProtocolFeeBipsUpdates() and push the update to only the last market in a batch. This will result in reverting tranasctions for admins or other honest users and thus loss of gas costs, as well as delayed updates. If there are 1000 deployed markets in a template, the admin will have to do 1000 separate transactions to avoid the issue, which is a big overhead. Note that this can happen by accident as well, if a user happens to push an update to their own fees just ahead of the admin.

Proof of Concept

The pushProtocolFeeBipsUpdates() function iterates over markets in the range [marketStartIndex, marketEndIndex] and pushes the fees update via calls to market.setProtocolFeeBips(protocolFeeBips):

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/HooksFactory.sol#L553

  /**
   * @dev Push any changes to the fee configuration of `hooksTemplate` to markets
   *      using any instances of that template at `_marketsByHooksTemplate[hooksTemplate]`.
   *      Starts at `marketStartIndex` and ends one before `marketEndIndex`  or markets.length,
   *      whichever is lowest.
   */
  function pushProtocolFeeBipsUpdates(
    address hooksTemplate,
    uint marketStartIndex,
    uint marketEndIndex
  ) public override nonReentrant {
    HooksTemplate memory details = _templateDetails[hooksTemplate];
    if (!details.exists) revert HooksTemplateNotFound();

    address[] storage markets = _marketsByHooksTemplate[hooksTemplate];
    marketEndIndex = MathUtils.min(marketEndIndex, markets.length);
    uint256 count = marketEndIndex - marketStartIndex;
    uint256 setProtocolFeeBipsCalldataPointer;
    uint16 protocolFeeBips = details.protocolFeeBips;
    assembly {
      // Write the calldata for `market.setProtocolFeeBips(protocolFeeBips)`
      // this will be reused for every market
      setProtocolFeeBipsCalldataPointer := mload(0x40)
      mstore(0x40, add(setProtocolFeeBipsCalldataPointer, 0x40))
      // Write selector for `setProtocolFeeBips(uint16)`
      mstore(setProtocolFeeBipsCalldataPointer, 0xae6ea191)
      mstore(add(setProtocolFeeBipsCalldataPointer, 0x20), protocolFeeBips)
      // Add 28 bytes to get the exact pointer to the first byte of the selector
      setProtocolFeeBipsCalldataPointer := add(setProtocolFeeBipsCalldataPointer, 0x1c)
    }
    for (uint256 i = 0; i < count; i++) {
      address market = markets[marketStartIndex + i];
      assembly {
        if iszero(call(gas(), market, 0, setProtocolFeeBipsCalldataPointer, 0x24, 0, 0)) {
          // Equivalent to `revert SetProtocolFeeBipsFailed()`
          mstore(0, 0x4484a4a9)
          revert(0x1c, 0x04)
        }
      }
    }
  }

However, the setProtocolFeeBips function will revert if the protocol fee bips remains the same, meaning the change was already pushed, so _protocolFeeBips == state.protocolFeeBips will be true:

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L171

  function setProtocolFeeBips(
    uint16 _protocolFeeBips
  ) external nonReentrant sphereXGuardExternal {
    if (msg.sender != factory) revert_NotFactory();
    if (_protocolFeeBips > 1_000) revert_ProtocolFeeTooHigh();
    MarketState memory state = _getUpdatedState();
    if (state.isClosed) revert_ProtocolFeeChangeOnClosedMarket();
    if (_protocolFeeBips == state.protocolFeeBips) revert_ProtocolFeeNotChanged(); // @audit here, the fee will remain the same, so the tx will revert.
    hooks.onSetProtocolFeeBips(_protocolFeeBips, state);
    state.protocolFeeBips = _protocolFeeBips;
    emit ProtocolFeeBipsUpdated(_protocolFeeBips);
    _writeState(state);
  }

Thus, a malicios user can frontrun batch calls and cause them to revert and incur gas losses. The malicios user would specifically push the update to the last market in a batch for maximum gas losses. This can also happen if for example a user pushes a fee update to their market just before the admin batch call.

Recommended Mitigation Steps

Do not revert, but just return if the fee bips is unchanged.

Assessed type

MEV

[d1ll0n]

The proposed attack seems pretty outlandish IMO, that someone would pay gas fees to frontrun these update transactions just for the sake of griefing the admins. Definitely a low not a medium but probably worth changing just in case.

[3docSec]

Thanks!
I consider this a valid attack vector. However, since the target is an admin call, and the impact is just delaying the admin call, we can't really say that the protocol functionality is disrupted to have fit with M severity. That's why I'm assessing this a valid L.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[gesha17]

Hello @3docSec ,

I believe this issue fits well into the medium severity criteria for the following reasons:

1. The attacker can cause actual loss of fees for either the protocol or users.

Consider this example: The protocol fee bips is increased by 1% accross the entire protocol. At $25m in loans(current tvl of wildcat is ~25m), this would make the protocol fee increase by 25000000*1/100/365 ~ 685 USD per day. Thus the attacker will cause a 685 USD loss(on average across loans) to the protocol for each day the fee update is not pushed. On the other hand if the fee is decreased by 1% the loss will be 685 USD of additional fees for the users for each day the fee is not updated. Either way the attacker will cause a loss for one party. A competing protocol would have the incentive to mess with this.

2. Users have an incentive to push the value themselves if the fee is decreased as it makes them pay less fees in total. This makes it more likely for the admin transactions to unexpectedly revert by chance.

Consider this example: A user has $2.5m in a loan. If the protocol fee bips is decreased by 1% it would mean the user is losing 2500000*1/100/365= 68.5 USD per day for each day the fee is not updated. As a result they would want to push the update as early as possible.

3. When the protocol fee is increased, users have an incentive to delay the update.

Borrowers want to pay as little fees as possible. As a result they would want to delay an update to the protocol fee bips if it is increased.

4. As per the competition page, the protocol will be deployed on Base, Arbitrum and Polygon as well.

On ethereum mainnet an attacker would have to bear the gas fees, but on the polygon chain, the attack would be practically free.

This issue causes the protocol function to be impacted and the proposed attack vector will cause loss of fees for either the protocol or users, so I think it fits well into the code4rena criteria for a medium severity issue.

[laurenceday]

Fixed by wildcat-finance/v2-protocol@4c7fc06

[3docSec]

Hi @gesha17
QA is still appropriate here, because:

• it's a griefing attack with impact that is at most availability
• the functionality with risk on availability is an admin function, not a user-facing protocol functionality
• admins can be considered sophisticated enough to easily circumvent frontrunning (if it ever happens) by at the very least submitting their updates through a private mempool