In Broker.sol, Rounding down in wrong direction will cause leakage of value

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Leakage of Value in the long term, applying more slippage than intended

• whenever we exceed GNOSIS_MAX_TOKENS

Proof of Concept

The problem arises in Broker::newBatchAuction

File: Broker.sol
243:     function newBatchAuction(TradeRequest memory req, address caller) private returns (ITrade) {
////////////////
249:         // Apply Gnosis EasyAuction-specific resizing of req, if needed: Ensure that
250:         // max(sellAmount, minBuyAmount) <= maxTokensAllowed, while maintaining their proportion
251:         uint256 maxQty = (req.minBuyAmount > req.sellAmount) ? req.minBuyAmount : req.sellAmount;
252: 
253:         if (maxQty > GNOSIS_MAX_TOKENS) {
254:             req.sellAmount = mulDiv256(req.sellAmount, GNOSIS_MAX_TOKENS, maxQty, CEIL);
255:             req.minBuyAmount = mulDiv256(req.minBuyAmount, GNOSIS_MAX_TOKENS, maxQty, FLOOR);
256:         }

in Line 253 we check if any of the trading amounts are bigger than GNOSIS_MAX_TOKENS to lower the tokens amounts while preserving the Ratio of both (which is not true)

• in Line 254 we round up (CEIL) the sellAmount the tokens that the protocol will provide to the contract, while in the same time in Line 255 we round down (FLOOR) minBuyAmount Tokens which will be sent to the protocol as a minimum after the trade settled
  • This will cause the ratio between both tokens decrease and More slippage applied bigger than maxTradeSlippage

Potentially leaking value in the long run every time GNOSIS_MAX_TOKENS is exceeded

Note!: exceeding GNOSIS_MAX_TOKENS is not hard for tokens = 21 decimals

Tools Used

Manual Review

Recommended Mitigation Steps

Revert the rounding direction to be in favor of the protocol

Assessed type

Math

[Al-Qa-qa]

In this report i showed how this problem will cause long term leakage of value:

1. as said they round the sold amount (the amounts given from the protocol) up, "to have larger value"
2. the minBuyAmount rounded down (the amount taken to protocol from auction finalization as revenue for example)

the comments says that we scale the amounts down if it exceeds GNOSIS_MAX_TOKENS while preserving the ratio (to preserve the slippage percentage set at the trading contract):

now due to different rounding direction this will cause the ratio to increase (slippage will increase) exceeding max slippage set at trade initiation

this will cause leakage of value on the long run on every trade that has any amount exceeding GNOSIS_MAX_TOKENS (which is not rare since 21 decimal tokens are in scope)

[thereksfour]

There is no loss. minBuyAmount is used to calculate worstCasePrice, not the actual trade price. The actual trade price is determined by the auction results.

        worstCasePrice = shiftl_toFix(req.minBuyAmount, 9).divu(req.sellAmount, FLOOR);

[Al-Qa-qa]

File: TradeLib.sol
118:     function prepareTradeToCoverDeficit(
119:         TradeInfo memory trade,
120:         uint192 minTradeVolume,
121:         uint192 maxTradeSlippage
122:     ) internal view returns (bool notDust, TradeRequest memory req) {
123:         assert(
124:             trade.prices.sellLow != 0 &&
125:                 trade.prices.sellLow != FIX_MAX &&
126:                 trade.prices.buyHigh != 0 &&
127:                 trade.prices.buyHigh != FIX_MAX
128:         );
129: 
130:         // Don't buy dust.
131:         trade.buyAmount = fixMax(
132:             trade.buyAmount,
133:             minTradeSize(minTradeVolume, trade.prices.buyHigh)
134:         );
135: 
136:         // {sellTok} = {buyTok} * {UoA/buyTok} / {UoA/sellTok}
137:         uint192 exactSellAmount = trade.buyAmount.mulDiv(
138:             trade.prices.buyHigh,
139:             trade.prices.sellLow,
140:             CEIL
141:         );
142:         // exactSellAmount: Amount to sell to buy `deficitAmount` if there's no slippage
143: 
144:         // slippedSellAmount: Amount needed to sell to buy `deficitAmount`, counting slippage
145:         uint192 slippedSellAmount = exactSellAmount.div(FIX_ONE.minus(maxTradeSlippage), CEIL);
146: 
147:         trade.sellAmount = fixMin(slippedSellAmount, trade.sellAmount); // {sellTok} <<<<<<<<@ HERE
148:         return prepareTradeSell(trade, minTradeVolume, maxTradeSlippage); <<<<<<<<@ HERE
149:     }
150: 

line 147, 148, we determine minBuyAmount by calling prepareTradeSell with slipped sell amount (the amount that we are okey to lose at max)

now when we do the bad rounding direction, we exceed that slippage

worstCasePrice = shiftl_toFix(req.minBuyAmount, 9).divu(req.sellAmount, FLOOR); in wors case price we div by sellAmount (that we previously rounded up) and we use minBuyAmount that we previously rounded down

making worstCasePrice further lower

[thereksfour]

worstCasePrice is only used in determining whether there should report violation or not, we can say that a lower worstCasePrice will be more lenient on violation checking, but only a 1 wei dust effect.

            if (clearingPrice.lt(worstCasePrice)) broker.reportViolation();