Hook deployment is vulnerable to reorg situations

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/HooksFactory.sol#L317-L319

Vulnerability details

Impact

Hook deployment is not protected from reorg situations, even though protocol plans on deployment to chains that are very vulnerable to block reorgs. This will cause markets to be deployed with the wrong hooks, messing up users' potential hook configurations.

Proof of Concept

The protocol plans to deploy on Ethereum, Base, Arbitrum, Polygon. Re-orgs can happen in all EVM chains. Not super common on Ethereum, but still happens. The same also occurs on Poylgon, which has experienced large re-orgs in the past. and on Optimistic and Arbitrum rollups.

_deployHooksInstance uses the ordinary "create" to deploy the hooks,

      let initCodeSizeWithArgs := add(add(initCodeSize, 0x60), constructorArgsSize)
      // Deploy the contract with the initcode
      hooksInstance := create(0, initCodePointer, initCodeSizeWithArgs)

therefore during multiple hook/market deployments through the deployMarketAndHooks and deployMarket functions, a reorg situation will lead to a "swap" of address instances, in which the reorganization of chain blocks will lead to a different address instance being attached to another hook, other than the one it would have gotten in case of a normal deployment. In short, hook deployment A gets address instance B, rather than address instance A that it would have gotten in case of a normal deployment.
in case of multiple deployments with the deployMarketAndHooks function will cause the wrong hooksinstance to be attached to the wrong markets, which could tamper with various expected market configurations and lead to unexpected behaviour. Also, if the deployer uses the initially derived address and sends funds to the address, the switch in address instance will make the funds open to the other deployment, leading to loss of funds.

Tools Used

Manual code review

Recommended Mitigation Steps

Recommend using CREATE2 instead which uses salt that contains msg.sender to deploy hooks instead.

Assessed type

Other

[d1ll0n]

It's vanishingly unlikely this would happen in practice, but you're right that this could possibly be an issue.

[3docSec]

in case of multiple deployments with the deployMarketAndHooks function will cause the wrong hooksinstance to be attached to the wrong markets

I don't think this is correct. The function deployMarketAndHooks is atomic: it creates a hook instance, and a market instance with the created address.

If user A and B both call deployMarketAndHooks and a reorg changes the order A, B into B, A, you'll still have a consistent result, where the market configured by user A has the very same hooks user A they wanted, just a different address, but market and hooks will be linked consistently.

What is at risk is in my opinion only when the user then transfers funds to the hooks immediately afterwards. I do not understand under which circumstances one (the borrower even) may want to transfer funds to the hooks contract, I find the described "loss of funds" impact not realistic.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[laurenceday]

Fixed by wildcat-finance/v2-protocol@1a52863