Reentrancy Concerns with External Calls in refreshBasket() function

[c4-bot-8]

Lines of code

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

Vulnerability details

Summary

The refreshBasket() function within the BasketHandlerP1 contract calls the external function assetRegistry.refresh(), which could interact with other contracts or components. Following this, the function _switchBasket() is called, which also potentially interacts with external components. If any of these external calls result in reentrant calls back into the BasketHandlerP1 contract, and if state changes occur before these external calls are finalized, there could be a vulnerability that allows reentrancy attacks.

Impact

If an attacker successfully exploits this vulnerability, they could manipulate the state of the contract, potentially bypassing key contract logic or triggering unintended actions that could disrupt the basket handling process or lead to financial loss.

Proof of Concept

To demonstrate the reentrancy concern, consider the following scenario:

Contract Setup:

Deploy the BasketHandlerP1 contract. Deploy a mock AssetRegistry contract that has a refresh() function which triggers a callback or reentrancy into the BasketHandlerP1 contract. Exploit Contract:

Create a contract that implements the IAssetRegistry interface and overrides the refresh() function to reenter the BasketHandlerP1 contract.

// SPDX-License-Identifier: MIT pragma solidity ^0.8.19;

import "../interfaces/IAssetRegistry.sol";

contract MaliciousAssetRegistry is IAssetRegistry { address public basketHandler;

constructor(address _basketHandler) {
    basketHandler = _basketHandler;
}

function refresh() external override {
    // Reentrancy attack
    BasketHandlerP1(basketHandler).disableBasket();
}

} Trigger Attack: Call refreshBasket() on the BasketHandlerP1 contract. The MaliciousAssetRegistry contract reenters BasketHandlerP1's disableBasket() function, potentially exploiting any vulnerabilities present there.

Tools Used

Manual Review

Recommended Mitigation

Implement reentrancy guards (nonReentrant modifier) on the refreshBasket() function or ensure that all state changes are completed before any external calls are made. Additionally, audit the assetRegistry.refresh() and _switchBasket() functions for potential reentrancy vulnerabilities.

Assessed type

Reentrancy