Freeze permissions are less permissive than KittyCore, unable to freeze if CEO unavailable or CEO key lost

[ghost]

Description

CryptoKitties Core Contract contains more permissive settings for which accounts can freeze the contract than Offers.sol. KittyCore has an only C-Level modifier that is accessible to any of the CEO/CFO/COO roles. In contrast, Offers.sol uses an only CEO modifier to freeze the contract.

Scenario

By only allowing the CEO to freeze Offers.sol, there are two main concerns.

(1) If a serious vulnerability is found, restricting freeze permissions to only the CEO does not allow the team to react as quickly. If both the CFO and COO also have the ability to freeze the contract, they could do so quicker.

(2) Additionally if the CEO key were ever lost, there would be no way to freeze the contract, and thus no way to withdraw funds fully and start again.

KittyCore's only C-Level modifier could solve this by allowing either CFO or COO to freeze the contract.

Impact

Medium: Low probability of occurring, high impact if it does.

If the CEO key is lost, nobody could call bidderWithdrawFunds() to retrieve their funds fully.

Even if the CEO key is not lost, if a serious vulnerability is found in the contract, the team could respond quicker if both the CFO and COO could also freeze the contract.

Reproduction

Two cases:

Case 1:

(1) A serious vulnerability is found in the contract.

(2) The CFO and COO are awake, but the CEO is asleep.

(3) An Attacker can drain funds out of contract while team watches vulnerability unfold, being unable to freeze the contract until CEO key is available.

Case 2:

(1) The CEO key is lost by whatever means (death, damage, natural disaster, theft, etc.)

(2) The contract can never be frozen, and users can never call bidderWithdrawFunds()

Fix

Add the only C-Level modifier from KittyCore:

modifier onlyCLevel() {
        require(
            msg.sender == cooAddress ||
            msg.sender == ceoAddress ||
            msg.sender == cfoAddress
        );
        _;
    }

and apply it to freeze:

function freeze() external onlyCLevel whenNotFrozen {
        frozen = true;
    }

[hwrdtm]

Thanks @TomLeeFounder for your feedback! We will take it into consideration

[arthcmr]

Thanks for your participation, @TomLeeFounder! Our team has reviewed your submission, and we are pleased to reward you for your report.

Severity: Low Points: 125