repay opearation can be effectively simulated bypassing state.isClosed and hooks

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarket.sol#L202-L215

Vulnerability details

Impact

It bypasses state.isClosed and nullifies any hook for repay operation. Also it breaks one of the invariants under All Markets.

Proof of Concept

According to the implementation of repay function, it should revert when either state.isClosed is true or hooks.onRepay fails.
However, user can effectively make the same effect bypassing above revert conditions by:

1. Transfer assets to the market
2. Call WildcatMarket.updateState()

This bypasses any reverting situation that should happen with state.isClosed or hook. Considering we'd have more hook templates that may utilize onRepay for some extended monitoring, it could arise as a bigger issue.

Furthermore, it breaks the invariant under All Markets:

Underlying assets transferred to a market outside of a deposit are treated as a payment by the borrower, i.e. they do not mint new market tokens or otherwise affect internal accounting other than by increasing totalAssets.

Tools Used

Manual Review

Recommended Mitigation Steps

Revise the repayment mechanism to ensure it is processed exclusively through designated market functions and strictly complies with any conditions that may cause it to revert.

Assessed type

Access Control

[laurenceday]

repay is written the way it is precisely because it acts as a one-transaction replacement for the transfer-and-update-state identified by the warden. In fact, it's:

1. a function that we added specifically because having to do the latter in two transactions is bad UX,

2. one that was suggested in the Wildcat V1 C4 audit by warden rvierdiiev (I'm including the screenshot here because I intend to link back to this issue quite a lot depending on how many people filed issues about this):

and 3) [which I was unaware of at the time] something we already had in our Sepolia deployment but didn't put into what was reviewed, precisely because it's so basic.

History aside, and alluded to above: we cannot prevent addresses from transferring arbitrary ERC-20 tokens to a market contract, and more importantly we wouldn't want to. In the event that a borrower somehow lost the key to the address they deployed the market from, given that the contracts are immutable there needs to be a way for an alternative address owned by the borrower to return assets to the market in order to effect withdrawal requests. What the warden is requesting is something that can't be done. The contract receives what it receives, regardless of the mechanism by which they get there.

The inability to execute repay in a closed market is present to stop borrowers from accidentally fat-fingering more assets in via the UI, and while there is certainly an onRepay hook option that's available, in practice there aren't any barring conditions we'd ever want to place on it for the aforementioned reason, and it hasn't been implemented in the hook templates we provided for review (this alone is admittedly not a reason to dismiss the finding out of hand, but again: we're talking about a QOL function which has its genesis in making something that we can't prevent simpler to do).

It is unclear how the invariant mentioned is breached by the existence of repay as-is: its invocation does not mint new market tokens, nor does it affect internal accounting other than increasing totalAssets, precisely as stated.

There are several observations relating to repay that have cropped up in this review: this is just the first one I saw - suffice it to say that it works as expected, hasn't meaningfully changed since V1 beyond porting it into the hook structure (which appears to be the original sin behind this particular filing), changes nothing net-net even if it were possible to do what the warden suggests, and any findings related to it are QA-category at best.

[3docSec]

Closing as insufficient proof: the report does not clarify how skipping the hook calls can create an H/M impactful situation

[c4-judge]

3docSec marked the issue as unsatisfactory:
Insufficient proof