A transaction leading a reserve to become zero will cause incorrect TWAP calculation

Summary

It is possible for a transaction to cause reserve of the baseToken or the quoteToken to become 0. In that case, TWAP calculation will be incorrect.

Vulnerability Detail

GSPVault::_twapUpdate() function calculates TWAP price by calculating mid price and elapsed time if reserves are not zero.
https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L86C2-L97C6

    function _twapUpdate() internal {
        // blockTimestamp is the timestamp of the current block
        uint32 blockTimestamp = uint32(block.timestamp % 2**32);
        // timeElapsed is the time elapsed since the last update
        uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;
        // if timeElapsed is greater than 0 and the reserves are not 0, update the twap price
-->     if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
-->         _BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed; //@audit reserves are checked to prevent error in getMidPrice
        }
        // update the last block timestamp
        _BLOCK_TIMESTAMP_LAST_ = blockTimestamp;
    }

The reason for checking reserves are not zero is to prevent division by zero error in the getMidPrice() function (shown with arrows below).

    function getMidPrice(PMMState memory state) internal pure returns (uint256) {
        if (state.R == RState.BELOW_ONE) {
-->         uint256 R = DecimalMath.divFloor(state.Q0 * state.Q0 / state.Q, state.Q); //@audit-info division by current reserve
            R = DecimalMath.ONE - state.K + (DecimalMath.mulFloor(state.K, R));
            return DecimalMath.divFloor(state.i, R);
        } else {
-->         uint256 R = DecimalMath.divFloor(state.B0 * state.B0 / state.B, state.B); ////@audit-info division by current reserve
            R = DecimalMath.ONE - state.K + (DecimalMath.mulFloor(state.K, R));
            return DecimalMath.mulFloor(state.i, R);
        }
    }

Normally, in constant product AMM's, it is impossible to totally drain one of the tokens totally and make the balance 0 due to x * y = k formula. However, this protocol is not work like constant product AMM's. In this protocol, token reserves are calculated as token balance - maintainer fee.

Thereby, it is possible to perform a huge selling action that results in a token balance == maintainer fee, which makes the reserve for that token to be equal to 0. Let's check sellBase function.

    function sellBase(address to) external nonReentrant returns (uint256 receiveQuoteAmount) {
        uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this)) - _MT_FEE_BASE_;
        uint256 baseInput = baseBalance - uint256(_BASE_RESERVE_);
        uint256 mtFee;
        uint256 newBaseTarget;
        PMMPricing.RState newRState;
        // calculate the amount of quote token to receive and mt fee
        (receiveQuoteAmount, mtFee, newRState, newBaseTarget) = querySellBase(tx.origin, baseInput);
        // transfer quote token to recipient
        _transferQuoteOut(to, receiveQuoteAmount);
        // update mt fee in quote token
        _MT_FEE_QUOTE_ = _MT_FEE_QUOTE_ + mtFee;

        // ...

        // update reserve
-->     _setReserve(baseBalance, _QUOTE_TOKEN_.balanceOf(address(this)) - _MT_FEE_QUOTE_); //@audit if the remaining quote token balance == total mt fees, reserves for quote token will be 0.

        //...
    }

_setReserve function will be called after the selling action, and reserves for the quote token will be 0 if the remaining quote token balance == total mt fees. The _setReserve function will then call the _twapUpdate().

As we mentioned above, the _twapUpdate() function will not update the cumulative price due to one of the reserves being 0.

        if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
           _BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed;
        }
        // update the last block timestamp
-->     _BLOCK_TIMESTAMP_LAST_ = blockTimestamp; //@audit this is updated regardless of the if block above.

_BASE_PRICE_CUMULATIVE_LAST_ parameter will not be updated, but the _BLOCK_TIMESTAMP_LAST_ parameter will be updated.
So basically, the protocol will mistakenly interpret the TWAP as updated due to the last timestamp is changed, yet in truth, the TWAP remains unchanged as the associated function will not execute the if block.

Note: The TWAP is not directly used in-scope contracts but it is updated in-scope contracts. I assume it is used in other parts of the protocol and an important element.

Impact

TWAP will be incorrect.

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L92C1-L96C49

Tool used

Manual Review

Recommendation

I am not sure what is the best solution for this issue but maybe disabling TWAP usage in case of a reserve becomes zero might be better than using incorrect TWAP prices. Setting _IS_OPEN_TWAP_ to false when one of the reserves become 0, and adding an only admin function to open it again when reserves are filled up might work out.

sherlock-audit / 2023-12-dodo-gsp-judging

osmanozdemir1 - A transaction leading a reserve to become zero will cause incorrect TWAP calculation #96