StableSwapFactory.sol is intended to be upgraded in the future via the Proxy pattern. This can be seen from the deployment script and also the sponsor has confirmed it in the public chat:
For upgradeable contracts, there must be a storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise, it may be very difficult to write new implementation code and storage collisions may occur.
Without a storage gap, the variable in the child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences for the child contracts.
Attack Scenario\
Scenario
1) The protocol team deploys a new version of StableSwapFactory.sol-> StableSwapFactoryV2.sol which inherits StableSwapFactory
2) At some point the team finds a vulnerability in the base contract(StableSwapFactory) and releases a fix which includes new/updated storage variables
3) The next deployments of StableSwapFactoryV2 will have storage collisions with the latest version of StableSwapFactory due to the missing storage gap in StableSwapFactory
Attachments
Proof of Concept (PoC) File
Same as in Description section
Revised Code File (Optional)
Introduce storage gaps insde StableSwapFactory.sol as recommended from the OpenZeppelin docs (Storage Gaps section)
Github username: -- Twitter username: -- Submission hash (on-chain): 0x3e11c5baea67ac1840f347b2be29d994bbe7bb9035ef1cf3d4d4c105d8118970 Severity: low
Description: Description\
StableSwapFactory.solis intended to be upgraded in the future via the Proxy pattern. This can be seen from the deployment script and also the sponsor has confirmed it in the public chat:https://github.com/hats-finance/Thorn-protocol-0x1286ecdac50215a366458a14968fbca4bd95067d/blob/main/src/deploy-upgrades/1-deploy-stable-swap-factory.ts#L16-L20
For upgradeable contracts, there must be a storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise, it may be very difficult to write new implementation code and storage collisions may occur.
Without a storage gap, the variable in the child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences for the child contracts.
Attack Scenario\ Scenario
1) The protocol team deploys a new version of
StableSwapFactory.sol->StableSwapFactoryV2.solwhich inherits StableSwapFactory 2) At some point the team finds a vulnerability in the base contract(StableSwapFactory) and releases a fix which includes new/updated storage variables 3) The next deployments ofStableSwapFactoryV2will have storage collisions with the latest version ofStableSwapFactorydue to the missing storage gap inStableSwapFactoryAttachments
Same as in Description section
Introduce storage gaps insde StableSwapFactory.sol as recommended from the OpenZeppelin docs (Storage Gaps section)