Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/RevenueTrader.sol#L161

Vulnerability details

Impact

Detailed description of the impact of this finding. There is no price check in manageTokens. There is no check for buyLow and there is no check for sellLow and sellHigh.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds) external nonReentrant notTradingPausedOrFrozen { uint256 len = erc20s.length; require(len != 0, "empty erc20s list"); require(len == kinds.length, "length mismatch"); RevenueTotals memory revTotals = distributor.totals(); require( (tokenToBuy == rsr && revTotals.rsrTotal != 0) || (address(tokenToBuy) == address(rToken) && revTotals.rTokenTotal != 0), "zero distribution" );

    // Calculate if the trade involves any RToken
    // Distribute tokenToBuy if supplied in ERC20s list
    bool involvesRToken = tokenToBuy == IERC20(address(rToken));
    for (uint256 i = 0; i < len; ++i) {
        if (erc20s[i] == IERC20(address(rToken))) involvesRToken = true;
        if (erc20s[i] == tokenToBuy) {
            _distributeTokenToBuy();
            if (len == 1) return; // return early if tokenToBuy is only entry
        }
    }

    // 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) {
        IERC20 erc20 = erc20s[i];
        if (erc20 == tokenToBuy) continue;

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

        IAsset assetToSell = assetRegistry.toAsset(erc20);
        (uint192 sellLow, uint192 sellHigh) = assetToSell.price(); // {UoA/tok}

        TradeInfo memory trade = TradeInfo({
            sell: assetToSell,
            buy: assetToBuy,
            sellAmount: assetToSell.bal(address(this)),
            buyAmount: 0,
            prices: TradePrices(sellLow, sellHigh, buyLow, buyHigh)
        });

        // Whether dust or not, trade the non-target asset for the target asset
        // Any asset with a broken price feed will trigger a revert here
        (, TradeRequest memory req) = TradeLib.prepareTradeSell(
            trade,
            minTradeVolume,
            maxTradeSlippage
        );
        require(req.sellAmount > 1, "sell amount too low");

        // Launch trade
        tryTrade(kinds[i], req, trade.prices);
    }
}

Tools Used

Recommended Mitigation Steps

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

(uint192 sellLow, uint192 sellHigh) = assetToSell.price(); // {UoA/tok}

require(sellHigh != 0 && sellHigh != FIX_MAX, "buy asset price unknown"); require(sellLow != 0 && sellLow != FIX_MAX, "buy asset price unknown");

Assessed type

Context

code-423n4 / 2024-07-reserve-validation

NO check for the price in manageTokens #201