Krace - Users don't need to pay for swapFee when selling or buying tokens

[sherlock-admin]

Krace

high

Users don't need to pay for swapFee when selling or buying tokens

Summary

The D3Trading contract only charges the mtFee and ignores the swapFee in function querySellTokens and queryBuyTokens. This results in users obtaining more to tokens with the same amount of from tokens or expending fewer from tokens to acquire the same amount of to tokens.

Vulnerability Detail

queriySellTokens should return the amount of to tokens that user should receive, which should exclude mtFee and swapFee, but it only returns receiveToAmount - mtFee.

    function querySellTokens(
        address fromToken,
        address toToken,
        uint256 fromAmount
    ) public view returns (uint256 payFromAmount, uint256 receiveToAmount, uint256 vusdAmount, uint256 swapFee, uint256 mtFee) {
        require(fromAmount > 1000, Errors.AMOUNT_TOO_SMALL);
        Types.RangeOrderState memory D3State = getRangeOrderState(fromToken, toToken);

        {
        uint256 fromTokenDec = IERC20Metadata(fromToken).decimals();
        uint256 toTokenDec = IERC20Metadata(toToken).decimals();
        uint256 fromAmountWithDec18 = Types.parseRealAmount(fromAmount, fromTokenDec);
        uint256 receiveToAmountWithDec18;
        ( , receiveToAmountWithDec18, vusdAmount) =
            PMMRangeOrder.querySellTokens(D3State, fromToken, toToken, fromAmountWithDec18);

        receiveToAmount = Types.parseDec18Amount(receiveToAmountWithDec18, toTokenDec);
        payFromAmount = fromAmount;
        }

        receiveToAmount = receiveToAmount > state.balances[toToken] ? state.balances[toToken] : receiveToAmount;

        uint256 swapFeeRate = D3State.fromTokenMMInfo.swapFeeRate +  D3State.toTokenMMInfo.swapFeeRate;
        swapFee = DecimalMath.mulFloor(receiveToAmount, swapFeeRate);
        uint256 mtFeeRate = D3State.fromTokenMMInfo.mtFeeRate +  D3State.toTokenMMInfo.mtFeeRate;
        mtFee = DecimalMath.mulFloor(receiveToAmount, mtFeeRate);
//@audit -> (receiveToAmount - mtFee) doesn't take the swapFee into consideration, user will receive more to tokens
        return (payFromAmount, receiveToAmount - mtFee, vusdAmount, swapFee, mtFee);
    }

The same logic is also implemented in funtion queryBuyTokens

    function queryBuyTokens(
        address fromToken,
        address toToken,
        uint256 toAmount
    ) public view returns (uint256 payFromAmount, uint256 receiveToAmount, uint256 vusdAmount, uint256 swapFee, uint256 mtFee) {
        require(toAmount > 1000, Errors.AMOUNT_TOO_SMALL);
        Types.RangeOrderState memory D3State = getRangeOrderState(fromToken, toToken);

        // query amount and transfer out
        uint256 toAmountWithFee;
        {
        uint256 swapFeeRate = D3State.fromTokenMMInfo.swapFeeRate +  D3State.toTokenMMInfo.swapFeeRate;
        swapFee = DecimalMath.mulFloor(toAmount, swapFeeRate);
        uint256 mtFeeRate = D3State.fromTokenMMInfo.mtFeeRate +  D3State.toTokenMMInfo.mtFeeRate;
        mtFee = DecimalMath.mulFloor(toAmount, mtFeeRate);
//@audit -> toAmountWithFee only adds mtFee, swapFee is ignored.
        toAmountWithFee = toAmount + mtFee;
        }

        require(toAmountWithFee <= state.balances[toToken], Errors.BALANCE_NOT_ENOUGH);

        uint256 fromTokenDec = IERC20Metadata(fromToken).decimals();
        uint256 toTokenDec = IERC20Metadata(toToken).decimals();
        uint256 toFeeAmountWithDec18 = Types.parseRealAmount(toAmountWithFee, toTokenDec);
        uint256 payFromAmountWithDec18;
        (payFromAmountWithDec18, , vusdAmount) =
            PMMRangeOrder.queryBuyTokens(D3State, fromToken, toToken, toFeeAmountWithDec18);
        payFromAmount = Types.parseDec18Amount(payFromAmountWithDec18, fromTokenDec);
        if(payFromAmount == 0) {
            payFromAmount = 1;
        }

        return (payFromAmount, toAmount, vusdAmount, swapFee, mtFee);
    }

Impact

Users don't need to pay for swapFee when selling or buying tokens

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo/blob/ea7f786161113144562a900dbff31457ff7025ef/dodo-v3/contracts/DODOV3MM/D3Pool/D3Trading.sol#L206 https://github.com/sherlock-audit/2023-12-dodo/blob/ea7f786161113144562a900dbff31457ff7025ef/dodo-v3/contracts/DODOV3MM/D3Pool/D3Trading.sol#L229

Tool used

Manual Review

Recommendation

It is recommended to subtract swapFee from receiveToAmount in function querySellTokens and add swapFee to toAmountWithFee in function queryBuyTokens.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

0xLogos commented:

dup of #43

[Attens1423]

It's not a bug. The fees are divided into lpFee and mtFee. The lpFee is implicitly included in the calculation of bidUpPrice and askDownPrice, and the outer swapFee is only used for recording events. The mtFee is calculated and transferred at the outermost level.

[osmanozdemir1]

Escalate

This is an update contest. I don't understand how changing this to this, but keeping the record the same (previous, current) is not a bug. Either one of them must be incorrect.

Thanks.

[sherlock-admin2]

Escalate

This is an update contest. I don't understand how changing this to this, but keeping the record the same (previous, current) is not a bug. Either one of them must be incorrect.

Thanks.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@Attens1423 @traceurl To my knowledge, there is no issues with how fees are charged based on your comments here. You might want to shed some light on the mechanisms around how swap fee and mt fees are charged to clear watsons doubt.

[Czar102]

@osmanozdemir1 I'm not getting your point in the escalation, please explain your logic in more detail. Otherwise, I will be rejecting the escalation and leaving the issue as is.

Please note that you are to check the current system, so while diff may be useful when searching for bugs, be careful when arguing about the validity of the report that way.

[osmanozdemir1]

Hi @Czar102 I escalated this issue because it was the primary one. My original submission #9 explains why I considered this diff as a bug.

In summary, protocol changed the receiveToAmount return variable in the querySellTokens function but didn't change the corresponding swap record. #9 explains it in detail.

[Czar102]

Given comments on #10, planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• osmanozdemir1: rejected