QA Report

[howlbot-integration[bot]]

See the markdown file with the details of this report here.

[laurenceday]

Thank you for the effort involved in putting together this report. There's only one change we'll make here, a docs update based on number 6. The rest is either expected behaviour or things we aren't interested in fixing.

1. This is expected behaviour: we don't envisage anyone ever actually creating a market with a 100% reserve ratio, but at the same time it's similarly unlikely that anyone would create one with 90%, so the boundary is somewhat illusory. If someone did do the former, they'd be expected to immediately transfer some assets in upon first deposit to account for protocol fees anyway (indeed, they'd be required to): perhaps if a borrower wanted a vanity wrapped token at 0% APR, for example.

2. We're not too fussed about this taking place (it's extremely unlikely that feeRecipient would get blocked by stablecoin issuers/WETH/cbBTC), and if someone created a market for a memecoin that blocked the address, that's just too bad for Wildcat really. One would anticipate that the market wouldn't react well to a token that made a decision like that in any event, so it's not as if the protocol would likely stand to lose much by way of revenue. We want to keep feeRecipient immutable, and a pull pattern would need a whitelisted set of addresses anyway that we don't want to have update power for.

3. Rebasing tokens are explicitly mentioned in the audit repo and whitepaper for V1 as breaking the underlying interest model, and should not be used as underlying assets (Wildcat is likely to blacklist a few of the most popular ones just to hammer the point home).

4. Pardon? That's not how this works - approving someone for 100 market tokens isn't going to let someone transfer 101 market tokens down the line, the approval mapping stays static. The 'scaling being done' that you observe is literally just tracking how much of the underlying asset is now held, in exactly the same way as balanceOf works for standard ERC-20s.

5. The impact on frequent updating is not nearly as severe as you might imagine - we are well aware of this, however: it is a design choice.

6. We'll update the docs in time, cheers.

7. It is extremely unlikely that an external protocol would want to integrate the result of a sanctioned escrow account being activated, but fair observation.

8. Expected behaviour that we don't wish to fix: the point here is to allow sentinels or borrowers to execute things to keep the withdrawal queue clear. We can't reasonably account for things like wallet upgrades. We can see a lender getting confused and angry that their funds aren't in their wallet because they forgot to execute a queued withdrawal themselves, and it'll just be easier for someone else to sweep things to them.

9. Not a concern.

10. Not a concern.

11. Burning a claim on credit is a mad thing to do (read: we don't expect this to happen, and if it does, good luck to the lender). Adding this check would add unreasonable gas bloat to every single transfer.

12. ERC-20s used as underlyings are expected to be well-formed/'standard'. Esoteric tokens such as this aren't a concern of ours.

13. We want this in place: it's our observed experience that capped vaults don't have people trying to push right up to the wei, and if they're interested in doing so they should just be using depositUpTo instead of deposit. Our UI handles this.

14. Incorrect. Sanctions are checked in the _getAccount(msg.sender) function.

15. Not expecting this to happen: if it does, the borrower can transfer in from another address using repay as needed. More generally, the potential to update the borrower address would be a significant attack vector, and as such we made the early decision to not enable this.

16. Market controller factories are deprecated in V2. Even if they weren't, the archcontroller owners would quite simply not be deploying MCFs that were owned by someone else except in the case of a mistake, and any borrower that tried to use them would find that they weren't registered with that new archcontroller address so couldn't deploy anyway.

17. This may be precisely what is desired in the event that a borrower is targeted by a hostile Chainalysis oracle, and it's not as if we can distinguish between a 'real' and a 'fake' sanction. Besides, a borrower wouldn't have any market tokens to send into a sentinel escrow contract anyway: they aren't using withdrawals as lenders do.

18. Sir, if (state.isClosed) revert_AprChangeOnClosedMarket(); is literally the second line of the function.

19. Not a concern: fees are read directly into the UI from a lens contract, and even if this happened, a borrower that disagreed would be free to terminate their market as soon as they noticed for minimal 'damage'.

20. Expected behaviour - each market is its' own token contract as well, and allowing the 'resetting' of these would be extremely dangerous.

21. No response needed.

22. No response needed: compilation speed is not a concern of ours for this.

23. Dawg, that's eighty years from now. I think we'll be fine. My grandchildren will not be inheriting a Wildcat market.

24. Expired credentials are still credentials, and this is used for withdrawal permissions. Unset credentials (i.e. explicitly rescinded) are set to 0. This is expected behaviour.

25. Not a concern.

26. Not a concern: we'll deploy on chains once PUSH0 is supported.

[c4-judge]

3docSec marked the issue as grade-a

[3docSec]

Please exclude from report: QA-04, QA-14, QA-18 All others can be included without changes.

[c4-judge]

3docSec marked the issue as selected for report

[liveactionllama]

For awarding purposes, C4 staff have marked as 1st place.

[liveactionllama]

In the official report, C4 will be excluding the 3 items referenced in the judge's comment here above.