Hardcoded rounding strategy in `DynamicFees::dynamicFeeBps()` rounds against the protocol on lovTokens deposits

[hats-bug-reporter[bot]]

Github username: @JacoboLansac Twitter username: jacolansac Submission hash (on-chain): 0x4cd8fb5219bf45add119fbd5a035bb9c7deee73803091c4edb69f641bbcf3d0b Severity: low

Description: The dynamic fees mechanism aims to impose a higher fee on users when they invest/exit at an unusually favorable rate. The DynamicFees library is in charge of calculating these fees, and it does so by querying the latest prices from the oracle: SPOT and HISTORIC. Then these prices are compared to calculate a _delta, which is then used to increase the dynamic fees when the difference gives a favorable deal to the user. For context:

The general principle can be summarized as:

If a delta between HISTORIC and SPOT goes in the direction that favors the user, the higher the absolute _delta, the higher the fee should be.

For context, in the case of lovStEth, the oracle returns prices in [WETH/wstETH] units (how many WETH you get per 1 wstETH). A lower price means a higher value of wstETH, which would be reflected in a free market in relation to the lovStEth tokens. This, a user gets a favorable operation if:

• deposits when SPOT < HISTORIC, as the value you get per wstETH is higher than usual.
• exits when SPOT > HISTORIC, as the value you get per wstETH is higher than usual

The issue

When DynamicFees::dynamicFeeBps() queries the SPOT and HISTORIC prices, the SPOT is always queried with ROUND_UP and HISTORIC always with ROUND_DOWN. This rounding aims to round up the resulting _delta between those prices, which would round up the resulting fees, to favor the protocol. However, this is only true when SPOT > HISTORIC. However, when SPOT < HISTORIC, the hardcoded rounding strategy will result in a downward rounding of _delta, which will round down the protocol fees, going against the protocol.

DynamicFees.sol

    /**
     * @notice The current deposit or exit fee based on market conditions.
     * Fees are applied to the portion of lovToken shares the depositor 
     * would have received. Instead that fee portion isn't minted (benefiting remaining users)
     */
    function dynamicFeeBps(
        FeeType feeType,
        IOrigamiOracle oracle,
        address expectedBaseAsset,
        uint64 minFeeBps,
        uint256 feeLeverageFactor
    ) internal view returns (uint256) {
        (uint256 _spotPrice, uint256 _histPrice, address _baseAsset, address _quoteAsset) = oracle.latestPrices(
            IOrigamiOracle.PriceType.SPOT_PRICE,
@>          OrigamiMath.Rounding.ROUND_UP,      //  @audit-issue SPOT price is always rounded UP
            IOrigamiOracle.PriceType.HISTORIC_PRICE,
@>          OrigamiMath.Rounding.ROUND_DOWN     //  @audit-issue HISTORIC price is always rounded DOWN
        );

        bool _inQuotedOrder;
@>      if (_baseAsset == expectedBaseAsset) {
            _inQuotedOrder = true;
        } else if (_quoteAsset == expectedBaseAsset) {
            _inQuotedOrder = false;
        } else {
            revert CommonEventsAndErrors.InvalidToken(expectedBaseAsset);
        }

        // @audit-info: in the case of lovSstEth, inQuoteOrder=true, as the price comes in [wstETH/WETH].

        uint256 _delta;
        if (feeType == FeeType.DEPOSIT_FEE) {
            // @audit-info wrongly copy pasted comment, as this talks about exit fees, not deposit fees
            // If spot price is > than the expected historic, then they are exiting
            // at a price better than expected. The exit fee is based off the relative
            // difference of the expected spotPrice - historicPrice.
            // Or opposite if the oracle order is inverted
            unchecked {
                if (_inQuotedOrder && _spotPrice < _histPrice) {
@>                  _delta = _histPrice - _spotPrice;  // @audit-issue  _delta rounded DOWN: (SPOT rounded up, HISTORIC rounded down)
                } else if (!_inQuotedOrder && _spotPrice > _histPrice) {
                    _delta = _spotPrice - _histPrice;  // @audit-ok     _delta rounded UP: (SPOT rounded up, HISTORIC rounded down)
                }
            }
        } else {
            // If spot price is > than the expected historic, then they are exiting
            // at a price better than expected. The exit fee is based off the relative
            // difference of the expected spotPrice - historicPrice.
            // Or opposite if the oracle order is inverted
            unchecked {
                if (_inQuotedOrder && _spotPrice > _histPrice) {
                    _delta = _spotPrice - _histPrice;  // @audit-ok     _delta rounded UP: (SPOT rounded up, HISTORIC rounded down)
                } else if (!_inQuotedOrder && _spotPrice < _histPrice) {
@>                  _delta = _histPrice - _spotPrice;  // @audit-issue  _delta rounded DOWN: (SPOT rounded up, HISTORIC rounded down)
                }
            }
        }

        // If no delta, just return the min fee
        if (_delta == 0) {
            return minFeeBps;
        }

        // Relative diff multiply by a leverage factor to match the worst case lovToken
        // effective exposure
        uint256 _fee = _delta.mulDiv(
            feeLeverageFactor * OrigamiMath.BASIS_POINTS_DIVISOR,
            _histPrice,
            OrigamiMath.Rounding.ROUND_UP
        );

        // Use the maximum of the calculated fee and a pre-set minimum.
        return minFeeBps > _fee ? minFeeBps : _fee;
    }

Impact: low

The hardcoded rounding mechanism when querying SPOT and HISTORIC prices will round down the protocol fees in the following situations:

• For strategies that query prices with _baseAsset == expectedBaseAsset, (lovStEth, lovDsr strategies), fees are rounded down on deposits.
• For strategies that query prices with _quoteAsset == expectedBaseAsset, (none for now), fees are rounded down on exits.

Recommendation

A solution is not trivial. The fact that the appropriate rounding direction depends on the result of the query makes it necessary to make two queries to get the absolute perfect price, and this is a waste of gas for the overall impact of getting the exact rounding.

However, as the current implementation is biased towards exits, a solution that compromises the rounding between deposits and exits would be to round both prices in the same direction (up or down).

DynamicFees.sol

    function dynamicFeeBps(
        FeeType feeType,
        IOrigamiOracle oracle,
        address expectedBaseAsset,
        uint64 minFeeBps,
        uint256 feeLeverageFactor
    ) internal view returns (uint256) {
        (uint256 _spotPrice, uint256 _histPrice, address _baseAsset, address _quoteAsset) = oracle.latestPrices(
            IOrigamiOracle.PriceType.SPOT_PRICE,
            OrigamiMath.Rounding.ROUND_UP,
            IOrigamiOracle.PriceType.HISTORIC_PRICE,
-           OrigamiMath.Rounding.ROUND_DOWN
+           OrigamiMath.Rounding.ROUND_UP
        );

        // ...

[frontier159]

Thanks for the report. I agree that your finding is correct, however practically this won't introduce a risk/vulnerability. The fee is just an economic tool to ensure that we disincentivise abuse when the observed spot price is away from the expected historic price.

A +- of 1 wei in that spot price will be negligible in the fee outcome, perhaps +-1 bps at most. It doesn't have to be perfectly symmetric, as long as the economic tool is still effective (which it will be)

In saying that, yes it's still a low. Agree we can add a comment to explain this, and round in the same direction to bias for exits

[JacoboLansac]

100% agree on the super low impact. If there was an informational category, I would have labelled a that. Thanks for accepting though