Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Storage conflict vulnerability in upgradeable contracts: parent and child contracts repeatedly define __gap variables

wttt

Summary

In an upgradeable contract, if both the parent contract UpgradeableStorage and the child contract CapitalPoolStorage define a storage variable named __gap and their sizes are different, this will cause storage layout conflicts and potential problems.

Vulnerability Details

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/storage/UpgradeableStorage.sol#L12-L22

contract UpgradeableStorage {
/// @dev storage slot is 0
ITadleFactory public tadleFactory;
​
/**
* @dev This empty reserved space is put in place to allow future versions to add new
* variables without shifting down storage in the inheritance chain.
* See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
*/
uint256[49] private __gap;
}

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/storage/CapitalPoolStorage.sol#L12-L18

contract CapitalPoolStorage is UpgradeableStorage {
/// @dev empty reserved space is put in place to allow future versions to add new
/// variables without shifting down storage in the inheritance chain.
/// See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
/// start from slot 50, end at slot 149
uint256[100] private __gap;
}

forge inspect CapitalPoolStorage storage-layout

"storage": [
{
"astId": 34247,
"contract": "src/storage/CapitalPoolStorage.sol:CapitalPoolStorage",
"label": "tadleFactory",
"offset": 0,
"slot": "0",
"type": "t_contract(ITadleFactory)32654"
},
{
"astId": 34252,
"contract": "src/storage/CapitalPoolStorage.sol:CapitalPoolStorage",
"label": "__gap",
"offset": 0,
"slot": "1",
"type": "t_array(t_uint256)49_storage"
},
{
"astId": 34074,
"contract": "src/storage/CapitalPoolStorage.sol:CapitalPoolStorage",
"label": "__gap",
"offset": 0,
"slot": "50",
"type": "t_array(t_uint256)100_storage"
}
],

If a child contract redefines a __gap variable and a __gap variable already exists in the parent contract, this may cause storage conflicts.

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

Impact

The __gap variable in the parent contract occupies a specific storage slot.
The __gap variable in the child contract continues to occupy a new storage slot.
This duplicate definition may cause overlapping or conflicts in storage slots, especially when the contract is upgraded.

Tools Used

Manual review

Recommendations

To avoid these issues, ensure that the __gap variable is only defined in the parent contract and not redefined in the child contract. This way, the storage layout remains consistent and predictable, allowing for safe upgrades.

DeliveryPlaceStorage, PerMarketsStorage, SystemConfigStorage, TokenManagerStorage are the same.

Corrected Child Contract CapitalPoolStorage

contract CapitalPoolStorage is UpgradeableStorage {
//Upgraded storage
}

By following this approach, you can ensure that the storage layout remains consistent, preventing storage conflicts and ensuring the safe upgradeability of your contracts.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.