Sender with both `emergency_council` and `authorised_enablers` roles cannot disable the pool

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/lib.rs#L1177-L1186

Vulnerability details

The enable_pool_579_D_A658 function is intended to be used only by the seawater admin or the emergency council. Therefore, it performs access control checks:

    /// Changes if a pool is enabled. Only usable by the seawater admin, or the emergency council, or the
    /// ... other code
    pub fn enable_pool_579_D_A658(&mut self, pool: Address, enabled: bool) -Result<(), Revert{
        assert_or!(
            self.seawater_admin.get() == msg::sender()
                || self.emergency_council.get() == msg::sender()
                || self.authorised_enablers.get(msg::sender()),
            Error::SeawaterAdminOnly
        );

        if self.emergency_council.get() == msg::sender()
            && self.seawater_admin.get() != msg::sender()
            && enabled
        {
            // Emergency council can only disable!
            return Err(Error::SeawaterEmergencyOnlyDisable.into());
        }
    /// ... other code

lib.rs#L1170-L1184

These checks are easily passed if the sender is whitelisted as either emergency_council or authorised_enablers. However, if the sender has both of these roles, the assertion will fail, preventing them from setting the pool status to enabled or disabled.

Impact

The aforementioned checks prevent a user with both emergency_council and authorised_enablers roles from changing the pool status. This results in unnecessary restrictions, limiting the functionality of authorized users who may be unable to enable or disable pools when necessary, potentially leaving pools in an undesired state during critical situations.

Proof of Concept

Let's assume Alice has both emergency_council and authorised_enablers roles. She wants to disable the pool because she has an authorised_enablers role, which she believes should allow her to do it.

Alice calls the enable_pool_579_D_A658 function with a pool address and enabled parameter set to false (see comments):

    pub fn enable_pool_579_D_A658(&mut self, pool: Address, enabled: bool) -Result<(), Revert{
        assert_or!(
            self.seawater_admin.get() == msg::sender() // @audit false
                || self.emergency_council.get() == msg::sender() // @audit true
                || self.authorised_enablers.get(msg::sender()), // @audit true
            Error::SeawaterAdminOnly
        ); // @audit the assertion passed (false || true || true)

        if self.emergency_council.get() == msg::sender() // @audit true
            && self.seawater_admin.get() != msg::sender() // @audit true
            && enabled // @audit true
            // @audit (true && true && true) 
            // @audit -returns an Error even though Alice has an `authorised_enablers` role
        {
            // Emergency council can only disable!
            return Err(Error::SeawaterEmergencyOnlyDisable.into());
        }

        self.pools.setter(pool).set_enabled(enabled);
        Ok(())
    }

lib.rs#L1169-L1187

Tools Used

Manual review.

Recommended Mitigation Steps

To allow any authorised_enablers, even with the emergency_council role to disable the pool, modify the current checks in the enable_pool_579_D_A658 function. For instance, replace:

        if self.emergency_council.get() == msg::sender()
            && self.seawater_admin.get() != msg::sender()
            && enabled
        {
            // Emergency council can only disable!
            return Err(Error::SeawaterEmergencyOnlyDisable.into());
        }

        self.pools.setter(pool).set_enabled(enabled);
        Ok(())

in lib.rs#L1177-L1186, with:

        if self.authorised_enablers.get(msg::sender()) {
            self.pools.setter(pool).set_enabled(enabled);
        }
        else if self.emergency_council.get() == msg::sender()
            && self.seawater_admin.get() != msg::sender()
            && enabled
        {
            // Emergency council can only disable!
            return Err(Error::SeawaterEmergencyOnlyDisable.into());
        }

        Ok(())

Assessed type

Invalid Validation

[devival]

Hello @alex-ppg,

I would like to request a review here,

AI-based duplicate group recommendation mistakenly assigned this issue to _04_group, which refers to theauthorised_enablers privileged role and its ability to enable and disable pools. The judge's comment on that group:

The submission details that the in-line documentation of a particular role does not match its actual capabilities.

I do not believe that these submissions are HM vulnerabilities as the roles are privileged, all functionality of the system is properly accessible, and no harm arises from the discrepancy identified. In reality, it is expected that enablers can also disable and that they cannot create pools if we did not have the in-line documentation to accompany the role.

However, this issue (validation-217) arises in a different setting: when a privileged user has BOTH emergency_council and authorised_enablers roles. In the case where somebody holds both emergency_council and authorised_enablers, they should be able to disable the pool (docs: “emergency council can shut the dapp down if needed”).

But due to the flaw in theenable_pool_579_D_A658function logic, it is not possible for theemergency_councilto disable the pool if they are also whitelisted asauthorised_enablers.

As a consequence, in case the dapp needs to be shut down, the emergency council might not be able to do that.

[alex-ppg]

Hey @devival, thank you for your PJQA contribution. I would like to clarify that validation repository findings are not directly evaluated by the judge of a contest if they do not pass the validation phase.

I do not believe the risk outlined merits an HM severity rating as there is no indication both of those roles are expected to be held by the same address directly in the codebase.