no __gap variable in contracts marked upgradeable as seen in Lens V2 Social Layer Architecture diagram

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/namespaces/LensHandles.sol#L25 https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/namespaces/TokenHandleRegistry.sol#L19 https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/LensHub.sol#L45

Vulnerability details

Impact

The LensHandles, LensHub and TokenHandleRegistry contract are intended to be used as logic contracts with a proxy, but do not have a __gap variable. This would become problematic if a subsequent version was to inherit one of these contracts. If the derived version were to have storage variables itself and additional storage variables were subsequently added to the inherited contract, a storage collision would occur.

Proof of Concept

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/namespaces/LensHandles.sol#L25

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/namespaces/TokenHandleRegistry.sol#L19

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

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/base/LensHubStorage.sol#L14

Tools Used

manual review

Recommended Mitigation Steps

Consider appending a gap variable as the last storage variable to these upgradeable contracts, such that the storage slots sum up to a fixed amount (e.g. 50). This will proof any future storage layout changes to the base contract. Note that the gap variable space will need to be adjusted accordingly as subsequent versions include more storage variables, in order to maintain the fixed amount of slots (e.g. 50).

Assessed type

Upgradable

[c4-pre-sort]

141345 marked the issue as low quality report

[141345]

invalid

misunderstanding of upgradeable and logic contract.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-c

[Picodes]

Invalidating this issue considering the work and comments all around the codebase about storage upgrades by the dev team

[c4-judge]

Picodes marked the issue as grade-b