# Wildcat V2 Security Review

This document reports the results of a security review of the Wildcat V2 smart contracts. These contracts are both an extension and modification of the wildcat v1 contracts which I also reviewed. The review started on the commit hash f30ea6d and took place over two weeks. Because the contracts have made various modification to the previous core contracts all code is in scope except some libraries which were forked from external versions.

### Findings

Summary: Critical: 0, High: 0, Medium: 2, Low: 2

Findings are presented chronologically:

* Fee updates in the hook template can be used to extract funds from deployers:
The deploy market function in the hooks factory uses a stored fee and the user does not indicate in their message the expected fee rate or expected fee currency. Therefore if the contract is approved by the user for an unlimited amount or in multiple currencies, and the user signs a transaction intending to pay x of asset A, the archcontroler's owner can send a transaction before the users transaction finalizes changing the fee to y of asset B. This can occur either via targeted MEV for large impact or on accident. Due to the necessity of a highly permissioned key and unlimited approvals, the likelihood is low this would be exploited.
* Severity Estimate: Low [Likelihood: Low, Impact: Medium]
* Remediation Recommendation: Label versions of the template and have the user indicate the version they intend to use.
* Use of `_tryValidateAccessInner` instead of `_tryValidateAccess` in the `onTransfer` access control:
The contract allows anyone to call `onTransfer` which uses `_tryValidateAccessInner` but then does not update the state of the `to` account regardless of if it should be updated. Because the inner function makes a possibly state changing call to a role provider anyone can make a state change for any user in the provider without making the corresponding state change in the access control function. For example if an access control provider uses state to prevent signature or zk access proof replay, then Alice can grief Bob by copying his signature from the mempool and calling `onTransfer` to exhaust it blocking Bob's reauthorization call.
* Severity Estimate: Medium [Likelihood: Medium, Impact: Medium]
* Remediation Recommendation: Use `_tryValidateAccess` instead.
* Borrowers can intentionally cause reverts in access control hooks to block the withdraws of users who were transfered debt tokens:
The `onTransfer`, `onDeposit`, and `onQueueWithdrawal` each call `_tryValidateAccess` which failing to find a current authorization from a user will then loop over all of the providers of authorization labeled as `pushProviders`. These calls are protected such that reverts do not bubble up, as reverting in the hook will block the use of `transfer`, `deposit`, and `queueWithdraw` functions in the market. However the borrower can add or remove access control providers, and therefore by removing all current access control providers and either adding so many providers that the loop cannot complete or adding providers which consume a large amount of gas but fail, the borrower can force `onTransfer` and `onDeposit` to revert with out of gas inside of the call, which therefor blocks the execution of these functions. In`onQueueWithdrawal` the withdraw is allowed if (1) the user has previously deposited or (2) the `_tryValidateAccess` call succeeds, since solidity does lazy evaluation it won't execute `_tryValidateAccess` for users who have deposited preventing forced out of gas reverts. However, many users may have or are expected to receive debt tokens via transfer which does not bypass `_tryValidateAccess`. Therefore a malicious borrower in a market where most debt tokens in secondary market hand can use the intentional out of gas attack to avoid paying delinquency fees while preventing all withdraws (including those there may be liquidity to process).
* Severity Estimate: Medium [Likelihood: Medium, Impact: Medium]
* Remediation Recommendation: Allow users who receive debt tokens via transfer to queueWithdraws, return as failed if the next call in the loop has not enough gas to succeed, and limit push provider array length.
* Interest rates cause the min deposit to increase contrary to intended behavior:
The on deposit hook gets `scaledAmount` and not the normalize amount, therefore as interest accumulates the scaled amount decreases. If there is a hook control to only allow a minimum deposit based on a constant minimum, as is indicated as the intent by the team, then this will not produce a constant minimum and instead it will go up over time as interest accumulates. If a mechanism were introduced which allowed manipulating the share rate (such as paying bonus interest) it's possible this could require min deposits greater than max deposit and brick the contract [which is not considered as part of the impact as no such code is identified].
* Severity Estimate: Low [Likelihood: Medium, Impact: Low]
* Remediation Recommendation: Enforce minimum deposit rules on the asset itself not on the scaled asset amount

### Notes

Very low severity, performance notes, or other non-issue comments

* It's theoretically possible to use a very large computational power to compute a collision between an EOA and the create2 deployment which would allow a lender to set allowances which allow them to directly transfer from the pools bypassing all security controls. This is not a serious concern but is technically possible.
* It is best practice to reset the free memory pointer at the end of `emit_SanctionedAccountAssetsQueuedForWithdrawal`, currently this memory corruption has no effect because the emission is right before the end of the function, but it is a rough edge which can easily cause errors on small refactors.