Modifying the loan term setting can default existing loans

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L365

Vulnerability details

Summary

Protocol admins can modify the loan term settings. This action can inadvertently default existing loans created under different terms.

Impact

Positions in the Particle LAMM protocol are created for a configurable period of time, defined by the LOAN_TERM variable. If the loan exceeds this duration, and the LP owner stops renewals that affect their position, the lien can be liquidated.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L358-L368

358:         // check for liquidation condition
359:         ///@dev the liquidation condition is that
360:         ///     (EITHER premium is not enough) OR (cutOffTime > startTime AND currentTime > startTime + LOAN_TERM)
361:         if (
362:             !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
363:                 closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
364:                 (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
365:                     lien.startTime + LOAN_TERM < block.timestamp))
366:         ) {
367:             revert Errors.LiquidationNotMet();
368:         }

The liquidation condition in line 365 does the check using the current value of LOAN_TERM. As the loan term can be updated using updateLoanTerm(), this means that reducing this value may inadvertently cause the liquidation of existing positions that were originally intended for a longer period of time.

Proof of concept

Let's say the current configured loan term in ParticlePositionManager is 2 weeks.

1. A user creates a new position, expecting it to last at least 2 weeks.
2. The owner of the LP calls reclaimLiquidity() to stop it from being renewed.
3. The protocol changes the loan term setting to 1 week.
4. The user is liquidated after 1 week.

Recommendation

Store the loan term value at the time the position was created in the Lien structure, e.g. in lien.loanTerm. When checking the liquidation condition, calculate the end time using this value to honor the original loan term.

    if (
        !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
            closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
            (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
-               lien.startTime + LOAN_TERM < block.timestamp))
+               lien.startTime + lien.loanTerm < block.timestamp))
    ) {
        revert Errors.LiquidationNotMet();
    }

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[wukong-particle]

Hmm we are reluctant to add more data into lien struct because we want to restrict it to be less than 3 uint256. This can only be affected by contract level update of LOAN_TERM, which should be quite rare (controlled by governance in the future).

[romeroadrian]

Hmm we are reluctant to add more data into lien struct because we want to restrict it to be less than 3 uint256. This can only be affected by contract level update of LOAN_TERM, which should be quite rare (controlled by governance in the future).

I don't understand the concern here. Is this just gas? Loading LOAN_TERM will also require loading a word from storage. In any case, even if this is gov controlled, given enough protocol activity it will be difficult to update the setting without conflicting with existing loans.

[wukong-particle]

Yeah mostly gas concern. Adding loan term in the storage increases gas for opening/closing/liquidating the position.

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

wukong-particle (sponsor) acknowledged