Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/p1/RevenueTrader.sol#L109-L183

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/p1/RevenueTrader.sol#L109-L183

    function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds){
..snip

        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
            //@audit
            (, TradeRequest memory req) = TradeLib.prepareTradeSell(
                trade,
                minTradeVolume,
                maxTradeSlippage
            );
            require(req.sellAmount > 1, "sell amount too low");

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

This function is used to process a set of tokens, issue however is that for all ERC20s that isn't the tokenToBuy, there would be a need to start an auction of the given kind for that token, however the max slippage attached to this auction is not provided in a case by case pattern, but rather what has been attached as the maxTradeSlippage in RevenueTrader#init().

Impact

Ineffective trading logic, considering a hardcoded slippage has been applied to trades and would lead either of the two cases below:

Often reverts in the case where the slippage is too strict. (which translates to a DOS since users could accept a less strict value to have their tx succeed).
Loss of value for users $ wise when they can't specify their exact wanted slippage and have to open themselves to risk.

Recommended Mitigation Steps

Consider allowing slippages in a case by case pattern.

Assessed type

Context

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