It's not possible to update spot price of yin

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L793-L798 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/roles.cairo#L127-L202

Vulnerability details

Impact

Function update_yin_spot_price() from shrine.cairo is responsible for updating spot price of yin. However, it uses a role, which is nowhere set. This leads to the scenario, where update_yin_spot_price() cannot be called by anyone - since protocol does not define anyone with shrine_roles::UPDATE_YIN_SPOT_PRICE role.

Proof of Concept

File: shrine.cairo

        fn update_yin_spot_price(ref self: ContractState, new_price: Wad) {
            self.access_control.assert_has_role(shrine_roles::UPDATE_YIN_SPOT_PRICE);
            self.emit(YinPriceUpdated { old_price: self.yin_spot_price.read(), new_price });
            self.yin_spot_price.write(new_price);
        }

When we take a look at update_yin_spot_price implementation, we can see, that it can be called by UPDATE_YIN_SPOT_PRICE role: self.access_control.assert_has_role(shrine_roles::UPDATE_YIN_SPOT_PRICE);.

Now, let's take a look how roles are defined:

File: roles.cairo

const UPDATE_YIN_SPOT_PRICE: u128 = 131072;

However, this role is nowhere defined. Let's scroll over every role for shrine_roles:

fn abbot() -> u128 { DEPOSIT + FORGE + MELT + WITHDRAW }
fn caretaker() -> u128 { EJECT + KILL + SEIZE }
fn controller() -> u128 { SET_MULTIPLIER }
fn default_admin_role() -> u128 { ADD_YANG + SET_DEBT_CEILING + SET_MINIMUM_TROVE_VALUE + SET_THRESHOLD + KILL + UPDATE_RATES + UPDATE_YANG_SUSPENSION }
fn equalizer() -> u128 { ADJUST_BUDGET + EJECT + INJECT + SET_DEBT_CEILING }
fn flash_mint() -> u128 { INJECT + EJECT + SET_DEBT_CEILING }
fn purger() -> u128 { MELT + REDISTRIBUTE + SEIZE }
fn seer() -> u128 { ADVANCE }
fn sentinel() -> u128 { ADD_YANG + UPDATE_YANG_SUSPENSION }
fn transmuter() -> u128 { ADJUST_BUDGET + EJECT + INJECT }

As demonstrated above, no function uses that role. This basically means, that it's not possible to call update_yin_spot_price() by anyone, since no one has UPDATE_YIN_SPOT_PRICE role.

When we futher analyse the roles.cairo file (e.g. let's grep for UPDATE_YIN_SPOT_PRICE), we can notice that this role occurs only here:

    #[cfg(test)]
    #[inline(always)]
    fn all_roles() -> u128 {
        ADD_YANG
            + ADJUST_BUDGET
            + ADVANCE
            + DEPOSIT
            + EJECT
            + FORGE
            + INJECT
            + KILL
            + MELT
            + REDISTRIBUTE
            + SEIZE
            + SET_DEBT_CEILING
            + SET_MINIMUM_TROVE_VALUE
            + SET_MULTIPLIER
            + SET_THRESHOLD
            + UPDATE_RATES
            + UPDATE_YANG_SUSPENSION
            + UPDATE_YIN_SPOT_PRICE
            + WITHDRAW
    }

But, all_roles() has #[cfg(test)] annotation. This annotation, according to Cairo documentation means that:

The #[cfg(test)] annotation on the tests module tells Cairo to compile and run the test code only when you run scarb cairo-test, not when you run cairo-run. This saves compile time when you only want to build the library and saves space in the resulting compiled artifact because the tests are not included. You’ll see that because integration tests go in a different directory, they don’t need the #[cfg(test)] annotation. However, because unit tests go in the same files as the code, you’ll use #[cfg(test)] to specify that they shouldn’t be included in the compiled result.

This implies, that all_roles() won't be callable on the production environment (#[cfg(test)] is only for testing environment).

To sum up, UPDATE_YIN_SPOT_PRICE is defined only in all_roles() defined for testing purposes (#[cfg(test)] annotation). In the production environment, we won't be able to use all_roles(). And, because no other function defines UPDATE_YIN_SPOT_PRICE role, it won't be possible to call update_yin_spot_price().

Tools Used

Manual code review

Recommended Mitigation Steps

Add additional role in roles.cairo related to UPDATE_YIN_SPOT_PRICE.

Assessed type

Access Control

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[alex-ppg]

Roles can be configured by the deployer based on the Access Control implementation in use by the system, rendering this submission incorrect as the role is meant to be externally assigned.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid