c4-bot-9 commented 2 months ago

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#L170

Vulnerability details

Impact

Batch calls to pushProtocolFeeBipsUpdates() will unexpectedly revert, because the setProtocolFeeBips() function will revert whenever a market is closed. This can occur due to the following reasons:

By chance a user closes a market just before the tranasction to push the fee updates.
A user maliciosly deploys dummy markets and closes them in frontrunning transcations to grief the system and delay the fee updates.
A user accidentally or unknowingly includes a market that is closed in the range. This can happen because the behavior is not documented.

Proof of Concept

Whenever there is a protocol fee bips update in a template, this can be pushed to the corresponding markets. The function takes a range of markets to apply the update to and calls market.setProtocolFeeBips(protocolFeeBips) as described in the comments:

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

  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)
        }
      }
    }
  }

The called setProtocolFeeBips() function will revert if the market is closed:

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

  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(); // @audit here the function will revert.
    if (_protocolFeeBips == state.protocolFeeBips) revert_ProtocolFeeNotChanged();
    hooks.onSetProtocolFeeBips(_protocolFeeBips, state);
    state.protocolFeeBips = _protocolFeeBips;
    emit ProtocolFeeBipsUpdated(_protocolFeeBips);
    _writeState(state);
  }

As a result transactions will unexpectedly revert. Consider the following scenario:

There is a protocol fee bips update for a template
Admin starts pushing the change to the corresponding markets.
By chance a user closes a market just before the change push
The admin transaction reverts unexpectedly.

Recommended Mitigation Steps

Do not revert the tranasction but just return without changes and continue the iteration for the next market without.

Assessed type

Error

laurenceday commented 2 months ago

This is expected behaviour. No parameter of a market should be adjustable once a market is closed.

Once a market is closed the protocol fee stops accruing regardless of whatever level it was at prior to the market closure. Borrowers incur no more or less in fees than were already present, and lenders don't see the impact of a protocol fee shift at all since it's not borne by them. There is no risk at all to any users here.

The only people that would get irritated by this happening would be the protocol operators, wondering why their transaction reverted: and even then, it would be incredibly simple to grab a list of markets to push an update to and filter by !state.isClosed prior to execution.

c4-judge commented 1 month ago

3docSec marked the issue as unsatisfactory: Invalid

code-423n4 / 2024-08-wildcat-findings