# Wildcat Review

The Wildcat team has engaged me for a two week security review of their trusted lending protocol. Because I am early in their dev cycle they have given me a restricted audit scope covering just: src/libraries/\*, src/market/\*, src/ReentrancyGuard.sol, this scope covers a lending vault but not the overall permissioning contracts or the sanctions escrow factory and sanctions escrow. The inscope lines of code are roughly 1k

The trust model of this codebase assumes that the users are known parties which should have legal agreements with the developer and with eachother. However the codebase review does not adjust the severity on this basis as it concerned with the code itself and so is applicable to non legally bound parties use of the system. Moreover in our treatment of the un-developed parts of the system I take a pessimistic view of their structure as it cannot be verified in code.

The results are: 1 critical, 2 high, 4 medium, 2 low, and various notes.

## Issue Log

Note the following is sorted chronologically based on discovery timeline.

* closeMarket allows zero interest borrowing: The close market function does not block a followup borrow by the borrower but also does not change the required liquidity as would normally happen when lowering the rate. If the borrower calls this then borrows they borrow up to the limit with zero interest and will only be sent into delinquency by pending withdraws.
* Severity Estimate: High [Impact: Medium, Likelihood: High]
* Remediation Recommendation: Stop borrows from closed markets.
* Status: Resolved
* Unintentional funds transfer when processing a withdraw from a sanctioned address: The market contract tries to ensure that no funds are paid directly to a sanctioned address, as verified by an onchain watcher contract, and so has a case where a sanctioned user has a processed withdraw. The code's intention is to create an escrow for the user and then pay that escrow but due to a typo it pays the user instead. This occurs only for users which are sanctioned while a withdraw is processing but not fully executed, and the sanctioned user retains custody of the funds.
* Severity Estimate: High [Impact: Medium, Likelihood: High]
* Remediation Recommendation: Fix the typo
* Status: Resolved
* Unintentional account status change via '\_getAccountWithRole': If an account is set to WithdrawOnly on the vault smart contract by calling with the controller but the controller contract is called to deauthorize the user first (or at any time after) the user can undo this by calling 'depositUpTo' which calls '\_getAccountWithRole'. This function will compare the user's status and the on failure call the controller which if the user is still authorized will report that to the vault and then the vault will set the user's status to DepositAndWithdraw. While this requires some mismanagement from the borrower it's not that unlikely as you can trigger it by calling the two functions the wrong order and having a user front run you.
* Severity Estimate: Low [Impact: Low, Likelihood: Medium]
* Remediation Recommendation: Instead of checking for failure in the conditional check if the user has the Null status. This lets you preserve the property that users don't have to be approved per vault but prevents this behavior from changing future statuses
* Status: Resolved
* Protocol fees can be bypassed while still paying the same rate to the lenders: The overall interest paid by the borrower is the interest times the protocol fee plus any delinquency fees. By setting the delinquency fee percent to the intended interest and then setting the protocol interest percent to zero (or close to zero) the borrower can construct a vault which pays the lenders the same amount of interest via delinquency fees as it would pay via interest. The borrower can increase the liquidity requirements to force payment of persistent delinquency fees. In this case the lenders have no incentive to withdraw as they get the same rate and borrowers pay a net lower fee as no protocol fee is collected.
* Severity Estimate: Medium [Impact: Medium, Likelihood: Medium]
* Remediation Recommendation: Charge protocol fees on the extra interest which is paid by delinquent loans.
* Status: Open
* Permanent Funds Locking by \_processExpiredWithdrawalBatch: The \_processExpiredWithdrawalBatch is called when a withdraw batch is expiring and if it is not fully paid it will pay out the batch. In this function, and the codebase more generally, objects are acted on in memory then written at the end of transactions. However in this function's case it sets the stored memory reference for `state.pendingWithdrawalExpiry` to zero then does `\_withdrawalData.batches[state.pendingWithdrawalExpiry] = batch;` which stores the mutated batch into zero instead of into the intended expiry. The memory state variable is still stored as normal in the end of the transaction, meaning the state storage object is out of sync with the batches memory object. This causes loss of funds as the state reserves funds from that payout for withdraw but user's cannot withdraw from the paid batch in zero as they do not have any values in the `accountStatuses` mapping for expiry zero. In particular if:
1) A withdraw is made for more than currently available funds then a batch will be partially paid with a delta value remaining
2) The borrower direct transfers delta funds into the contract
3) The batch is paid out by the \_processExpiredWithdrawalBatch
4) Then the delta is added to reserved funds in state but cannot be withdrawn and the delta funds are locked and are unwithdrawable by all parties.
* Severity Estimate: Critical [Impact: High, Likelihood: High]
* Remediation Recommendation: Cache the expiry value and write the batch memory object to that storage location instead of zero
* Status: Resolved
* Permanent Funds locking via controlled sentinel: the sanctions sentinel is controlled by an EOA which updates onchain sanctions lists. An admin can use control over this to blocklist the create2 predicted user escrow, and if they then blocklist the user account the funds are permanently locked and even sanctions list removal isn't able to remove them. Included as a note here as the admins are trying to prevent user funds custody relying on them, and this lets them permalock funds.
* Severity Estimate: Low [Impact: High, Likelihood: Low]
* Remediation Recommendation: Set escrow account to unblocked on creation of escrow. Or add an escrow role which cannot be blocked
* Status: Resolved
* Incorrect not implementation in the BoolUtils: The BoolUtils library has a function 'not(x)' which intends to return false if x is true and vice versa. However because solidity treats all bools as u8 and the yul not(x) function is a binary not if you provide this function with a solidity generated bool 0x01 for example you get back 0xfe and when solidity interprets this in the future it will treat that 0xfe as true (because it is not 0x00). So the not function will return what solidity considers to be a true when called on a true. This library is not used elsewhere in the codebase, but we strongly recommend removing or fixing the function to prevent future bugs.
* Severity Estimate: Note [Impact: High, Likelihood: Code Unused]
* Remediation Recommendation: Delete the incorrect code, or use 'isZero' if the code is intended to be used in the future.
* Status: Resolved
* Reentrancy drain backdoor in `nukeAccount`: The nukeAccount function can be used to drain the contract by either an admin of the blacklisting system or by a toxic vault owner. In particular the sentinel address can reenter on the `createEscrow` call in blockAccount and can also return an admin controlled address from this call. In this attack a re-entrant call is made before the escrow account is credited with the balance of the user account. This works because the 'account.scaledBalance = 0;' before the re-entrant call changes the account balance only in memory which is reset in the external call scope. The real storage balance of the account is only changed at the very end of the call, after the possible re-entrant call. Because of the implicit permissioning required by the Wildcat system we consider this attack quite unlikely.
* Severity Estimate: Medium [Impact: High, Likelihood: Low]
* Remediation Recommendation: Add a re-entrancy guard modifier to the nuke function.
* Note - Even if the attack here is considered unlikely I would still recommend the remediation as other malicious byproducts could be produced by this re-entrancy.
* Status: Resolved
* `setLiquidityCoverageRatio()` can make a borrower delinquent: The setLiquidityCoverageRatio function has a check designed to ensure that a borrower is non-delinquent but it only does so before the state memory object is altered and so the borrower can become delinquent under the raised ratio. Based on the information provided by the team (but not validated in this report) it is intended that this function is called by a controller function which ensures that if a borrower lowers interest rates that they must have a 90% liquidity ratio for a period to ensure people can withdraw. However this function does not force the borrower to pay 90% of the loan back they can set the loan rate to a low interest rate and users will have to depend on them redepositing to withdraw. Alternatively if the users do not notice the borrower can only repay for the period directly before lowering the liquidity ratio, and the borrower can cycle raise, repay, lower, to get a low interest rate for periods of half the loan, even with the intended controller system.
* Severity Estimate: Medium [Impact: Medium, Likelihood: Medium]
* Remediation Recommendation: Add checks that the user is not using the function and entering delinquency as a result,
* Status: Resolved

Notes:

* https://github.com/wildcat-finance/wildcat-protocol/blob/ce7d832f257c1ff5425acdd55034b7a47586043b/src/libraries/MathUtils.sol#L172C21-L172C27 The usage of isZero(isZero(gt(x,y))) gives the same result as gt(x, y)
* In under used markets the interest rate requirements can make loans undercollateralized and because each contract is unique and will have a limited number of approved deposit or withdrawers it's likely each contract has only a small number of interactions over the loan duration. Combined this means it's likely that a borrower could use a front running strategy to preempt delinquency triggers and spend large portions of the loan delinquent. Because interest is linear the project could check if the user is delinquent at the start of a funding txn and then extrapolate to when the interest triggered delinquency, ensuring those periods are charged the proper fees.
* Since when a pending withdraw batch is first executed it is executed using the expiry timestamp to set the interest rates, if a user is the last withdraw they will not be paid interest on any period where their funds are held after their withdraw batch expiry. Since there is not a known locking bug in the executeWithdrawal function, the user can call this at any time to trigger delinquency if the borrower has not paid.
* Lender interest rates are compounded based on the transaction frequency and therefore have a strong dependency on the number of transactions, the differential between per block compounding (aprox every 12 seconds) vs weekly or monthly can be large over the period. In addition when there is a completed batch the interest from the period between the last call and expiry will be overcharged by the amount of the interest accumulated after the block is completed because in this case the interest accumulation function is called twice once up to expiry and once after.