GnosisTrade contract can be frontrunned in order to make it report violation and block broker

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/Broker.sol#L187-L209 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/GnosisTrade.sol#L87

Vulnerability details

Impact

GnosisTrade contract can be frontrunned in order to make it report violation and block broker. This will stop broker from creating another traders.

Proof of Concept

When BackingManager is rebalancing, then it can open trade, which can be gnosis trade auction.

In this case new GnosisTrade is created using create opcode. Note, it's possible to calculate address of next created contract by contract. Attacker needs to know address of creator and his nonce. So it's not hard for attacker to frontrun BackingManager.rebalance which will create GnosisTrade auction. What attacker will do is top up that address with some small amount of req.sell token.

When GnosisTrade is initiated, then his balance is stored to initBal variable. And also worstCasePrice is calculated, which is req.minBuyAmount/req.sellAmount. This variable is used inside settle function to check that gnosis executed auction with good price. https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/GnosisTrade.sol#L162-L205

    //   state' is CLOSED
    function settle()
        external
        stateTransition(TradeStatus.OPEN, TradeStatus.CLOSED)
        returns (uint256 soldAmt, uint256 boughtAmt)
    {
        require(msg.sender == origin, "only origin can settle");

        // Optionally process settlement of the auction in Gnosis
        if (!isAuctionCleared()) {
            // By design, we don't rely on this return value at all, just the
            // "cleared" state of the auction, and the token balances this contract owns.
            // slither-disable-next-line unused-return
            gnosis.settleAuction(auctionId);
            assert(isAuctionCleared());
        }

        // At this point we know the auction has cleared

        // Transfer balances to origin
        uint256 sellBal = sell.balanceOf(address(this));
        boughtAmt = buy.balanceOf(address(this));

        if (sellBal > 0) IERC20Upgradeable(address(sell)).safeTransfer(origin, sellBal);
        if (boughtAmt > 0) IERC20Upgradeable(address(buy)).safeTransfer(origin, boughtAmt);

        // Check clearing prices
        if (sellBal < initBal) {
            soldAmt = initBal - sellBal;

            // Gnosis rounds defensively in the buy token; we should not consider it a violation
            uint256 adjustedSoldAmt = Math.max(soldAmt, 1);
            uint256 adjustedBuyAmt = boughtAmt + 1;

            // {buyTok/sellTok}
            uint192 clearingPrice = shiftl_toFix(adjustedBuyAmt, -int8(buy.decimals())).div(
                shiftl_toFix(adjustedSoldAmt, -int8(sell.decimals()))
            );

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

You can see, that soldAmt = initBal - sellBal is used to determine how much sell tokens were sold. And then this soldAmt is used to check the price.

So the purpose of attacker is to inflate soldAmt variable in order to make clearing price less than worstCasePrice and initiate reportViolation call and stop broker.

Tools Used

VsCode

Recommended Mitigation Steps

Maybe you need to store initBal = req.sellAmount in the init. Then no matter if attacker donated smth to the contract, clearing price should be correct.

Assessed type

Error

[tbrent]

We believe there is no issue here, but we will write a test to confirm. I think the relevant detail is that uint256 sellBal = sell.balanceOf(address(this));. This variable will contain the initial donated balance that did not go out to EasyAuction during init().

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof