fix: chiado

[filoozom]

As the "broken" withdrawal contract was already deployed to Chiado, it is essential to keep the storage layout intact on the testnet. I propose to keep these changes in a chiado branch for now, so that we don't needlessly bloat the mainnet contract.

I moved all the variables up to be before the constructor, because having them sprinkled around the entire contract will make upgrades more complicated.

Presumably the private constant and struct don't change the storage layout, but confirmation for this would be appreciated (cc @Barichek).

This is based on #45 for now but should be kept in sync with master once the former is merged.

[dapplion]

Starting to count after the variables of the pre-withdrawal contract (this version https://github.com/gnosischain/deposit-contract/blob/c7217fccac3049901f78547f4024127fa1dcdcd4/contracts/SBCDepositContract.sol)

Legacy withdrawal version deployed in chiado

0:    address private constant SYSTEM_WITHDRAWAL_EXECUTOR = 0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE;
1:    uint256 private constant DEFAULT_GAS_PER_WITHDRAWAL = 300000;
2:    mapping(uint256 => FailedWithdrawalRecord) public failedWithdrawals;
3:    mapping(uint64 => uint256) public failedWithdrawalIndexByWithdrawalIndex;
4:    uint256 public numberOfFailedWithdrawals;
5:    uint64 public nextWithdrawalIndex;

New withdrawal version

0:    address private constant SYSTEM_WITHDRAWAL_EXECUTOR = 0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE;
1:    mapping(address => uint256) public withdrawableAmount;

Storage slot at index 1 cannot have collisions because in the new version the slot is computed with hash(1 | address) while in the legacy version is hash(1). Only storage slots hash(4) and hash(5) have the potential risk of collisions. But we can revisit this problem latter if we need to store a non-map variable in those slots. If that's the case the slots can be zero-ed out in an intermediary update

[filoozom]

I think that the private constant values aren't actually stored in slots, meaning that mapping(uint256 => FailedWithdrawalRecord) public failedWithdrawals; and mapping(address => uint256) public withdrawableAmount; would be in the same slot.

From my understanding, both the addresses and integers are left padded with zeroes, and it is technically possible to have a clash for these, although not very likely as it would only concern the first 3_826_127 (right now, going to increase until we upgrade the contract or fund it with GNO) addresses at this point according to https://blockscout.com/gnosis/chiado/address/0xb97036A26259B7147018913bD58a774cf91acf25/read-proxy#address-tabs. So technically, if anyone found an address with a lot of leading zeroes, it could break, however unlikely this may be.

I'd say that it's probably fine for Chiado, but not "perfect".

[4rgon4ut]

I looked up a git and here are storage layouts:
Old version

│  SBCDepositContractOld  │                _paused                 │      0       │                              t_bool                               │       1       │
│  SBCDepositContractOld  │              zero_hashes               │      1       │                   t_array(t_bytes32)32_storage                    │     1024      │
│  SBCDepositContractOld  │                 branch                 │      33      │                   t_array(t_bytes32)32_storage                    │     1024      │
│  SBCDepositContractOld  │             deposit_count              │      65      │                             t_uint256                             │      32       │
│  SBCDepositContractOld  │    validator_withdrawal_credentials    │      66      │              t_mapping(t_bytes_memory_ptr,t_bytes32)              │      32       │
│  SBCDepositContractOld  │           failedWithdrawals            │      67      │ t_mapping(t_uint256,t_struct(FailedWithdrawalRecord)2627_storage) │      32       │
│  SBCDepositContractOld  │ failedWithdrawalIndexByWithdrawalIndex │      68      │                   t_mapping(t_uint64,t_uint256)                   │      32       │
│  SBCDepositContractOld  │       numberOfFailedWithdrawals        │      69      │                             t_uint256                             │      32       │
│  SBCDepositContractOld  │          nextWithdrawalIndex           │      70      │                             t_uint64                              │       8       │
│  SBCDepositContractOld  │        failedWithdrawalsPointer        │      71      │                             t_uint256                             │      32       |

Current PR version

│   SBCDepositContract    │                _paused                 │      0       │                              t_bool                               │       1       │
│   SBCDepositContract    │              zero_hashes               │      1       │                   t_array(t_bytes32)32_storage                    │     1024      │
│   SBCDepositContract    │                 branch                 │      33      │                   t_array(t_bytes32)32_storage                    │     1024      │
│   SBCDepositContract    │             deposit_count              │      65      │                             t_uint256                             │      32       │
│   SBCDepositContract    │    validator_withdrawal_credentials    │      66      │              t_mapping(t_bytes_memory_ptr,t_bytes32)              │      32       │
│   SBCDepositContract    │           withdrawableAmount           │      67      │                  t_mapping(t_address,t_uint256)                   │      32       │

The most common recommendation about upgrades is to follow the initial storage layout as much as possible (e.g. certik recommendations)

I personally see no reason to step aside from initial layout and create additional unsecure/uncertain point which should be 'treated specifically' as we can easily make it in more transparent and straightforward way.
I propose to keep this variables as is and add new following the order or add gap with comprehensive description right after the validator_withdrawal_credentials:
e.g.

// This gap holds out deprecated storage slots.
// New state variables MUST be added AFTER this gap to follow historical storage layout.
uint256[5] private _deprecated_slots_gap;

This gap will create such storage layout:

│   SBCDepositContract    │                _paused                 │      0       │                              t_bool                               │       1       │
│   SBCDepositContract    │              zero_hashes               │      1       │                   t_array(t_bytes32)32_storage                    │     1024      │
│   SBCDepositContract    │                 branch                 │      33      │                   t_array(t_bytes32)32_storage                    │     1024      │
│   SBCDepositContract    │             deposit_count              │      65      │                             t_uint256                             │      32       │
│   SBCDepositContract    │    validator_withdrawal_credentials    │      66      │              t_mapping(t_bytes_memory_ptr,t_bytes32)              │      32       │
│   SBCDepositContract    │         _deprecated_slots_gap          │      67      │                    t_array(t_uint256)5_storage                    │      160      │
│   SBCDepositContract    │           withdrawableAmount           │      72      │                  t_mapping(t_address,t_uint256)                   │      32       │

This approach is more safe, its less likely will make us revisit this topic again in future and also it will make code self descriptive as historical storage changes will be highlighted and explained.

[dapplion]

So technically, if anyone found an address with a lot of leading zeroes, it could break, however unlikely this may be.

I'd say that it's probably fine for Chiado, but not "perfect".

The chance of this happening within our lifetime is on the order of finding a specific atom in the ocean. So it should not be used as an argument for this case.

This approach is more safe, its less likely will make us revisit this topic again in future and also it will make code self descriptive as historical storage changes will be highlighted and explained.

This is a very valid reason. However it's annoying to have to pollute such a key contract in the network with a chiado-specific issue.

Why don't we do this:

• Open a branch and a function to the contract that sets to zero the rogue storage slots
• Deploy this contract to chiado
• Effectively the issue is solved, we can forget about it, and have a clean contract in mainnet