Inconsistent State in AssetRegistry Due to Modifiable _erc20s Array During Refresh

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L58-L69

Vulnerability details

The refresh() function in AssetRegistry is responsible for updating the state and price information of these assets to ensure the system operates with accurate data. However, there is a critical issue in the implementation of the refresh() function that can lead to inconsistencies in the AssetRegistry state, causing dependent functions to behave incorrectly and compromising the stability of the entire system.

Impact

• If an asset's refresh() function modifies the _erc20s array by registering or unregistering assets during the iteration, it will lead to an inconsistent state in the AssetRegistry. The length and contents of the _erc20s array will be different from what is expected, causing discrepancies in the system.

• The refresh() function may not refresh all the intended assets if the _erc20s array is modified during iteration. This means that some assets might not have their state and price information updated.

Proof of Concept

The issue can be observed in the following scenario.

• The AssetRegistry is initialized with an initial set of assets.

• The refresh() function is called to update the state and price information of the registered assets.

• During the iteration in the refresh() function, an asset's refresh() function is called.

• The asset's refresh() function modifies the _erc20s array by registering or unregistering assets.

• The iteration continues, but the length and contents of the _erc20s array have changed, causing inconsistencies.

• After the refresh() function completes, the AssetRegistry state is inconsistent, and dependent functions may behave incorrectly.

AssetRegistry.sol:L58-L69

function refresh() public {
    uint256 length = _erc20s.length();
    for (uint256 i = 0; i < length; ++i) {
        assets[IERC20(_erc20s.at(i))].refresh();
    }

    basketHandler.trackStatus();
    lastRefresh = uint48(block.timestamp);
}

Tools Used

Manual review

Recommended Mitigation Steps

Snapshot the _erc20s array before iterating over the _erc20s array in the refresh() function, create a snapshot of the array and use the snapshot for the iteration. The AssetRegistry to ensure that asset registration and unregistration cannot happen within an asset's refresh() function. Instead, provide separate functions for asset registration and unregistration that can only be called by authorized entities or governance.

diff --git a/contracts/p1/AssetRegistry.sol b/contracts/p1/AssetRegistry.sol
index 1234567..8901234 100644
--- a/contracts/p1/AssetRegistry.sol
+++ b/contracts/p1/AssetRegistry.sol
@@ -58,10 +58,10 @@ contract AssetRegistryP1 is ComponentP1, IAssetRegistry {
     function refresh() public {
         // It's a waste of gas to require notPausedOrFrozen because assets can be updated directly
         // Assuming an RTokenAsset is registered, furnace.melt() will also be called
-        uint256 length = _erc20s.length();
-        for (uint256 i = 0; i < length; ++i) {
-            assets[IERC20(_erc20s.at(i))].refresh();
+        IERC20[] memory erc20Snapshot = _erc20s.values();
+        uint256 length = erc20Snapshot.length;
+        for (uint256 i = 0; i < length; ++i) {
+            assets[erc20Snapshot[i]].refresh();
         }

         basketHandler.trackStatus();
         lastRefresh = uint48(block.timestamp); // safer to do this at end than start, actually

The purpose of creating a snapshot of the _erc20s array is to ensure that any modifications to the original _erc20s array during the iteration do not affect the iteration process. By using the snapshot, we can safely iterate over the assets and refresh them without worrying about the array being modified concurrently.

Assessed type

Loop