Multiplier is incorrectly calculated in `Controller`

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/controller.cairo#L167

Vulnerability details

Impact

In Controller smart contract, multiplier value should be calculated using this formula:

However, due to mistake in multiplication of i_gain, it's miscalculated and the error occurs. This can lead to a multiplier reflecting an incorrect value and therefore affect yin borrowing rate.

Proof of Concept

According to the multiplier formula, first we calculate p_term. The formula for p_term is following:

p_term = p_gain * error

And that's done right using the get_p_term_internal() function:

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/controller.cairo#L243-245

fn get_p_term_internal(self: @ContractState) -> SignedRay {
            self.p_gain.read() * nonlinear_transform(self.get_current_error(), self.alpha_p.read(), self.beta_p.read())
        }

Then we add right and go to integral term calculation. It's almost the same as p_term - only time scale factor is added:

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/controller.cairo#L256-258

  old_i_term
                + nonlinear_transform(self.get_prev_error(), self.alpha_i.read(), self.beta_i.read())
                    * time_since_last_update_scaled

The problem is that when we add this formula to the multiplier, we multiply it additionally by i_gain:

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/controller.cairo#L166-167

 let new_i_term: SignedRay = self.get_i_term_internal();
                multiplier += i_gain * new_i_term;

And, according to the formula, only this part should be multiplied by i_gain:

nonlinear_transform(self.get_prev_error(), self.alpha_i.read(), self.beta_i.read())
                    * time_since_last_update_scaled

But old_term also ends up multiplied.

Tools Used

Manual review.

Recommended Mitigation Steps

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/controller.cairo#L256-258

-               old_i_term + nonlinear_transform(self.get_prev_error(), 
-               self.alpha_i.read(), self.beta_i.read()) * 
-               time_since_last_update_scaled

+               old_i_term + i_gain * nonlinear_transform(self.get_prev_error(), 
+               self.alpha_i.read(), self.beta_i.read()) * 
+               time_since_last_update_scaled

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/controller.cairo#L167

-               multiplier += i_gain * new_i_term;
+               multiplier += new_i_term;

Assessed type

Other

[bytes032]

The source for the formula is not available

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[alex-ppg]

The Warden has demonstrated how the formula that is in use by the Controller contradicts the documentation of the project, and namely the specification of the aforementioned formula.

In detail, the specification states:

$$ y[k] = 1 + kp\frac{u[k]^{a{kp}}}{\sqrt{1 + u[k]^{β_{kp}}}} + y_i[k-1] + ki\frac{u[k-1]^{a{ki}}}{{\sqrt{1 + u[k-1]^{β_{ki}}}}}\Delta t $$

We are interested in the latter part of the formula, specifically:

$$ y_i[k-1] + ki\frac{u[k-1]^{a{ki}}}{{\sqrt{1 + u[k-1]^{β_{ki}}}}}\Delta t $$

The documentation states that $k_i$ stands for the:

gain that is applied to the integral term

The problem highlighted by the Warden is that the implementation will calculate the following:

$$ k_i (yi[k-1] + \frac{u[k-1]^{a{ki}}}{{\sqrt{1 + u[k-1]^{β_{ki}}}}}\Delta t) $$

The above corresponds to:

let new_i_term: SignedRay = self.get_i_term_internal();
multiplier += i_gain * new_i_term;

Whereby the get_i_term_internal function will ultimately yield:

old_i_term
    + nonlinear_transform(self.get_prev_error(), self.alpha_i.read(), self.beta_i.read())
    * time_since_last_update_scaled

In the above:

• i_gain: $k_i$
• old_i_term: $y_i[k-1]$
• nonlinear_transform(self.get_prev_error(), self.alpha_i.read(), self.beta_i.read()): $\frac{u[k-1]^{a{ki}}}{{\sqrt{1 + u[k-1]^{β{ki}}}}}$
• time_since_last_update_scaled: $\Delta t$

Based on the above analysis, the documentation indeed contradicts what the code calculates. The Opus team is invited to evaluate this exhibit, however, regardless of the correct behaviour of the system this submission will be accepted as valid due to the documentation being the "source of truth" during the contest's duration.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-sponsor]

milancermak (sponsor) confirmed

[rodiontr]

@alex-ppg sorry, but why it’s marked as insufficient quality ? I don’t know what’s the problem with the source but I can see the formula when I open up the issue

[alex-ppg]

Hey @rodiontr, thanks for supplying feedback on this submission. The source is visible to you because you used a personal link that is private for all other GitHub users:

• https://github.com/rodiontr/fdkfd/assets/109477234/c9f8db25-3b94-4662-b86a-1541a1fc133c

You should try utilizing other image hosting platforms in the future if you wish your images to be visible. Additionally, the submission has been accepted as valid, and the Insufficient Quality Report label is one assigned by the pre-sorter rather than the judge. It does not impact the submission's payout.