Pausable: Allow _pause() when the contract is currently paused

[CarsonCase]

🧐 Motivation
Currently in the Pausable contract, the docstring states regarding _pause() "The contract must not be paused".
I don't believe this should be an invariant of the pause function. I'll explain why in detail in the next section.

Currently it is not possible to use Pausable to change this, as if you attempt to override _pause() without the whenNotPaused modifier you'll quickly find that _paused is private and you cannot set it to true. Thus meaning that if you are using Pausable you can never pause when the contract is already paused.

📝 Details
But why is this an issue?

Imagine for a moment you have a script that will pull from a database the list of all your companies deployed pausable contracts, and attempt to pause them all in a multicall batch transaction in the event of an emergency. There are hundreds of contracts in this database, and when you see a worrying message from another team member about some on-chain activity you decide to be safe rather than sorry and pause all contracts.
However, somewhere in this database a contract is duplicated. Now because of the duplicated contract address, the entire multicall will fail. As the first pause to this contract will set _paused to true, then the second call will revert with: EnforcedPause.
Now, because of this, you must either find the duplicate contract and remove it, or individually pause contracts, taking significantly more time to react to the situation.

TL;DR : Enforcing that _pause can only be triggered when the contract is not paused can lead to DOS. While this does rely on an error of periphery tools and is not the responsibility of OZ, it's of significant concern for something as important as Pausable, and I think the function should at least have the ability to be overriden.

Thank you for reviewing this issue! I'm curious to hear what the OZ team/community thinks about this as I'm not a security researcher but a humble dev, so any thoughts or opinions on this matter are appreciated!

[kaym0]

While I think this works in theory, it's an inefficient use of resources. Likely by design to enforce better practices. I think the ideal solution would be to simply query all contracts (see public function below) and determine which ones are paused and then use a less expensive batch transaction to pause the remaining contracts.

function paused() public view virtual returns (bool) {
        return _paused;
    }

[CarsonCase]

I agree that this would be a good solution and that the issue here does lie on the builder of the offchain tools.

But making the query of what's paused and what's not takes time and for pause, it's possible that time is of the essence. Though hopefully a quarter of a second or so isn't what you're relying on.

I just bring it up since I've had to go and replace Pausable myself for our contracts because we want to uphold this guarantee that it won't fail, even though external batch tools will try-catch pauses

[kaym0]

I think if you really intent to solve on-chain, maybe delegating paused bool storage to parent contract which serves as administrator over any number of child contracts. Parent contract stores uint where each bit denotes a child contracts pause status as a "bit" at index. Using the code below as a guideline to getting / setting pause values. You will have to change it to suit your implementation.

Then in the case of pausing all contracts, you could simply set the uint to 0 to pause all contracts and 111..... to reenable them while still maintaining the functionality of individually pausing and unpausing each contract. This would allow to pause or unpause with a single low-cost transaction without having to worry about queries or any other overhead.

    function getBoolean(uint256 _packedBools, uint256 _boolNumber) public pure returns(bool) {
        uint256 flag = (_packedBools >> _boolNumber) & uint256(1);
        return (flag == 1 ? true : false);
    }


    function setBoolean(
        uint256 _packedBools,
        uint256 _boolNumber,
        bool _value
    ) public pure returns(uint256) {
        if (_value) {
            _packedBools = _packedBools | uint256(1) << _boolNumber;
            return _packedBools;
        }
        else {
            _packedBools = _packedBools & ~(uint256(1) << _boolNumber);
            return _packedBools;
        }
    }

[arr00]

It is actually possible to override our Pausable contract to achieve the desired behavior; however, we don't think it should be the default behavior. Note that the code below is not audited and should be verified prior to deploying.

contract NewPausable is Pausable {
    bool private _paused;

    function paused() public view virtual override returns (bool) {
        return _paused;
    }

    function _pause() internal virtual override {
        _paused = true;
        emit Paused(_msgSender());
    }

    function _unpause() internal override {
        _paused = false;
        emit Unpaused(_msgSender());
    }
}

[CarsonCase]

It is actually possible to override our Pausable contract to achieve the desired behavior; however, we don't think it should be the default behavior. Note that the code below is not audited and should be verified prior to deploying.

contract NewPausable is Pausable {
    bool private _paused;

    function paused() public view virtual override returns (bool) {
        return _paused;
    }

    function _pause() internal virtual override {
        _paused = true;
        emit Paused(_msgSender());
    }

    function _unpause() internal override {
        _paused = false;
        emit Unpaused(_msgSender());
    }
}

This is a good point and retains use of the canonical Pausable. I think though at this point I may as well continue to just fork Pausable myself and remove the modifier to avoid the redundant

[Amxx]

To iterate on @arr00's proposal, it is also possible to "just" do

contract PausableNoRevert is Pausable {
    function _pause() internal virtual override {
        if (!paused()) super._pause();
        // else do nothing
    }

    function _unpause() internal virtual override {
        if (paused()) super._unpause();
        // else do nothing
    }
}