FixedTermLoanHook looks at block.timestamp instead of expiry

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L848

Vulnerability details

Impact

FixedTermLoanHook looks at block.timestamp instead of expiry

Proof of Concept

The idea of FixedTermLoanHook is to only allow for withdrawals after a certain term end time. However, the problem is that the current implementation does not look at the expiry, but instead at the block.timestamp

  function onQueueWithdrawal(
    address lender,
    uint32 /* expiry */,
    uint /* scaledAmount */,
    MarketState calldata /* state */,
    bytes calldata hooksData
  ) external override {
    HookedMarket memory market = _hookedMarkets[msg.sender];
    if (!market.isHooked) revert NotHookedMarket();
    if (market.fixedTermEndTime > block.timestamp) {
      revert WithdrawBeforeTermEnd();
    }

This creates inconsistencies such as forcing users not only to wait until term's end, but also having to wait an extra withdrawalBatchDuration before they're able to withdraw their funds.

Tools Used

Manual review

Recommended Mitigation Steps

Check the expiry instead of block.timestamp

Assessed type

Context

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[laurenceday]

We've reflected on this a little bit, and decided that we want to turn this from a confirmed into an acknowledge.

The reasoning goes as follows:

Imagine that a fixed market has an expiry of December 30th, but there's a withdrawal cycle of 7 days.

Presumably the borrower is anticipating [and may have structured things] such that they are expecting to be able to make full use of any credit extended to them until then, and not a day sooner.

Fixing this in the way suggested would permit people to place withdrawal requests on December 23rd, with the potential to tip a market into delinquent status (depending on grace period configuration) before the fixed duration has actually met.

Net-net we think it makes more sense to allow the market to revert back to a perpetual after that expiry and allow withdrawal requests to be processed per the conditions. The expectation here would be that the withdrawal cycle would actually be quite short.

[InfectedIsm]

Hi, thanks for the timely judging, its refreshing!
May I comment on this issue: can we really consider this a bug rather than a feature and a design improvement, also considering sponsor comment?
Expiry mechanism is known by borrower and lender, so if borrower want lenders to be able to withdraw on time, he can simply configure fixedTermEndTime = value - withdrawalBatchDuration

[3docSec]

Hi @InfectedIsm,
I agree we are close to an accepted trade-off territory. Here I lean on the sponsor who very transparently made it clear this trade-off is not something they had deliberately thought of.

Therefore, because the impact is compatible with M severity, "satisfactory M" plus "sponsor acknowledged" is a fair way of categorizing this finding