updateIRMParams does not call applyInterestForToken before updating irmParams which leads to incorrect calculation of interest rate for subsequent trades.

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/AddPairLogic.sol#L128
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/AddPairLogic.sol#L96

Vulnerability details

Impact

Incorrect interest rate value will be calculated for future trades as the update to irmParams does not update the lastUpdatedAt or calculate the interest rate before updating the value of irmParams. This also results in the incorrect calculation of totalProtocolFees

Proof of Concept

Consider the function updateIRMParams. This is called from the PredyPool contract which internally calls this function in the AddPairLogic contract -

    function updateIRMParams(
        DataType.PairStatus storage _pairStatus,
        InterestRateModel.IRMParams memory _quoteIrmParams,
        InterestRateModel.IRMParams memory _baseIrmParams
    ) external {
        validateIRMParams(_quoteIrmParams);
        validateIRMParams(_baseIrmParams);

        _pairStatus.quotePool.irmParams = _quoteIrmParams;
        _pairStatus.basePool.irmParams = _baseIrmParams;

        emit IRMParamsUpdated(_pairStatus.id, _quoteIrmParams, _baseIrmParams);
    }

It validates the IRMParams and updates it. But, there is an issue in doing so.
Before the IRMParams is updated, applyInterestForToken in ApplyInterestLib should be called first. It's not being called.

applyInterestForToken first calls the function applyInterestForPoolStatus twice (for quotepool and basepool) and then applyInterestForToken also updates the lastUpdateTimestamp to block.timestamp.

In applyInterestForPoolStatus -

    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);
        }

        // Calculates interest rate
        interestRate = InterestRateModel.calculateInterestRate(poolStatus.irmParams, utilizationRatio)
            * (block.timestamp - lastUpdateTimestamp) / 365 days;

        totalProtocolFee = poolStatus.tokenStatus.updateScaler(interestRate, fee);

        poolStatus.accumulatedProtocolRevenue += totalProtocolFee / 2;
        poolStatus.accumulatedCreatorRevenue += totalProtocolFee / 2;
    }

Note this line -

        interestRate = InterestRateModel.calculateInterestRate(poolStatus.irmParams, utilizationRatio)
            * (block.timestamp - lastUpdateTimestamp) / 365 days;

The irmParams params are used to calculate the interestRate, which is then further used to calculate the protocolFee.

So, the period between -> block.timestamp - lastUpdateTimestamp is supposed to use the current poolStatus.params to calculate the interest rate. But, when updateIRMParams are directly updated, without first calling applyInterestForToken, any time a new trade is opened, it will be using the updated values of irmParams to calculate the interest rate for the current time period instead of using the previous values. This is problematic.

For example, if the previous values of slope1 and slope2 were smaller in the previous values of irmParams, then the interestRate calculated would be smaller. But, if the new updated values of irmParams (slope1 and slope2) were bigger, then the interest rate would become bigger. But, for the current time period (block.timestamp - lastUpdateTimestamp), this indicates a step-wise jump in the calculation of interestRate. A higher interest rate is being calculated than intended for the current period. This directly affects the calculation of the totalProtocolFees as well.

This can be seen in the graph here -

Consider that the interest rate is the area under the graph. The first one represents the current case, where applyInterestForToken is not called before the params are updated. In the second case, applyInterestForToken is called first before updating. It's clear from the graphs that the first case is considering a higher value of interest rate because of a larger area, which is incorrect.

The same argument can be made for the updateFeeRatio function as well. The current time period must use the previous feeRatio to account for accurate fee calculation. So, before feeRatio is updated, it should also call the applyInterestForToken function.

Tools Used

Manual review

Recommended Mitigation Steps

While updating the params, the applyInterestForToken function should be called first for the current period to calculate the interestRate from block.timestamp to lastUpdateTimestamp, and then the updated value of irmParams can be used for durations after this block.timestamp.

The same should be done for the updateFeeRatio function.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

The submission and its duplicates have demonstrated how an update of the protocol's IRM parameters is not preceded by an interest update, permitting the updated variables to retroactively apply to the time elapsed since the last interest rate update incorrectly.

I believe a medium-risk severity rating is appropriate given that all IRM reconfigurations will lead to interest rates being improperly tracked in the system.