Lack of balance check can DOS bidding and Settling of Dutch Trades

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/trading/DutchTrade.sol#L306-L307

Vulnerability details

Impact

Bidding and subsequent settling of Dutch Trades can not be completed in some scenarios. This would cause unnecessary DOS and grief to auctioners and bidders using the protocol

Proof of Concept

Dutch Trades are one of two auction methods used in selling and buying tokens within the reserve system. An auctioner can create Dutch auctions for an amount of sell tokens which bidders can bid on. Once a bid is accepted DutchTrade.settle() is called to settle the trade.

        function settle()
        external
        stateTransition(TradeStatus.OPEN, TradeStatus.CLOSED)
        returns (uint256 soldAmt, uint256 boughtAmt)
    {
        require(msg.sender == address(origin), "only origin can settle");
        require(bidder != address(0) || block.timestamp > endTime, "auction not over");

        if (bidType == BidType.CALLBACK) {
            soldAmt = lot(); // {qSellTok}
        } else if (bidType == BidType.TRANSFER) {
            soldAmt = lot(); // {qSellTok}
            sell.safeTransfer(bidder, soldAmt); // {qSellTok}
        }

        //audit: Should check balances
        // Transfer remaining balances back to origin
        boughtAmt = buy.balanceOf(address(this)); // {qBuyTok}
@>      buy.safeTransfer(address(origin), boughtAmt); // {qBuyTok}
@>      sell.safeTransfer(address(origin), sell.balanceOf(address(this))); // {qSellTok}
    }

The issues comes in because the balance of the tokens in the contract is not checked before attempting to transfer them to origin. This would cause a revert where the token is designed to revert on zero value transfers.

There are two scenarios where there can be 0 balance of tokens in the contract.

1. The exact amount of sell tokens has been transferred to the bidder. Therefore, no more sell tokens remains in the contract
2. No successful bid has been made and the original seller wants to manually settle the auction and cancel the trade. This means boughtAmount would be zero as no bidder has transferred anything to the contract.

In scenario 1, bidding in such cases would always fail causing grief to otherwise legitimate bidders. In [2], original sellers may get their sell tokens stuck in the trading contract, as the attempt to transfer non-existent buy tokens would revert.

Note that in the gnosis trade contract, the balance is checked before transfering to origin but it is missed here. In private discussions, the sponsor has also agreed on this issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Only transfer balance of buy and/or sell tokens if they're greater than 0

-   buy.safeTransfer(address(origin), boughtAmt); // {qBuyTok}
-   sell.safeTransfer(address(origin), sell.balanceOf(address(this))); // {qSellTok}
+  if(boughtAmt > 0) buy.safeTransfer(address(origin), boughtAmt); // {qBuyTok}
+  if(sell.balanceOf(address(this)) > 0) sell.safeTransfer(address(origin), sell.balanceOf(address(this))); // {qSellTok}

Assessed type

Invalid Validation

[thereksfour]

"revert on zero transfer" tokens are out of scope

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid