Inconsistent Last Refresh Timestamp Due to Order of Operations in AssetRegistry.refresh()

[c4-bot-7]

Lines of code

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

Vulnerability details

There is an issue with the order of operations in the refresh() function lastRefresh variable, which stores the timestamp of the last successful refresh, which is updated after calling basketHandler.trackStatus(). This can lead to inconsistencies if basketHandler.trackStatus() reverts due to an exception.

AssetRegistry.refresh()

function refresh() public {
    // ...

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

Impact

If basketHandler.trackStatus() reverts, the lastRefresh variable will not be updated. lastRefresh will not reflect the actual timestamp of the last successful refresh, leading to an inconsistent state within the AssetRegistry contract.

If issues arise related to the lastRefresh value, it may be challenging to identify and debug the root cause, as the value may not accurately represent the last successful refresh timestamp.

Proof of Concept

Ccenario:

1. The refresh() function is called on the AssetRegistry contract.
2. The function iterates over the registered assets and calls refresh() on each asset.
3. After the loop, basketHandler.trackStatus() is called.
4. If basketHandler.trackStatus() encounters an exception and reverts, the execution of refresh() will be halted.
5. As a result, the lastRefresh variable will not be updated with the current block timestamp.

This scenario demonstrates how the lastRefresh variable can remain outdated if basketHandler.trackStatus() reverts.

Tools Used

Manual review

Recommended Mitigation Steps

The lastRefresh update should be moved before the call to basketHandler.trackStatus(). This ensures that lastRefresh is always updated with the current block timestamp, even if basketHandler.trackStatus() reverts.

function refresh() public {
    // ...

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

Updating lastRefresh before calling basketHandler.trackStatus(), we guarantee that the lastRefresh value accurately reflects the timestamp of the last successful refresh, regardless of any exceptions that may occur in basketHandler.trackStatus().

Assessed type

Context