code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/DutchTrade.sol#L160 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L46 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BackingManager.sol#L81

Vulnerability details

Impact

notTradingPausedOrFrozen that is turned on and off during an open Dutch trade could have the auction closed with a lower price depending on the timimg, leading to lesser capability to boost the Rtoken and/or stRSR exchange rates as well as a weakened recollaterization.

Proof of Concept

Here's the scenario:

A 30 minute Dutch trade is opened by the Revenue trader selling a suplus token for Rtoken.
Shortly after the price begins to decrease linearly, Alice calls bid(). As can be seen in line 160 of the code block below, settleTrade() is externally called on the origin, RevenueTrader.sol in this case:

    function bid() external returns (uint256 amountIn) {
        require(bidder == address(0), "bid already received");

        // {qBuyTok}
        amountIn = bidAmount(uint48(block.timestamp)); // enforces auction ongoing

        // Transfer in buy tokens
        bidder = msg.sender;
        buy.safeTransferFrom(bidder, address(this), amountIn);

        // status must begin OPEN
        assert(status == TradeStatus.OPEN);

        // settle() via callback
160:        origin.settleTrade(sell);

        // confirm callback succeeded
        assert(status == TradeStatus.CLOSED);
    }

However, her call is preceded by pauseTrading() invoked by a PAUSER, and denied on line 46 of the function below:

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L43-L52

    function settleTrade(IERC20 sell)
        public
        override(ITrading, TradingP1)
46:        notTradingPausedOrFrozen
        returns (ITrade trade)
    {
        trade = super.settleTrade(sell); // nonReentrant
        distributeTokenToBuy();
        // unlike BackingManager, do _not_ chain trades; b2b trades of the same token are unlikely
    }

As the auction is nearing to endTime, the PAUSER calls unpauseIssuance().
Bob, the late comer, upon seeing this, proceeds to calling bid() and gets the sell token for a price much lower than he would initially expect before the trading pause.

Tools Used

Manual

Recommended Mitigation Steps

Consider removing notTradingPausedOrFrozen from the function visibility of RevenueTrader.settleTrade and BackingManager.settleTrade. This will also have a good side effect of allowing the settling of a Gnosis trade if need be. Collectively, the settled trades could at least proceed to helping boost the RToken and/or stRSR exchange rates that is conducive to the token holders redeeming and withdrawing. The same shall apply to enhancing recollaterization, albeit future tradings will be halted if the trading pause is still enabled.

Assessed type

Timing

0xean commented 1 year ago

This also seems like QA. It outlines a very specific set of events that are very unlikely to occur during production scenarios and would additionally come down to admin misconfiguration / mismanagement. will wait for sponsor comment, but most likely downgrade to QA

0xean commented 1 year ago

The PAUSER role should be assigned to an address that is able to act quickly in response to off-chain events, such as a Chainlink feed failing. It is acceptable for there to be false positives, since redemption remains enabled.

it is good to consider this quote from the documentation stating that pausing may have false positives.