code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/d7ff917c8dad78bb6b95d65c8e21938fe3c84d6a/contracts/FollowNFT.sol#L103-L113 https://github.com/code-423n4/2023-07-lens/blob/d7ff917c8dad78bb6b95d65c8e21938fe3c84d6a/contracts/FollowNFT.sol#L156-L185

Vulnerability details

Impact

wrap after unfollownft is enabled , cause many problems

Proof of Concept

by design, wrap after unfollowed is not allowed,but it seems that it's possible due to lack of limitation.
poc below:

add below script in FollowNFTTest.t.sol


//forge test --match-test  testUnfollowAndWrap
    function testUnfollowAndWrap() public{
        uint256 followTokenId = followNFT.getFollowTokenId(alreadyFollowingProfileId);
//        vm.prank(alreadyFollowingProfileOwner);
        vm.prank(address(hub));
        followNFT.unfollow({
            unfollowerProfileId: alreadyFollowingProfileId,
            transactionExecutor: alreadyFollowingProfileOwner
        });

        vm.prank(alreadyFollowingProfileOwner);
        followNFT.wrap(followTokenId);

    }

it can cause many problems, for example If one user call function like this : follow -> basefollow（get tokenid） -> unfollow -> wrap(possible according to code ) -> burnprofile

this tokenId will be forever occupied , because the new follow logic will revert() in _followWithWrappedToken

Tools Used

manual

Recommended Mitigation Steps

add unfollow check in warp

Assessed type

Invalid Validation

donosonaumczuk commented 1 year ago

This is expected behaviour. We allow to recover the follow NFT after unfollowing when is unwrapped, because the unfollow is a operation allowed by delegated executors, and we don't want delegated executors to put any asset on risk.

However, only the owner of the follower profile can burn his own profile NFT, or any of the Follow NFTs he owns. And once he does that, there is no way back. Same with any other NFT or ERC-20, if you burn them, you can't recover them.

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as sponsor disputed

donosonaumczuk commented 1 year ago

Also, the reason why the wrap takes into account the "allowed to recovery" profile, is to have a better UX. So the user does not need to do multiple txs to achieve that same valid state.

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-07-lens-findings

wrap after unfollow is enabled #84