code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L131

Vulnerability details

Impact

In FollowNFT.sol, approve() function is used to allow other users (not the owner) to be able to use the privileges of owning the followTokenId but at the same time for the owner not to sell it. removeFollower(), however, provides functionality to remove anybody who is following current followTokenId (it's either the owner itself or somebody approved). This makes approve() function almost useless (see PoC).

Proof of Concept

Imagine the following scenario:

Alice has followTokenId of Bob's FollowNFT collection and this tokenId is among the first 10 ones that basically allows Alice to get some premium content from Bob. That said, the only reason for the owner (Alice) to make an approval for somebody is just to give another user an ability to get this content from Bob. And that could be only done by off-chain agreements conducted between the owner and the third party (that's like renting the token and paying fixed amount for it) - let's say, another person, Carol. But all these agreements can be easily disrupted by either untrustworthy third party (for example, if Alice and Carol agreed that the payment for the content will be at the end of the month, and the payment didn't happen) or by calling removeFollower() by Alice (this is most likely the case as Alice will get her money from the Carol first and then call approve() ) that allows Alice to remove the third party immediately :

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L131-138

The other case that can be considered is that Carol can first pay 50% then Alice will call approve() and then Carol will pay other 50%. But Alice can still remove Carol as follower almost instantly and his approval will be nullified (after he called follow() ).

Apart from allowing to "rent" the followTokenId, it's hard to find any usage for the approve() function for the owner as he doesn't have any economic incentives to do so.

Tools Used

Manual review.

Recommended Mitigation Steps

Reconsider the logic that allows the owner by calling removeFollower() to remove his follower immediately.

Assessed type

Other

vicnaum commented 1 year ago

This kind of agreement logic can be implemented using a separate rental hub contract which doesn't allow to remove the follower until the rent is done, or have some different logic. The protocol allows these functions as simple primitives, but additional logic and agreements should be coded as separate contracts, and not implemented in the protocol itself (because it allows more flexibility).

c4-sponsor commented 1 year ago

vicnaum marked the issue as sponsor disputed

Picodes commented 1 year ago

This doesn't seems like a valid report as it discusses the utility of the function in a specific context and not a security issue.

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Out of scope

rodiontr commented 1 year ago

Hi @Picodes ! Would really appreciate it if you'd take a look at my explanation:

I think this issue is protocol-specific as we should consider approve() functionality in the context of the protocol type it’s used in. The issue and issue #78 highlighted approve() function’s only use cases in the protocol:

1) The owner of the tokenId can approve his token to another profileId that he has created to avoid being blocked. This was considered by the sponsors in my another issue as expected behavior: “in every social media, users can create new accounts and follow with them”. But in this scenario, approve() just exists so the owner can avoid being blocked ?

2) Another use case is that the owner approves his tokenId to another user so he can use it himself. But this means that this particular tokenId has significant value (or why the user even wants this tokenId if he can just follow with the new profileId and still be the follower ?) and the user who has been approved can use to get some privileges of owning it. And sponsors described this functionality as basically “renting”. The problem is that every actor in the system is untrustworthy meaning that the user who’s been approved need to trust the owner so that he wouldn’t use removeFollower() right away after approving the another user to rent for some money (or what’s the incentive for the owner to give another user his tokenId for free ?). Therefore, there will be no incentives for the users to try this “renting” as they can be removed immediately if the owner calls removeFollower().

I think this is a serious issue as it describes improper implementation of one of the key functions in FollowNFT contract - approving - that makes this function useless. This issue and issue #78 can be considered as one issue or two separate issues (see comment in issue #78 on the differences).

I would suggest to create some separate logic (such as rental hub as mentioned by the sponsor) where some tokens will be possible to be rented if only the money are transferred to the owner and after that the appropriate function is triggered that gives an approval (with approvalTime) to the user.

Thank you again for reviewing!