Malicious actors can exploit race condition in `RevenueTraderP1::returnTokens()` and `RevenueTraderP1::manageTokens()` to cause inconsistent state and potential loss of funds

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RevenueTrader.sol#L76-L93 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RevenueTrader.sol#L109-L184

Vulnerability details

Description

The RevenueTraderP1 contract implements two key functions, returnTokens() and manageTokens(), which are responsible for managing token balances and executing trades. These functions operate on the same set of tokens but lack proper synchronization mechanisms, potentially leading to race conditions and inconsistent contract states.

The returnTokens() function is designed to return specified ERC20 tokens to the BackingManager:

function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
    // ... (check distribution totals)
    for (uint256 i = 0; i < len; ++i) {
        require(assetRegistry.isRegistered(erc20s[i]), "unregistered erc20");
        erc20s[i].safeTransfer(address(backingManager), erc20s[i].balanceOf(address(this)));
    }
}

On the other hand, the manageTokens() function prepares and executes trades based on the current token balances:

function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds)
    external
    nonReentrant
    notTradingPausedOrFrozen
{
    // ... (prepare trades)
    for (uint256 i = 0; i < len; ++i) {
        IERC20 erc20 = erc20s[i];
        if (erc20 == tokenToBuy) continue;

        require(address(trades[erc20]) == address(0), "trade open");
        require(erc20.balanceOf(address(this)) != 0, "0 balance");

        // ... (execute trade)
    }
}

The root cause of the issue lies in the lack of synchronization between these two functions. If returnTokens() is called while manageTokens() is in the process of preparing a trade, it can lead to inconsistent state and potential loss of funds. This is because manageTokens() might be operating on outdated balance information if tokens are transferred out by returnTokens() during its execution.

Impact

The race condition between returnTokens() and manageTokens() can lead to failed trades, trades executed with incorrect amounts, or inconsistent contract states. This issue could be exploited by malicious actors to manipulate token balances and potentially cause financial losses to the protocol or its users.

In a worst-case scenario, an attacker could time their transactions to consistently disrupt trading operations, leading to a denial-of-service condition for the trading functionality. This could undermine the core functionality of the RevenueTraderP1 contract and erode user trust in the protocol.

Proof of Concept

1. Alice calls manageTokens() to initiate a trade for TokenA.
2. manageTokens() checks the balance of TokenA and starts preparing the trade.
3. Before the trade is executed, Bob calls returnTokens() for TokenA.
4. returnTokens() transfers all TokenA back to the BackingManager.
5. manageTokens() attempts to execute the trade with the now outdated balance information.
6. The trade fails or executes with an incorrect amount, leading to potential loss of funds or inconsistent state.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, implement a locking mechanism to ensure atomic execution of returnTokens() and manageTokens(). This can be achieved by introducing a mutex for each token:

+ mapping(IERC20 => bool) private tokenLock;

+ modifier lockToken(IERC20 token) {
+     require(!tokenLock[token], "Token is locked");
+     tokenLock[token] = true;
+     _;
+     tokenLock[token] = false;
+ }

- function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
+ function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
    // ... (check distribution totals)
    for (uint256 i = 0; i < len; ++i) {
+       lockToken(erc20s[i]);
        require(assetRegistry.isRegistered(erc20s[i]), "unregistered erc20");
+       require(address(trades[erc20s[i]]) == address(0), "trade open");
        erc20s[i].safeTransfer(address(backingManager), erc20s[i].balanceOf(address(this)));
    }
}

function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds)
    external
    nonReentrant
    notTradingPausedOrFrozen
{
    // ... (prepare trades)
    for (uint256 i = 0; i < len; ++i) {
        IERC20 erc20 = erc20s[i];
        if (erc20 == tokenToBuy) continue;

+       lockToken(erc20);
        require(address(trades[erc20]) == address(0), "trade open");
        require(erc20.balanceOf(address(this)) != 0, "0 balance");

        // ... (execute trade)
    }
}

Assessed type

Other