coded `pauseFor(uint time)` functionality for `Pausable.sol`

[CarlosAlegreUr]

Summary :notebook:

I've re-factored, in a backwards compatible way, the Pausable.sol contract to allow developers to optionally support a pauseFor(uint time) functionality.

All tested and coded in this fork: my fork

Motivation 🧐

It is common to use the Pausable.sol contract in a project with some special permissions to 1 or more addresses.

However, as of now it exists the danger of pausing forever. The simplest example is if the only permissioned address losses its private key while the contract was paused.

Another more complex real-example I came across while auditing and which motivated me to propose this change is the following:

A project had 1 owner and X number of "guardians" which could pause the contracts.

The owner could be renounced to `address(0)` and once that was done, no-one, including guardians, 
would be able to pause the contracts again.

A malicious guardian could out of malice, or simply by accident, front-run the owner's renouncement 
call with a pause function call, effectively locking the contract forever.

Conclusion :end:

As protocols evolve in complexity and more complex role structures and permissions are created, the risk of pausing something forever won't be lower but higher. Thus, I consider useful this re-factored Pausable.sol contract to allow developers to optionally support a pauseFor() functionality.

Technical Details :computer:

• Backwards compatible:

The amount of state slots used remains unchanged. Only the first one. And new values do not override previous values as:

Previous values: 0 or 1 on the least significant bit. New values: 0 or 1 on the least significant bit, and the remaining 248 bytes are used for duration amounts when using pauseFor().

This makes proxies who desire to implement this functionality easily updateable.

• Optional functionality:

As per the usual Pasuable.sol, all new functionalities have been created using internal functions, thus if the project using Pausable.sol thinks they do not need the extra pauseFor() functionality they can just never call it and the compiler won't include it in the final bytecode.

How it works :gear:

• _pauseFor(uint time): Allows the addresses capable of pausing to pause for "at most" time seconds if the contract is not paused already.

This action can be unpaused anytime by _unpause() or ONLY WHEN TIME HAS ELAPSED by _unpauseAfterPausedFor().

It is up to the project to decide if to allow earlier unpausing of _pauseFor() through _unapuse() or not. If not done the code will be paused until the time has elapsed.

Ideally the projects should make _unpauseAfterPausedFor() callable by anyone, this function allows unpausing actions started with pauseFor(uint time) after time seconds have passed.

Code changes :scroll:

0️⃣ The only state bool private _paused; now is uint256 private _pausedInfo; with the back-wards compatible structure explained earlier.

1️⃣ 3 functions, 1 event and 1 error added are:

• function _pauseFor(uint256 time) internal virtual whenNotPaused

• function _unpauseAfterPausedFor() internal virtual whenPaused

• function _unpauseDeadline() internal view virtual returns (uint256)

• event PausedFor(address account, uint256 duration);

• error PauseDurationNotElapsed();

2️⃣ SafeCast.sol library is now used to safely toUint248() when manipulating slot data-structure.

3️⃣ For readability when manipulating the slot, 2 constants have been added to the file:

• uint8 constant PAUSED = 1;
• uint8 constant PAUSE_DURATION_OFFSET = 8;

4️⃣ None already existing functions were deleted, they have only been modified to adapt to the new data structure in the slot.

5️⃣ New test cases have been added to account for new pauseFor() execution flows:

• pasuedFor and then unpaused with classic _unpause() interactions.
• pasuedFor and then unpaused with _unpauseAfterPausedFor() interactions.
• pausedFor and then classic _pause interactions.
• Classic _pause and then pauseFor interactions.
• Classic _pause and then _unpauseAfterPausedFor interactions.
• Classic _unpause() and then pauseFor interactions.
• Classic _unpause() and then _unpauseAfterPausedFor interactions.

[arr00]

This seems like a reasonable feature request to me. Feel free to make a PR and we will try to have more constructive comments there.

[Amxx]

This would probably be a good fit for a contribution to the community repository: https://github.com/openZeppelin/openzeppelin-community-contracts

[Amxx]

IMO _unpauseAfterPausedFor() should not exist. THe unpausing should be automatic (without an event)

uint48 private _pausedUntil;

function _pauseUntil(uint48 until) internal virtual whenNotPaused {
    // checks ?
    _pausedUntil = until;
    // emit event
}

function _unpause() internal virtual override {
    super._unpause(); // includes whenPaused
    _pausedUntil = 0;
}

function paused() public view virtual overide returns (bool) {
    return _paused || _pausedUntil > clock(); // from IERC6372
}

[CarlosAlegreUr]

Hi @Amxx and @arr00 thanks for the feedback and I'm glad my idea was deemed useful. 2 things.

• :one: First commenting on @Amxx opinion about deleting _unpauseAfterPausedFor(), I agree, it is much cleaner to add that OR gate on the paused() function.

Also I like the specifying the deadline instead of the duration of the pause, changing the name from pauseFor() to pauseUntil() would fit this criteria. (haven't changed that yet on my repo)

I've been developing a bit more to adapt to this improvements and also use the mentioned IERC6372. But I found myself a compatibility issue with some inheritance mock on that uses ERC721Votes and ERC721Pausable. You can see the issue in my latest commit de621e9. Link: Pausable.sol.

If I implement the clock like this it will give inheritance tree linearization errors at compile time for some mocks. So I thought of creating a new /utils/pausability/ directory where to add Pausable.sol with virtual non-implemented IERC6372 functions and another DefaultPausable.sol where we define the clock as one based on block.timestamps measured in seconds.

That is the cleanest solution I've thought so far on how to arrange the new Pausable code that involves time.

• :two: Should I continue the development and adjust the incompatibilities and issues and then PR this repo or should I PR to the community repo?

[Amxx]

Please make a PR to the community repo !

[CarlosAlegreUr]

Done :D