code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The previous FollowNFT implementation is different between v1.3 and v2.0. The _followerCount is 0 for existing FollowNFT(even if _lastFollowTokenId is not 0) after upgrade.

// Old uint256 `_lastFollowTokenId` slot splitted into two uint128s to include `_followerCount`.
uint128 internal _lastFollowTokenId;
// `_followerCount` will not be decreased when a follower profile is burned, making the counter not fully accurate.
// New variable added in V2 in the same slot, lower-ordered to not conflict with previous storage layout.
uint128 internal _followerCount;

It is possible that _followerCount-- can underflow(suppose the migration is not completed).

unchecked {
    // This is safe, as this line can only be reached if the unfollowed profile is being followed by the
    // unfollower profile, so _followerCount is guaranteed to be greater than zero.
    _followerCount--;
}

Proof of Concept

I submitted this in the last minute. Sorry that I do not have a POC.

link to the test to test the value of _followerCount.

How to run the test:

forge test -vvv --match-path ./test/GovernanceFunctions.t.sol --match-test testFollowNFT_forkUpgrade --fork-url "https://rpc.ankr.com/polygon"

_tokenIdCounter v1.3 is 0x55(85 decimal).

output:

[PASS] testFollowNFT_forkUpgrade() (gas: 37920)
Logs:
  v2.0 getFollowerCount: 0
  v2.0 _lastFollowTokenId: 85

Recommended Mitigation Steps

N/A

Assessed type

Upgradable

donosonaumczuk commented 1 year ago

Does not apply, as to unfollow you need to be following, which makes the _followerCount > 0. Becuase the _followerCount is incremented when migrating too.

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

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