Description

The WooracleV2_2 contract contains a fallback onlyAdmin function that takes in compressed data, in order for the admin to be able to update prices without paying as much gas:

fallback (bytes calldata _input) external onlyAdmin returns (bytes memory _output) {
  /*
      2 bit:  0: post prices,
              1: post states,
              2: post prices with local timestamp
              3: post states with local timestamp
      6 bits: length

      post prices:
         [price] -->
            base token: 8 bites (1 byte)
            price data: 32 bits = (27, 5)
  [...]

However, it is possible that the automatically generated calldata to execute this fallback function happens to match the selector of an existing function. Since the caller is an admin, this can cause undesirable effects depending on the function that is inadvertently selected.

Impact

A price update that is intended to invoke the fallback function can instead accidentally call any other function, passing in garbage for calldata.

The probability of this happening is very low, but price updates are not random, and an attacker may be able to manipulate prices and volume on the open market to increase the chance of this happening and try to control the input calldata to the resulting call.

For example, if any incorrect price is posted to a real pair due to the call being interpreted as postPrice, this creates an arbitrage opportunity that is likely very large.

Recommendations

We recommend adding a new role for the bot calling the fallback function, instead of making it an admin, and then changing the fallback modifier to check for that role instead of onlyAdmin.

This would solve this issue because even if the call collides with any function selectors, since the bot is not an admin, that mistaken call will not be made with any higher privileges than a random EOA, so it will either do something the unprivileged attacker can do anyways more directly, or it will revert.

Remediation

Additionally, the WOOFI team has stated that:

It's intended to save the gas. Better not to add any more admin check or function selector. Also this contract will be called in high frequency. The probability of method conflicting is so low, and could be negligible. We have less than 30 functions in Wooracle contract, so collision probability is 30/(2^32) = 0.000000006984919; We typically update our Wooracle in 5 seconds, so a collision only happen once every 1000,000,000 seconds , that is 31 years: https://calculat.io/en/date/seconds/1000000000. From an engineering perspective: we utilize this zip fallback function to save calldata's gas consumption, so it's impossible to add another plain 4 bytes to only avoid collision. Even with collusion, our offline script can catch the tx failure and resend it again, it won't cause any disaster.