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

0 stars 0 forks source link

`FollowNFT` storage collision #164

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The _lastFollowTokenId of FollowNFT contract has a storage collision. V2.0 storage layout:

| _lastFollowTokenId                | uint128                                         | 17   | 0      | 16    | contracts/FollowNFT.sol:FollowNFT |
| _followerCount                    | uint128                                         | 17   | 16     | 16    | contracts/FollowNFT.sol:FollowNFT |

Hence, the existing(v1.3) _tokenIdCounter will be interpreted as _lastFollowTokenId and _followerCount will be 0.

Proof of Concept

The previous FollowNFT implementation is different between v1.3 and v2.0. And

// 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;

link to the POC

How to run the POC:

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

output:

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

Recommended Mitigation Steps

Assessed type

Upgradable

code423n4 commented 1 year ago

Withdrawn by andy