Incorrect use of `state.isDelinquent` in `updateTimeDelinquentAndGetPenaltyTime` function - total misbehavior of applying delinquency fee

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/libraries/FeeMath.sol#L96 https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketBase.sol#L406

Vulnerability details

Impact

Due to the incorrect value of state.isDelinquent, the updateTimeDelinquentAndGetPenaltyTime function malfunctions, causing significant disruptions in fee accumulation.

Proof of Concept

Over time, as market operations occur, borrowers accumulate fees. This process is managed by the FeeMath.updateScaleFactorAndFees function. While this function updates the scaleFactor and applies the protocolFee, it also implements the delinquency fee based on the market's delinquency status. The calculation is performed by the updateDelinquency function, which then utilizes the updateTimeDelinquentAndGetPenaltyTime function to determine the duration for which the delinquencyFeeBips should be applied.

This function has a critical flaw by using incorrect state.isDelinquent value. It's assumed that:

1. state.isDelinquent is up-to-update
2. The delinqency state of market has been consistent (state.isDelinquent) since last update until current timestamp.

  function updateTimeDelinquentAndGetPenaltyTime(
    MarketState memory state,
    uint256 delinquencyGracePeriod,
    uint256 timeDelta
  ) internal pure returns (uint256 /* timeWithPenalty */) {
    // Seconds in delinquency at last update
    uint256 previousTimeDelinquent = state.timeDelinquent;

>>   if (state.isDelinquent) {

    ...
  }

However, this is WRONG because

• state.isDelinquent is the delinquency status of last updated time (indicated by state.lastInterestAccruedTimestamp)
• It is only updated when calling _writeState function which is normally at the end of an action.

So the whole deliquent penalty time is calculated wrongly, which leads to incorrect accumulation of delinquency fee.

Tools Used

Manual Review

Recommended Mitigation Steps

Though we can't completely guarantee 2nd point (The delinqency state of market has been consistent) because delinquency state can be changed in-between by direct transfer-in of assets (either by borrower or whoever), we can mostly assume it's true for a specific duration between market operations.

We can have correct value of state.isDelinquent, so that updateTimeDelinquentAndGetPenaltyTime function could perform correctly. It can be done by making changes to _getUpdatedState function of WildcatMarketBase contract.

  function _getUpdatedState() internal returns (MarketState memory state) {
    state = _state;
+   bool isDelinquent = state.liquidityRequired() > totalAssets();
+   state.isDelinquent = isDelinquent;
    ...
  }

Assessed type

Error

[d1ll0n]

The reason we record the delinquency status at the end of a transaction and then accrue time delinquent at the beginning of the next is to ensure the delinquency timer always increases by at least the amount of time the market was actually delinquent.

It is technically possible for a market to become healthy again without being updated, but there are only three ways a market can go from delinquent to not delinquent:

• A direct transfer, which no one has any incentive to do and which we can't react to since ERC20 does not have any kind of hooks
• A deposit, which will update the state

• A repay, which will update the state

  If it worked the way you're proposing, a borrower could negate a long period of delinquency simply by transferring tokens in at the end and then updating the state. This is certainly not a high, it's more of an opinion on who should bear the burden of updating the state: the lender or the borrower. If we went with the lender, they would have to be expected to constantly poke the state whenever a borrower is delinquent, lest they come along at the last second, cure any delinquency and incur no penalties. On the other hand, the borrower bearing this burden of updating the state when it is in their favor simply means they should use the repay function, which they would almost certainly be doing every time they cure delinquency regardless.

[3docSec]

If I am not mistaken, this finding was discussed extensively here https://github.com/code-423n4/2023-10-wildcat-findings/issues/323 and at this point it's quite clear it's a deliberate design choice.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid