Attacker can manipulate auction outcome by exploiting rounding in clearingPrice calculation (`GnosisTrade::settle`)

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/trading/GnosisTrade.sol#L175-L221

Vulnerability details

Description

The GnosisTrade contract facilitates trades using the Gnosis EasyAuction mechanism. The settle() function is responsible for finalizing the auction, transferring tokens, and verifying if the auction's clearing price meets the expected worst-case price. However, there is a vulnerability in the way the clearing price is calculated and compared to the worst-case price, potentially allowing an attacker to manipulate the auction outcome by exploiting rounding errors.

The settle() function includes the following logic to calculate the clearing price:

File: GnosisTrade.sol
212:             // Gnosis rounds defensively in the buy token; we should not consider it a violation
213:             uint256 adjustedSoldAmt = Math.max(soldAmt, 1);
214:             uint256 adjustedBuyAmt = boughtAmt + 1;
215: 
216:             // D27{buyTok/sellTok}
217:             uint192 clearingPrice = shiftl_toFix(adjustedBuyAmt, 9).divu(adjustedSoldAmt, FLOOR);
218:             if (clearingPrice.lt(worstCasePrice)) broker.reportViolation();

The issue arises from two main factors:

1. The use of Math.max(soldAmt, 1) to ensure a non-zero denominator.
2. The addition of 1 to boughtAmt to account for Gnosis' defensive rounding.

These adjustments, combined with the FLOOR rounding in the divu() function, can lead to scenarios where an attacker can manipulate the auction to barely avoid triggering a violation report, even when the actual clearing price is below the worst-case price.

For example, if the worstCasePrice is set to 100 and an attacker manages to settle the auction with soldAmt = 100 and boughtAmt = 9900, the actual price would be 99. However, due to the adjustments and rounding, the calculated clearingPrice would be 100, equal to the worstCasePrice, thus avoiding a violation report.

Impact

Attackers can exploit the rounding mechanism to settle auctions at prices slightly below the worst-case price without triggering violation reports.

Legitimate participants may receive fewer tokens than expected based on the intended worst-case price protection, resulting in direct financial losses.

Proof of Concept

1. Alice initiates an auction with a worstCasePrice of 100 (assuming appropriate scaling).
2. The auction proceeds, and Bob (the attacker) manipulates the final bid to result in:
   • soldAmt = 100
   • boughtAmt = 9900 (which would normally result in a price of 99, below the worst-case price)
3. In the settle() function:
   • adjustedSoldAmt becomes 100 (due to Math.max(soldAmt, 1))
   • adjustedBuyAmt becomes 9901 (due to boughtAmt + 1)
4. The clearingPrice calculation: clearingPrice = shiftl_toFix(9901, 9).divu(100, FLOOR); Due to rounding, this results in a clearingPrice of 100, equal to the worstCasePrice.
5. The condition clearingPrice.lt(worstCasePrice) evaluates to false, so no violation is reported.
6. The auction settles at an effective price of 99, below the worst-case price, without triggering a violation report.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this vulnerability, implement a more robust comparison mechanism that accounts for potential rounding errors. Here's a proposed fix:

function settle()
    external
    stateTransition(TradeStatus.OPEN, TradeStatus.CLOSED)
    returns (uint256 soldAmt, uint256 boughtAmt)
{
    // ... existing code ...

    if (sellBal < initBal) {
        soldAmt = initBal - sellBal;

-       uint256 adjustedSoldAmt = Math.max(soldAmt, 1);
-       uint256 adjustedBuyAmt = boughtAmt + 1;
-       uint192 clearingPrice = shiftl_toFix(adjustedBuyAmt, 9).divu(adjustedSoldAmt, FLOOR);
-       if (clearingPrice.lt(worstCasePrice)) broker.reportViolation();

+       // Use precise calculations without adjustments
+       uint192 clearingPrice = shiftl_toFix(boughtAmt, 9).divu(soldAmt, CEIL);
+       if (clearingPrice.lte(worstCasePrice)) broker.reportViolation();
    }

    // ... rest of the function ...
}

This change does the following:

1. Removes the adjustments to soldAmt and boughtAmt, using the actual values.
2. Changes the rounding mode in divu() from FLOOR to CEIL, ensuring we err on the side of caution.
3. Changes the comparison from lt (less than) to lte (less than or equal to), to catch cases where the clearing price exactly matches the worst-case price.

Assessed type

Other