ERC721Upgradeable setApprovalForAll should be overriden in BorrowedNFT

[Mikerah]

Severity: Informational

In BorrowedNFT, the isApproveForAll method is overriden to allow for the rental contract to be able to make transfers without explicit approval. However, this pattern doesn't allow you to explicitly revoke the rental contract in case that it becomes malicious (e.g. someone gets access to one of the manager roles for the rental protocol contract). Instead, we recommend to explicitly approve the rental protocol in setApprovalForAll. In the case that the rental protocol contract is compromised, setApprovalForAll can be called to revoke approvals for that rental protocol contract. Then, the isApproveForAll can keep its default behavior as per the original ERC721Upgradeable implementation.

[vincentlg]

When a sublease ends, the protocol transfers the borrowed NFT from the subtenant to the tenant. This is the only case where the override of this function is necessary. Following your observations, we have looked without success for another way of doing things. The proposed solution (setApprovalForAll) has an unwanted side effect: The owner could remove the tenant's right to move the NFT and this breaks a rule of the protocol: The subtenant cannot prevent the end of a sublease.

[Mikerah]

In the case of a malicious rental protocol, would it make sense to break a subtenant's lease? In the case that the subtenant is not malicious, then this invariant (i.e. the subtenant cannot prevent the end of the sublease) should still hold. If the subtenant is also malicious, then breaking this invariant might make sense.

Let me think more about this and I'll have a new proposal.

[Mikerah]

Here are some options I'm mulling over:

• Leave the defaults from ERC721Upgradeable for setApprovalForAll and isApprovedForAll. This would require the BorrowedNFT implementation to always explicitly approve the rental protocol contract. The main con with this approach is that there is extra gas incurred on behalf of the BorrowedNFT user each time they need to approve the rental contract. The main pro of this approach is that you can easily block a malicious rental protocol contract but this is at the expense of potentially breaking a sublease
• Leave the current implementation as it is and simply be very explicit to users about the threat model of the contract with respect to a potential malicious rental protocol contract
• Apply my previous suggestion i.e. modify setApprovalForAll. But as mentioned, this breaks an important protocol invariant.
• Create a factory contract with fine-grained access control that handles the creation of rental protocol contracts. Then, if a rental protocol contract is found to be malicious, that contract can be revoked/paused from the factory. Further, within the rental protocol contract itself, users should be able to cancel rentals and get refunded accordingly. This should be the only time a subtenant can cancel a sublease as the rental protocol can no longer be trusted from their end as well.

I'm thinking that the last option would be best considering that Cometh is still new and will need to iron out the protocol over several iterations until it can be completely permissionless.

[jeje]

We prefer to stick to our current approach right now, but may challenge this decision later.