Incorrect Timestamp Update Leading to Inaccurate Interest Calculations and Potential Financial Losses

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/ApplyInterestLib.sol#L26-L48
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/ApplyInterestLib.sol#L50-L63

Vulnerability details

Impact

When applyInterestForPoolStatus returns an interest rate of 0, pairStatus.lastUpdateTimestamp is still updated.

If there is a valid need to update the interest rate within the same block, the check if (pairStatus.lastUpdateTimestamp >= block.timestamp) prevents this from happening. As a result, the interest rate and protocol fees are not recalculated and updated as needed.

Incorrect or missed interest calculations result in lenders receiving less interest than they are entitled to. Over time, this can accumulate to significant financial losses for lenders.

Incorrect interest calculations also affect the protocol's fee income.

Proof of Concept

function applyInterestForToken(mapping(uint256 => DataType.PairStatus) storage pairs, uint256 pairId) internal {
        DataType.PairStatus storage pairStatus = pairs[pairId];

        Perp.updateFeeAndPremiumGrowth(pairId, pairStatus.sqrtAssetStatus);

        // avoid applying interest rate multiple times in the same block
        if (pairStatus.lastUpdateTimestamp >= block.timestamp) {
            return;
        }

        (uint256 interestRateQuote, uint256 protocolFeeQuote) =
            applyInterestForPoolStatus(pairStatus.quotePool, pairStatus.lastUpdateTimestamp, pairStatus.feeRatio);

        (uint256 interestRateBase, uint256 protocolFeeBase) =
            applyInterestForPoolStatus(pairStatus.basePool, pairStatus.lastUpdateTimestamp, pairStatus.feeRatio);

        // Update last update timestamp
        pairStatus.lastUpdateTimestamp = block.timestamp;

        if (interestRateQuote > 0 || interestRateBase > 0) {
            emitInterestGrowthEvent(pairStatus, interestRateQuote, interestRateBase, protocolFeeQuote, protocolFeeBase);
        }
    }

Link

 function applyInterestForPoolStatus(Perp.AssetPoolStatus storage poolStatus, uint256 lastUpdateTimestamp, uint8 fee)
        internal
        returns (uint256 interestRate, uint256 totalProtocolFee)
    {
        if (block.timestamp <= lastUpdateTimestamp) {
            return (0, 0);
        }

        uint256 utilizationRatio = poolStatus.tokenStatus.getUtilizationRatio();

        // Skip calculating interest if utilization ratio is 0
        if (utilizationRatio == 0) {
            return (0, 0);
        }

Link

POC

The current implementation of the applyInterestForToken function in the Predy protocol can lead to inaccurate interest rate and protocol fee calculations due to the improper update of pairStatus.lastUpdateTimestamp. Here’s a detailed technical explanation:

Interest Calculation Function (applyInterestForPoolStatus) Conditions:

The function applyInterestForPoolStatus may return an interest rate of 0 in the following scenarios:

• When block.timestamp is less than or equal to lastUpdateTimestamp Code.
• When the utilization ratio is 0 Code.

Issue in applyInterestForToken:

The applyInterestForToken function does not verify whether the interest rate is actually updated before updating pairStatus.lastUpdateTimestamp Code.
As a result, pairStatus.lastUpdateTimestamp can be updated even when the interest rate remains unchanged (i.e., interest rate is 0).

Consequences:

If an attempt is made to update the interest rate within the same block, the check if (pairStatus.lastUpdateTimestamp >= block.timestamp) prevents the function from applying the correct interest rate.
This issue leads to a situation where the interest rate and protocol fees are not updated as needed, causing potential losses in interest earnings and protocol fees.

Example Scenario
Initial Interaction:

A lender supplies tokens at time T0.
applyInterestForToken is called, and applyInterestForPoolStatus returns an interest rate of 0 due to low utilization.
Despite no interest being applied, pairStatus.lastUpdateTimestamp is updated to T0.
Subsequent Interaction within the Same Block:

At time T0 + 5 seconds, another interaction occurs.
applyInterestForToken checks pairStatus.lastUpdateTimestamp (T0) against the current block timestamp.
Since pairStatus.lastUpdateTimestamp >= block.timestamp, the function returns early without recalculating interest, even though conditions might have changed.

Tools Used

Manual Audit

Recommended Mitigation Steps

The pairStatus.lastUpdateTimestamp value only updated when there is change in interest rate

 function applyInterestForToken(mapping(uint256 => DataType.PairStatus) storage pairs, uint256 pairId) internal {
        DataType.PairStatus storage pairStatus = pairs[pairId];

        Perp.updateFeeAndPremiumGrowth(pairId, pairStatus.sqrtAssetStatus);

        // avoid applying interest rate multiple times in the same block
        if (pairStatus.lastUpdateTimestamp >= block.timestamp) {
            return;
        }

        (uint256 interestRateQuote, uint256 protocolFeeQuote) =
            applyInterestForPoolStatus(pairStatus.quotePool, pairStatus.lastUpdateTimestamp, pairStatus.feeRatio);

        (uint256 interestRateBase, uint256 protocolFeeBase) =
            applyInterestForPoolStatus(pairStatus.basePool, pairStatus.lastUpdateTimestamp, pairStatus.feeRatio);

        // Update last update timestamp
-        pairStatus.lastUpdateTimestamp = block.timestamp;
+  // Update last update timestamp only if interest rates were applied
        if (interestRateQuote > 0 || interestRateBase > 0) {
+    pairStatus.lastUpdateTimestamp = block.timestamp;
            emitInterestGrowthEvent(pairStatus, interestRateQuote, interestRateBase, protocolFeeQuote, protocolFeeBase);
        }

Assessed type

Other

[sathishpic22]

Hi @alex-ppg

Thank you for judging.

While it is acknowledged that updating the interest rate in the subsequent block is feasible, the challenge arises when there is a genuine need to adjust the interest rate. Under the current implementation, this adjustment is not possible. Additionally, the timestamp values denote the last instance when the interest rate was updated.

// Update last update timestamp
        pairStatus.lastUpdateTimestamp = block.timestamp;

In contrast, even though the values of pairStatus.lastUpdateTimestamp are updated, there is no corresponding change in the interest rate.

Two potential issues have been identified:

1. It is not possible to update the interest rate when pairStatus.lastUpdateTimestamp is updated, even if there is no change in the interest rate. If an attempt is made to update the correct interest rate within the same block, the current implementation does not permit this.

2. pairStatus.lastUpdateTimestamp returns inaccurate values externally, even when there is no update to the interest rate.

These technical issues, as demonstrated through the Proof of Concept (POC), underscore critical areas that require revision in the contract logic.

These issues seem to accurately describe the problem at hand. Please review and confirm. Thank you.

[alex-ppg]

Hey @sathishpic22, thanks for contributing to the PJQA process! This represents a validation repository finding and as such was not evaluated by me directly.

I do not foresee any HM-level issue arising from the described behavior, and updating the timestamp when there is no growth is acceptable.