xiaoming90 - Storage slot used during migration might result in storage collision

[sherlock-admin]

xiaoming90

medium

Storage slot used during migration might result in storage collision

Summary

Storage collision might occur on the storage slot used during the migration if future iterations of Notional read or write to that slot.

Vulnerability Detail

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/patchfix/MigratePrimeCash.sol#L66

File: MigratePrimeCash.sol
40: contract MigratePrimeCash is BasePatchFixRouter, StorageLayoutV2 {
..SNIP..
66:     mapping(uint256 => MigrationSettings) internal _migrationSettings;

Let $x$ be the last storage slot of StorageLayoutV2. During migration, the implementation of the Notional proxy is temporarily switched to MigratePrimeCash contract.

The MigratePrimeCash contract introduces an additional mapping _migrationSettings at the storage slot $x+1$ to keep track of some migration data. The _migrationSettings mapping is not used after the migration.

However, the problem is that in the future iteration of Notional, if any of the contracts use slot $x + 1$, it will lead to storage collision and corrupted data might be read from slot $x + 1$.

Impact

If corrupted data is read from the slot where old data exists, this could lead to unexpected behavior. If the storage collision affects a slot that stores critical data, such as the address of an owner or admin of a contract or the balance of a particular user, it might potentially result in the loss of assets.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/patchfix/MigratePrimeCash.sol#L66

Tool used

Manual Review

Recommendation

Consider using an unstructured storage pattern for storing migration data or create a new StorageLayoutV3 to keep track of the storage slot used by _migrationSettings mapping to avoid potential storage collision in the future.

[jeffywu]

Although I think this is probably good practice, disagree with the severity here. The migration settings storage slot is actually set on the deployed MigratePrimeCash contract (not the main Notional V3 proxy) since they are set prior to the upgrade by calling the MigratePrimeCash contract directly and fetched via an external self call back to the MigratePrimeCash contract.

In practice, there would not be a storage slot collision against the main proxy because the storage slots are stored in different contracts (although that is not immediately obvious here).