Pausable Contract

[zoruka]

As a step to improve the security of our NFA contract, we are willing to implement the Pause feature. That feature would allow us to pause some or all of the functions in specific situations. Imagining an occasion when some malicious intended person find some breach to exploit and get advantage over the contract, in this sense the Pause feature would provide us a way to reduce the possible damages to this ecosystem.

But, as everything, this feature comes with a tradeoff. The common implementations of Pause feature requires an entity to handle when the contract should be paused or resumed. This turn the contract to be susceptible to human error and gives a lot of control to Fleek or the contract owner. With that in mind, we are willing to get the community input about how we should have this in the contract.

For first, we need to define which strategies we must follow to achieve sustainable security losing as little as possible the reliability over the contract functions. For this we have to decide which functions should or should not be pausable. For an example, the first thoughts about it were:

Make pausable:

• Functions that manages the lifecycle of a NFA such as mint and burn;
• Functions that touch the market changes over the community such as transfer;
• Functions that changes the Access Control both for Collection and Token levels such as grantTokenRole and revokeTokenRole;
• Functions that manages the lifecycle of AccessPoints as addAccessPoint and removeAccessPoint.

Make not pausable:

• Any view function;
• Functions that update the token metadata such as setTokenName or setTokenDescription.

Within the thoughts about the functions being or not pausable, we also came up with the idea of having multiple levels of pause (e.g. pausing just one item from the pausable list above). Would be this worth it? Or should we have just a single flag that indicates if the contract is paused or not?

Over all being said we also want to get the community thoughts about which implementation to follow. Should we follow how OpenZeppelin has it or something else?

[jsonsivar]

Thank you for this summary! I think maybe for MVP we can go with a single global pause flag and like you said pause a set of functions all at once. Regarding the problem with putting to much control into the contract owners, it's not a full solution but what they suggest here at the end could help it: https://blog.logrocket.com/pause-functionality-secure-solidity-smart-contracts/. I.e., add a flag that could be controlled by governance whether or not the contract is pausable.

[jsonsivar]

Jotting down some notes based on our architecture conversation @zoruka

We discussed the idea of maybe creating another layer for pre-checks that brings in both access control and pausable and create require functions combining the two. This would move some of the access control functions like grantCollectionRole back outside of the main ERC721 contract and encapsulated in that new contract and added to ERC721.sol.

This diagram created by @zoruka is good IMO in terms of overall approach to making things modular:

I proposed we can combine this in a nested way to achieve the idea around a pre-checks layer:

We thought we'd put this discussion up so we can confirm this approach and subsequently add it to our backlog for a future version or if we have time, in the MVP.

[zoruka]

Just complementing: the idea is to implement all logic and storages through the modules and just have business rules applied over needed functions in the FleekERC721. Only function calls would be appear in the main contract.

[zoruka]

The second diagram would work for sure, but the issue I can see for it is in #114. There we would not have the token owner role over acl, what would lead to have to extend the ERC721 in the Pre-checks module what the owner can do or not.

[zoruka]

#110

[EmperorOrokuSaki]

Update: Closing the discussion due to inactivity.