Chinmay - Mathematical Discrepancies in equations used for calculating Interest Rates

[sherlock-admin]

Chinmay

medium

Mathematical Discrepancies in equations used for calculating Interest Rates

Summary

The two equations representing MAU - TU relationships that are used to check whether the Interest Rate should be decreased or Increased based on current state, are not homogenous. The implementation of these yields different values in calculations.

Vulnerability Detail

The two equations seen at PoolCommons.sol:L294 and L297 are checks that allow increasing or decreasing interest rates based on the current values of Meaningful Actual Utilization MAU and Target Utilization TU. The two equations are comparing same quantities mau and tu and should have same similar scaling/downscaling. Here is the code :

        if (4 * (tu - mau102) < (((tu + mau102 - 1e18) / 1e9) ** 2) - 1e18) {
            newInterestRate_ = Maths.wmul(poolState_.rate, INCREASE_COEFFICIENT);
        // decrease rates if 4*(tu-mau) > 1-(tu+mau-1)^2
        } else if (4 * (tu - mau) > 1e18 - ((tu + mau - 1e18) ** 2) / 1e18) {
            newInterestRate_ = Maths.wmul(poolState_.rate, DECREASE_COEFFICIENT);
        }

        // bound rates between 10 bps and 50000%
        newInterestRate_ = Maths.min(500 * 1e18, Maths.max(0.001 * 1e18, newInterestRate_));
    }

We notice a difference in how they use downscaling by 1e9 or 1e18 to attain WAD(1e18) precision on both sides of the inequality.

We are interested in the term (tu + mau102 - 1e18) / 1e9) ** 2). For the increase rate check, (tu + mau102 - 1e18) / 1e9) ** 2) is used, but notice that for the decrease rate check, ((tu + mau - 1e18) ** 2) / 1e18) is used instead. This discrepancy will lead to a different set of results for this expression in some cases.

When I asked the developers, why this discrepancy exists, they said "so the thing is that during the invariant testing, with some huge values, the first one overflowed, let me find the commit so we decided to make it allow higher values, by dividing by 1e9 and then pow 2, instead having the 1e18 at pow 2 and then / 1e18. But the else branch wasn't changed, as we hit no failure there."

They changed the first equation to deal with an overflow issue, but they did not consider changing the second one because it did not overflow. But they expected it to be mathematically the same. One of the developers said, "mathematically they should be the same IMO... not sure why differences, granted the else clause would be better written same way e.g. (4 * (tu - mau) > 1e18 - ((tu + mau - 1e18) / 1e9) ** 2)"

The effect of this discrepancy is that for some sets of values both these terms aren't equivalent. I plotted these two expressions here : https://www.desmos.com/calculator/cjaiyhtmob So the mathematical equation yields different values than was expected and what the equation should represent.

Impact

The mathematical equation used was not the one that was intended. There is a clear discrepancy in the results of these equations and thus this is a medium severity issue because the protocol will not work as expected in some cases.

One of the developers agreed, "mathematically they should be the same IMO... not sure why differences, granted the else clause would be better written same way e.g. (4 * (tu - mau) > 1e18 - ((tu + mau - 1e18) / 1e9) ** 2)"

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/libraries/external/PoolCommons.sol#L297

Tool used

Manual Review

Recommendation

Change Line 297 from 4 * (tu - mau) > 1e18 - ((tu + mau - 1e18) ** 2) / 1e18) to 4 * (tu - mau) > 1e18 - ((tu + mau - 1e18) / 1e9) ** 2)

[grandizzy]

We acknowledge this is a discrepancy in our code and will be fixed in https://github.com/ajna-finance/contracts/pull/903

[dmitriia]

We acknowledge this is a discrepancy in our code and will be fixed in ajna-finance/contracts#903

Fix looks ok

[ctf-sec]

Escalate for 10 USDC.

the severity is low unless the report describe what is the true impact of the mismatched implementation

[sherlock-admin]

Escalate for 10 USDC.

the severity is low unless the report describe what is the true impact of the mismatched implementation

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[chinmay-farkya]

Escalate for 10 USDC I'd like to elaborate the impact to justify medium severity

1. The two expressions above are not same for some sets of values as shown in the graph. This means that intended implementation is different from actual code. Not working as intended is itself a criteria based on previous judged issues. The protocol wanted to increase/decrease rates based on these conditions, but now the decrease rates conditions may result in an incorrect decrease operation when it should have behaved homogenous to the increase rates condition check.
2. Issues like this where mathematical implementations differ from desired/documented one are considered a medium severity issue : https://github.com/code-423n4/2023-03-mute-findings/issues/19
3. This will prevent interest rate from decreasing when it should have decreased (and the other way round), which means the new Interest rate may be wrong and this value is then propagated to the state (for example at PoolCommons#L181 new Interest rate is not updated if it was the same as before, and the mentioned discrepancy can result in this because it was incorrect to not change it when it should have ) and used in the system. What this means is an incorrect value of interest rate is being used across the system, which leads to it being a medium severity issue.

[sherlock-admin]

Escalate for 10 USDC I'd like to elaborate the impact to justify medium severity

1. The two expressions above are not same for some sets of values as shown in the graph. This means that intended implementation is different from actual code. Not working as intended is itself a criteria based on previous judged issues. The protocol wanted to increase/decrease rates based on these conditions, but now the decrease rates conditions may result in an incorrect decrease operation when it should have behaved homogenous to the increase rates condition check.
2. Issues like this where mathematical implementations differ from desired/documented one are considered a medium severity issue : https://github.com/code-423n4/2023-03-mute-findings/issues/19
3. This will prevent interest rate from decreasing when it should have decreased (and the other way round), which means the new Interest rate may be wrong and this value is then propagated to the state (for example at PoolCommons#L181 new Interest rate is not updated if it was the same as before, and the mentioned discrepancy can result in this because it was incorrect to not change it when it should have ) and used in the system. What this means is an incorrect value of interest rate is being used across the system, which leads to it being a medium severity issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xffff11]

There is no real impact shown on the issue. Discrepancies code-docs if there is no loss of funds is not a medium. In this case it is not demonstrated that having different equations affect the protocol. I would agree at the current state with first escalation if no extra info is given, low

[dmitriia]

Tend to agree, it looks like valid low.

The difference with (2) is that there is no clear material loss path here, such as stakers losing the rewards there:

As we will see this means that all stakers that do not have a tokenRatio of exactly equal 0 or exactly equal 1 lose out on rewards that they should receive according to the documentation.

[chinmay-farkya]

It does affect the protocol imo. See point 3 in above escalation. Interest rates not being decreased when they should have is incorrect logic. I will reiterate the third point here,

"This will prevent interest rate from decreasing when it should have decreased (and the other way round), which means the new Interest rate may be wrong and this value is then propagated to the state (for example at PoolCommons#L181 new Interest rate is not updated if it was the same as before, and the mentioned discrepancy can result in this because it was incorrect to not change it when it should have ) and used in the system. What this means is an incorrect value of interest rate is being used across the system, which leads to it being a medium severity issue."

This interest rate is used to calculate interests which means sometimes the borrower may be charged more than they deserve and sometimes less than they deserve, which leads to loss of funds for the interest accruals as well as the borrower.

[MLON33]

We acknowledge this is a discrepancy in our code and will be fixed in ajna-finance/contracts#903

Fix looks ok

Moving sign-off by @dmitriia here for clarity.

[hrishibhat]

@chinmay-farkya

This means that intended implementation is different from actual code. Not working as intended is itself a criteria based on previous judged issues.

This alone would not be considered as valid for mathematical discrepancies. While I'm not denying the validity of the issue, please provide a POC with numbers to show the impact of this issue:

What this means is an incorrect value of interest rate is being used across the system, which leads to it being a medium severity issue.

[hrishibhat]

Result: Low Unique While there is a difference in the implementation between if-else sections mathematically, the change seems to be of negligent impact based on the fix implemented and the sponsor comments on the fix.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ctf-sec: accepted
• chinmay-farkya: rejected