code-423n4 / 2024-08-wildcat-findings

3 stars 1 forks source link

The penalty APR to be returned can be manipulated #55

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/libraries/FeeMath.sol#L121

Vulnerability details

Impact

The penalty APR that is meant to be incurred can potentially be manipulated. In certain scenarios, a borrower or another party could deliberately manipulate the timing of these updates to minimize the penalty.

Description

The delinquency penalty retuned can be manipulated potentially by the borrower.

The function updateScaleFactorAndFees is called within the getUpdatedState function to accrue interest and calculate penalties, including the penalty APR, which is applied when a market enters delinquency.

function updateScaleFactorAndFees(
    MarketState memory state,
    uint256 delinquencyFeeBips,
    uint256 delinquencyGracePeriod,
    uint256 timestamp
  )
    internal
    pure
    returns (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee)
  {
    baseInterestRay = state.calculateBaseInterest(timestamp);

    if (state.protocolFeeBips > 0) {
      protocolFee = state.applyProtocolFee(baseInterestRay);
    }

    if (delinquencyFeeBips > 0) {
      delinquencyFeeRay = state.updateDelinquency(
        timestamp,
        delinquencyFeeBips,
        delinquencyGracePeriod
      );
    }

    // Calculate new scaleFactor
    uint256 prevScaleFactor = state.scaleFactor;
    uint256 scaleFactorDelta = prevScaleFactor.rayMul(baseInterestRay + delinquencyFeeRay);

    state.scaleFactor = (prevScaleFactor + scaleFactorDelta).toUint112();
    state.lastInterestAccruedTimestamp = uint32(timestamp);
  }

The problem, however, lies within the updateDelinquency function, where a borrower or another party can manipulate the delinquency penalty. This is because when the updateTimeDelinquentAndGetPenaltyTime function returns the timeWithPenalty, it uses a min value calculation, meaning the penalty time will be based on the smallest value between secondsRemainingWithPenalty and timeDelta.

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/libraries/FeeMath.sol#L120C4-L121C66

 // Only apply penalties for the remaining time outside of the grace period.
    return MathUtils.min(secondsRemainingWithPenalty, timeDelta);

The issue is that timeDelta is the time difference between the last update timestamp and the current timestamp, and this can be manipulated. Consider the following scenario:

  1. The market is in a delinquent state and has exceeded the grace period.
  2. The borrower makes a repayment to resolve the delinquency, triggering the _writeState function to update the state and mark the market as no longer delinquent.
  3. However, before the next transaction occurs, the borrower (or anyone else) calls updateState shortly after the repayment to ensure that timeDelta (the time difference) is less than secondsRemainingWithPenalty.

By calling the function early, the borrower can reduce the calculated penalty time, thereby manipulating the penalty that would otherwise be applied.

Tools Used

Manual review

Recommended Mitigation Steps

The issue can be mitigated by implementing restrictions on the call to updateState, ensuring that it can only be invoked after a set period following any significant protocol interaction, like repayment, borrowing, or similar actions. This would prevent users from calling the function too frequently and manipulating the penalty APR by reducing the timeDelta.

An additional solution is to make updateState an internal function. This will ensures that penalties are applied fairly and consistently based on the actual time elapsed and the borrower’s actions, rather than being subject to user-driven timing exploits.

Assessed type

Access Control

d1ll0n commented 2 months ago

This fundamentally misunderstands what is happening here. secondsRemainingWithPenalty does not permanently become zero if the current time delta is zero - it just means that interest is only charged as time actually elapses. If the borrower is 30 days past the grace period and then cures their delinquency, waits 15 days and updates the state, they will have incurred penalties for the previous 15 days, and also for the next 15 days.

3docSec commented 1 month ago

Invalid: misunderstanding of the code involved

c4-judge commented 1 month ago

3docSec marked the issue as unsatisfactory: Invalid