Attacker can manipulate token balances to affect auction settlement (`GnosisTrade::settle`)

[c4-bot-8]

Lines of code

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

Vulnerability details

Description

The GnosisTrade contract is designed to facilitate trades using the Gnosis EasyAuction mechanism. It includes a settle() function responsible for finalizing the auction, transferring tokens, and verifying the auction's clearing price. However, a issue exists in this function that allows an attacker to manipulate token balances just before settlement, potentially leading to incorrect calculations and unfair distribution of tokens.

The settle() function performs several key operations:

1. Settles the auction if not already done.
2. Reads the current balances of sell and buy tokens.
3. Transfers these balances to the origin address.
4. Calculates the clearing price and checks for violations.

The issue stems from the fact that the function reads token balances at the time of settlement, rather than using locked balances from the time of auction initiation. This opens up the possibility for an attacker to front-run the settle() transaction and manipulate the contract's token balances.

Specifically, the issue occurs in the following lines:

File: GnosisTrade.sol
205:         if (sellBal != 0) IERC20Upgradeable(address(sell)).safeTransfer(origin, sellBal);
206:         if (boughtAmt != 0) IERC20Upgradeable(address(buy)).safeTransfer(origin, boughtAmt);

An attacker could transfer additional tokens to the contract just before these lines are executed, artificially inflating the balances. This would lead to incorrect calculations of soldAmt and boughtAmt, potentially resulting in an unfair distribution of tokens and incorrect violation reporting.

Impact

The impact of this vulnerability is significant and multi-faceted:

• Financial Loss: The manipulated balances can result in incorrect token transfers, leading to financial losses for legitimate participants in the auction.

• Incorrect Violation Reporting: The clearingPrice calculation, which is based on these manipulated balances, may lead to false positives or negatives in violation reporting. This undermines the integrity of the auction process and the protocol's ability to detect and respond to unfair trades.

Proof of Concept

1. Alice initiates an auction by calling init() on the GnosisTrade contract.
2. The auction proceeds normally until it's time for settlement.
3. Bob, an attacker, monitors the mempool for settle() transactions.
4. When Alice attempts to settle the auction, Bob front-runs the transaction: a. Bob quickly transfers additional sell and buy tokens to the GnosisTrade contract. b. Bob's transaction is mined before Alice's settle() transaction.
5. Alice's settle() transaction is now processed with manipulated balances:

   uint256 sellBal = sell.balanceOf(address(this)); // Includes Bob's additional tokens
   boughtAmt = buy.balanceOf(address(this)); // Includes Bob's additional tokens

6. The function proceeds to transfer these inflated balances and calculate the clearing price based on incorrect amounts.
7. As a result, the auction settles with an unfair distribution of tokens, and the clearingPrice calculation may not accurately reflect the true auction outcome.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this vulnerability, implement a mechanism to lock the token balances at the time of auction initiation. Use these locked balances for all calculations and transfers during settlement, rather than reading the current balances.

Here's a proposed fix:

+ uint256 private lockedSellBal;
+ uint256 private lockedBuyBal;

function init(
    IBroker broker_,
    address origin_,
    IGnosis gnosis_,
    uint48 batchAuctionLength,
    TradeRequest calldata req
) external stateTransition(TradeStatus.NOT_STARTED, TradeStatus.OPEN) {
    // ... existing code ...

+   lockedSellBal = sell.balanceOf(address(this));
+   lockedBuyBal = buy.balanceOf(address(this));

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

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

-   uint256 sellBal = sell.balanceOf(address(this));
-   boughtAmt = buy.balanceOf(address(this));
+   uint256 sellBal = lockedSellBal;
+   boughtAmt = lockedBuyBal;

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

This change ensures that the settlement calculations and token transfers are based on the locked balances from the time of auction initiation, preventing any manipulation during the settlement process.

Assessed type

Other