Malicious actors can exploit stale prices to manipulate trades (`RevenueTraderP1::manageTokens`)

[c4-bot-3]

Lines of code

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

Vulnerability details

Description

The RevenueTraderP1 contract implements a manageTokens() function responsible for managing and trading a list of ERC20 tokens. This function refreshes asset prices and executes trades based on these prices. The current implementation refreshes prices for all assets if RToken is involved or for specific assets being traded at the beginning of the function. It then caches the price of the asset to buy (assetToBuy) and proceeds to execute trades in a loop using this cached price.

The core issue lies in the fact that the prices of assets being sold are not refreshed again within the trade execution loop. This design creates an issue where the cached prices may become stale during the execution of multiple trades, especially if the transaction takes a significant amount of time to process.

The problematic section of the code is as follows:

function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds)
    external
    nonReentrant
    notTradingPausedOrFrozen
{
    // ... (previous code)

    // Cache assetToBuy
    IAsset assetToBuy = assetRegistry.toAsset(tokenToBuy);

    // Refresh everything if RToken is involved
    if (involvesRToken) assetRegistry.refresh();
    else {
        // Otherwise: refresh just the needed assets and nothing more
        for (uint256 i = 0; i < len; ++i) {
            assetRegistry.toAsset(erc20s[i]).refresh();
        }
        assetToBuy.refresh(); // invariant: can never be the RTokenAsset
    }

    // Cache and validate buyHigh
    (uint192 buyLow, uint192 buyHigh) = assetToBuy.price(); // {UoA/tok}
    require(buyHigh != 0 && buyHigh != FIX_MAX, "buy asset price unknown");

    // For each ERC20 that isn't the tokenToBuy, start an auction of the given kind
    for (uint256 i = 0; i < len; ++i) {
        // ... (trade execution logic using cached prices)
    }
}

This implementation can lead to several issues:

1. Stale Prices: If the execution of trades takes a significant amount of time, the cached price of assetToBuy may become outdated, potentially leading to trades being executed at unfavorable rates.
2. Inconsistent State: The price of assetToBuy is cached once, but the prices of assets being sold are not refreshed within the loop. This could lead to inconsistencies between the buy and sell prices used in different trades within the same transaction.
3. Front-running : The gap between price refresh and trade execution creates an opportunity for front-running attacks. An attacker could observe the price refresh transaction and quickly execute their own transactions to take advantage of the known upcoming trades.

In the worst-case scenario, a malicious actor could exploit this vulnerability to manipulate trades and extract value from the system, potentially leading to significant financial losses for the protocol and its users.

Impact

The identified issue can lead to trades being executed at less favorable rates due to stale prices, inconsistencies between buy and sell prices, and front-running attacks. An attacker could exploit the window between price refresh and trade execution, leading to significant financial losses. This undermines the intended functionality of the manageTokens function and can result in suboptimal trade execution and financial losses for the contract.

Proof of Concept

1. An attacker observes the manageTokens() function being called with a list of multiple ERC20 tokens to trade.
2. The function refreshes prices at the beginning of its execution.
3. The attacker quickly executes a series of trades in the open market to manipulate the prices of the assets involved.
4. As the manageTokens() function continues to execute trades using the initially cached prices, it does so at rates that are now unfavorable due to the attacker's market manipulation.
5. The attacker then reverses their trades, profiting from the price discrepancy while the protocol incurs losses.

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, it's recommended to refresh prices immediately before each trade execution. This ensures that the most up-to-date prices are used for each trade, significantly reducing the risk of stale prices and front-running attacks.

Here's a proposed fix:

function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds)
    external
    nonReentrant
    notTradingPausedOrFrozen
{
    // ... (previous code)

    // For each ERC20 that isn't the tokenToBuy, start an auction of the given kind
    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");

+       // Refresh prices for this specific trade
+       IAsset assetToSell = assetRegistry.toAsset(erc20);
+       assetToSell.refresh();
+       assetToBuy.refresh();

+       (uint192 sellLow, uint192 sellHigh) = assetToSell.price(); // {UoA/tok}
+       (uint192 buyLow, uint192 buyHigh) = assetToBuy.price(); // {UoA/tok}
+       require(buyHigh != 0 && buyHigh != FIX_MAX, "buy asset price unknown");

        // ... (continue with trade execution logic using freshly fetched prices)
    }
}

This modification ensures that prices are refreshed immediately before each trade, reducing the risk of using stale prices and making the contract more resistant to front-running attacks.

Assessed type

Other