sherlock-audit / 2023-06-dinari-judging

5 stars 4 forks source link

serial-coder - Incorrect calculation of BUY orders' percentage fee #111

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

serial-coder

medium

Incorrect calculation of BUY orders' percentage fee

Summary

The percentage fees of all BUY orders were miscalculated. Compared to the percentage fees of SELL orders, the fees for BUY orders are more than 11.1111%. This miscalculation will cause all platform users to pay more buying fees unsuspectedly.

Vulnerability Detail

This section comprises two subsections: Proof Of Concept and Root Cause Analysis.

Proof Of Concept

The following PoC snippets prove that the percentage fees between BUY and SELL orders differ considerably.

I must create one mock file named BuyOrderIssuerMock.sol based on the /src/issuer/BuyOrderIssuer.sol. The only change I made was renaming the PrbMath object to PrbMath2 such that when this mock file is imported into the below test file, the PrbMath object will not conflict with the same object's name declared in the /src/issuer/SellOrderProcessor.sol.

diff --git a/src/issuer/BuyOrderIssuer.sol b/src/issuer/BuyOrderIssuerMock.sol
index 9d65a52..4436359 100644
--- a/src/issuer/BuyOrderIssuer.sol
+++ b/src/issuer/BuyOrderIssuerMock.sol
@@ -2,7 +2,7 @@
 pragma solidity 0.8.19;

 import {SafeERC20, IERC20} from "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
-import "prb-math/Common.sol" as PrbMath;
+import "prb-math/Common.sol" as PrbMath2; // ***CHANGE***
 import {OrderProcessor} from "./OrderProcessor.sol";
 import {IMintBurn} from "../IMintBurn.sol";

@@ -176,7 +176,7 @@ contract BuyOrderIssuer is OrderProcessor {
             uint256 collection = 0;
             if (feeState.remainingPercentageFees > 0) {
                 // fee = remainingPercentageFees * fillAmount / remainingOrder
-                collection = PrbMath.mulDiv(feeState.remainingPercentageFees, fillAmount, orderState.remainingOrder);
+                collection = PrbMath2.mulDiv(feeState.remainingPercentageFees, fillAmount, orderState.remainingOrder); // ***CHANGE***
             }
             // Update fee state
             if (collection > 0) {

Then, place the following test file in /test/PoCIncorrectFeeCalculation.t.sol.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import "forge-std/Test.sol";
import "solady/test/utils/mocks/MockERC20.sol";
import "openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import "../src/issuer/BuyOrderIssuerMock.sol";
import "../src/issuer/SellOrderProcessor.sol";
import {OrderFees, IOrderFees} from "../src/issuer/OrderFees.sol";

// cmd: forge test -vv --match-test testIncorrectBuyPercentageFeeCalculation
contract PoCIncorrectFeeCalculation is Test {
    BuyOrderIssuer buyIssuer;
    SellOrderProcessor sellIssuer;
    MockERC20 paymentToken;

    function setUp() public {
        paymentToken = new MockERC20("Money", "$", 6);
        address treasury = address(4);

        OrderFees orderFees = new OrderFees(address(this), 1 ether, 0.005 ether);

        BuyOrderIssuer buyIssuerImpl = new BuyOrderIssuer();
        buyIssuer = BuyOrderIssuer(
            address(
                new ERC1967Proxy(address(buyIssuerImpl), abi.encodeCall(buyIssuerImpl.initialize, (address(this), treasury, orderFees)))
            )
        );

        SellOrderProcessor sellIssuerImpl = new SellOrderProcessor();
        sellIssuer = SellOrderProcessor(
            address(
                new ERC1967Proxy(address(sellIssuerImpl), abi.encodeCall(sellIssuerImpl.initialize, (address(this), treasury, orderFees)))
            )
        );
    }

    function testIncorrectBuyPercentageFeeCalculation() public {
        uint64 perOrderFee = 100000000000000000;       // 1e17
        uint64 percentageFeeRate = 100000000000000000; // 1e17
        uint128 orderValue = 1000000000000000000;      // 1e18

        vm.assume(perOrderFee >= 1e17);
        vm.assume(percentageFeeRate < 1 ether);
        vm.assume(percentageFeeRate >= 1e17);
        vm.assume(orderValue >= 1 ether);

        OrderFees fees = new OrderFees(address(this), perOrderFee, percentageFeeRate);

        // BuyIssuer
        buyIssuer.setOrderFees(fees);

        console.log("perOrderFee: %d", perOrderFee);
        console.log("percentageFeeRate: %d", percentageFeeRate);
        console.log("orderValue: %d", orderValue);

        (uint256 inputValue, uint256 flatFee1, uint256 percentageFee1) =
            buyIssuer.getInputValueForOrderValue(address(paymentToken), orderValue);
        assertEq(inputValue - flatFee1 - percentageFee1, orderValue);
        (uint256 flatFee2, uint256 percentageFee2) = buyIssuer.getFeesForOrder(address(paymentToken), inputValue);
        assertEq(flatFee1, flatFee2);
        assertEq(percentageFee1, percentageFee2);

        // SellIssuer
        sellIssuer.setOrderFees(fees);

        uint256 flatFee3 = sellIssuer.getFlatFeeForOrder(address(paymentToken));
        uint256 percentageFee3 = sellIssuer.getPercentageFeeForOrder(orderValue);

        console.log("flatFee1: %d", flatFee1);
        console.log("flatFee2: %d", flatFee2);
        console.log("flatFee3: %d", flatFee3);

        assertEq(flatFee3, flatFee2);

        console.log("percentageFee1: %d", percentageFee1);
        console.log("percentageFee2: %d", percentageFee2);
        console.log("percentageFee3: %d", percentageFee3);

        assertNotEq(percentageFee3, percentageFee2);
    }
}

To run the PoC: forge test -vv --match-test testIncorrectBuyPercentageFeeCalculation.

❯ forge test -vv --match-test testIncorrectBuyPercentageFeeCalculation
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for test/PoCIncorrectFeeCalculation.t.sol:PoCIncorrectFeeCalculation
[PASS] testIncorrectBuyPercentageFeeCalculation() (gas: 714685)
Logs:
  perOrderFee: 100000000000000000
  percentageFeeRate: 100000000000000000
  orderValue: 1000000000000000000
  flatFee1: 100000
  flatFee2: 100000
  flatFee3: 100000
  percentageFee1: 111111111111111111
  percentageFee2: 111111111111111111
  percentageFee3: 100000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.87ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As you can see, with the same input params:

All flat fees (flatFee1 and flatFee2 were for BUY orders, whereas flatFee3 was for SELL orders) were identical (100000).

However, the percentage fees for BUY (percentageFee1 and percentageFee2) and SELL (percentageFee3) orders differed considerably. Compared to the percentage fees of the SELL orders, the fees for the BUY orders were more than 11.1111%.

Root Cause Analysis

From my deeper analysis, I found that the orderRequest.quantityIn param (L113) was the sum of order.paymentTokenQuantity and order.fee (containing both the flatFee and percentageFee).

FILE: https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/BuyOrderIssuer.sol
LOCATIONS: 71, 73, and 113

57:     function getFeesForOrder(address token, uint256 inputValue) //@audit inputValue = paymentTokenQuantity + fee (containing both the flatFee and percentageFee)
58:         public
59:         view
60:         returns (uint256 flatFee, uint256 percentageFee)
61:     {
62:         // Check if fee contract is set
63:         if (address(orderFees) == address(0)) {
64:             return (0, 0);
65:         }
66: 
67:         // Calculate fees
68:         flatFee = orderFees.flatFeeForOrder(token);
69:         // If input value is greater than flat fee, calculate percentage fee on remaining value
70:         if (inputValue > flatFee) {
71: @>          percentageFee = orderFees.percentageFeeForValue(inputValue - flatFee); //@audit the input = paymentTokenQuantity + percentageFee
72:         } else {
73: @>          percentageFee = 0; //@audit if inputValue <= flatFee, percentageFee = 0 (this is inconsistent with the percentage fee calculation of the SELL orders)
74:         }
75:     }

        // ...SNIPPED...

106:    function _requestOrderAccounting(OrderRequest calldata orderRequest, bytes32 orderId)
107:        internal
108:        virtual
109:        override
110:        returns (Order memory order)
111:    {
112:        // Determine fees
113: @>     (uint256 flatFee, uint256 percentageFee) = getFeesForOrder(orderRequest.paymentToken, orderRequest.quantityIn); //@audit orderRequest.quantityIn = order.paymentTokenQuantity + order.fee
114:        uint256 totalFees = flatFee + percentageFee;

            // ...SNIPPED...
142:    }

In L71, the percentage fee was calculated by inputting the inputValue - flatFee into the orderFees.percentageFeeForValue(). Let's dig deeper into the input of the percentageFeeForValue().

input = inputValue - flatFee
      = ( paymentTokenQuantity + fee (containing both the flatFee and percentageFee) ) - flatFee
      = paymentTokenQuantity + percentageFee

As you can see, there was the precomputed percentageFee inside the input of the percentageFeeForValue().

Furthermore, I also discovered another inconsistency in the fee calculation between the BUY and SELL orders in L73. Specifically, in the case of BUY orders, if the inputValue <= flatFee, the percentageFee would be 0. In SELL orders, all related parameters would be used to compute the percentageFee, regardless of whether or not the inputValue <= flatFee.

Impact

This miscalculation will cause all platform users to unsuspectedly pay more buying fees (about 11.1111%).

Code Snippet

https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/BuyOrderIssuer.sol#L71

https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/BuyOrderIssuer.sol#L73

https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/BuyOrderIssuer.sol#L113

Tool used

Manual Review

Recommendation

Recommend reworking all functions related to calculating the BUY orders' percentage fees to guarantee that the buying and selling fees are identical under the same test input parameters.

sherlock-admin commented 1 year ago

Escalate

I am confident this is a valid medium issue.

I discovered that the percentage fees of all BUY orders were miscalculated (with the PoC code and the root cause analysis sections provided in the issue's details).

I decided to extend my PoC to test what if I change the percentageFeeRate parameter, and the below shows the result.

Assume that the following are constants. perOrderFee = 100000000000000000 (1e18) orderValue = 1000000000000000000 (1e17)

  1. If percentageFeeRate = 100000000000000000 ( 1 * 1e17 (10% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 100000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 111111111111111111
  percentageFee2 (BUY order): 111111111111111111
  percentageFee3 (SELL order): 100000000000000000
  • The percentage change = 11.1111% increase.
  1. If percentageFeeRate = 200000000000000000 ( 2 * 1e17 (20% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 200000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 250000000000000000
  percentageFee2 (BUY order): 250000000000000000
  percentageFee3 (SELL order): 200000000000000000
  • The percentage change = 25% increase.
  1. If percentageFeeRate = 300000000000000000 ( 3 * 1e17 (30% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 300000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 428571428571428571
  percentageFee2 (BUY order): 428571428571428571
  percentageFee3 (SELL order): 300000000000000000
  • The percentage change = 42.8571% increase.
  1. If percentageFeeRate = 400000000000000000 ( 4 * 1e17 (40% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 400000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 666666666666666666
  percentageFee2 (BUY order): 666666666666666666
  percentageFee3 (SELL order): 400000000000000000
  • The percentage change = 66.6667% increase.
  1. If percentageFeeRate = 500000000000000000 ( 5 * 1e17 (50% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 500000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 1000000000000000000
  percentageFee2 (BUY order): 1000000000000000000
  percentageFee3 (SELL order): 500000000000000000
  • The percentage change = 100% increase.
  • With the percentage fee rate = 50%, the BUY order's percentage fee would be doubled with the SELL order's.
  1. If percentageFeeRate = 600000000000000000 ( 6 * 1e17 (60% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 600000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 1500000000000000000
  percentageFee2 (BUY order): 1500000000000000000
  percentageFee3 (SELL order): 600000000000000000
  • The percentage change = 150% increase.
  • With the percentage fee rate = 60%, the BUY order's percentage fee would be greater than even the order value.
  • This proves that the calculation of the BUY orders was miscalculated, resulting in the greater the percentage fee rate, the greater error.
  • In other words, the platform will overcharge the users.

Let me further explain the PoC code.

    function testIncorrectBuyPercentageFeeCalculation() public {
        uint64 perOrderFee = 100000000000000000;       // 1e17
        uint64 percentageFeeRate = 100000000000000000; // 1e17
        uint128 orderValue = 1000000000000000000;      // 1e18

        vm.assume(perOrderFee >= 1e17);
        vm.assume(percentageFeeRate < 1 ether);
        vm.assume(percentageFeeRate >= 1e17);
        vm.assume(orderValue >= 1 ether);

        OrderFees fees = new OrderFees(address(this), perOrderFee, percentageFeeRate);

        // BuyIssuer
        buyIssuer.setOrderFees(fees);

L1:     (uint256 inputValue, uint256 flatFee1, uint256 percentageFee1) =
            buyIssuer.getInputValueForOrderValue(address(paymentToken), orderValue);

L2:     assertEq(inputValue - flatFee1 - percentageFee1, orderValue);

L3:     (uint256 flatFee2, uint256 percentageFee2) = buyIssuer.getFeesForOrder(address(paymentToken), inputValue);
L4:     assertEq(flatFee1, flatFee2);
L5:     assertEq(percentageFee1, percentageFee2);

        // SellIssuer
        sellIssuer.setOrderFees(fees);

L6:     uint256 flatFee3 = sellIssuer.getFlatFeeForOrder(address(paymentToken));
L7:     uint256 percentageFee3 = sellIssuer.getPercentageFeeForOrder(orderValue);

L8:     assertEq(flatFee3, flatFee2);
L9:     assertNotEq(percentageFee3, percentageFee2);
    }

In L1: (uint256 inputValue, uint256 flatFee1, uint256 percentageFee1) = buyIssuer.getInputValueForOrderValue(address(paymentToken), orderValue);

  • The buyIssuer.getInputValueForOrderValue() requires an order value as input.
  • Thus, the orderValue was inputted into the buyIssuer.getInputValueForOrderValue(), and we received inputValue, flatFee1, and percentageFee1 as return parameters.

In L2: assertEq(inputValue - flatFee1 - percentageFee1, orderValue);

  • We can confirm that the return parameters from L1 were correct since inputValue - flatFee1 - percentageFee1 == orderValue.

In L3: (uint256 flatFee2, uint256 percentageFee2) = buyIssuer.getFeesForOrder(address(paymentToken), inputValue);

  • The buyIssuer.getFeesForOrder() requires a total input value (including fees) as input.
  • That's why the inputValue from L1 was inputted into the buyIssuer.getFeesForOrder(), and we received flatFee2 and percentageFee2 as return parameters.

In L4: assertEq(flatFee1, flatFee2);

  • To confirm that the implementation of both getInputValueForOrderValue() and getFeesForOrder() above is correct, the returned flatFee1 and flatFee2 must be equal.
  • That's why I executed the assertEq(flatFee1, flatFee2); in L4.

In L5: assertEq(percentageFee1, percentageFee2);

In L6: uint256 flatFee3 = sellIssuer.getFlatFeeForOrder(address(paymentToken));

In L7: uint256 percentageFee3 = sellIssuer.getPercentageFeeForOrder(orderValue);

  • The sellIssuer.getPercentageFeeForOrder() requires an order value as an input and would return a corresponding percentage fee percentageFee3.

In L8: assertEq(flatFee3, flatFee2);

  • With the same input parameters, the flat fees and percentage fees of the BUY order and SELL order must be equal.
  • Hence, I compared the flatFee3 (for the SELL order) and flatFee2 (for the BUY order) using the assertEq(flatFee3, flatFee2);.

In L9: assertNotEq(percentageFee3, percentageFee2);

  • Finally, I compared the percentageFee3 (for the SELL order) and percentageFee2 (for the BUY order).
  • Since the percentage fee for the BUY order (percentageFee2) was miscalculated, the execution of the assertNotEq(percentageFee3, percentageFee2); was true.
  • Specifically, the percentageFee3 (for the SELL order) and percentageFee2 (for the BUY order) should be the same, but they were not.
  • Again, I didn't invent the test code in L6 - L9 myself but slightly modified it from the test code written by the protocol developer. Therefore, the code must be correct.

The following presents an excerpt from the Root Cause Analysis section:

From my deeper analysis, I found that the orderRequest.quantityIn param (L113) was the sum of order.paymentTokenQuantity and order.fee (containing both the flatFee and percentageFee).

FILE: https://github.com/sherlock-audit/2023-06-dinari-serial-coder/blob/edf9d85f37395902978633b4e9ec53caa1df969a/sbt-contracts/src/issuer/BuyOrderIssuer.sol
LOCATIONS: 71, 73, and 113

57:     function getFeesForOrder(address token, uint256 inputValue) //@audit inputValue = paymentTokenQuantity + fee (containing both the flatFee and percentageFee)
58:         public
59:         view
60:         returns (uint256 flatFee, uint256 percentageFee)
61:     {
62:         // Check if fee contract is set
63:         if (address(orderFees) == address(0)) {
64:             return (0, 0);
65:         }
66: 
67:         // Calculate fees
68:         flatFee = orderFees.flatFeeForOrder(token);
69:         // If input value is greater than flat fee, calculate percentage fee on remaining value
70:         if (inputValue > flatFee) {
71: @>          percentageFee = orderFees.percentageFeeForValue(inputValue - flatFee); //@audit the input = paymentTokenQuantity + percentageFee
72:         } else {
73: @>          percentageFee = 0; //@audit if inputValue <= flatFee, percentageFee = 0 (this is inconsistent with the percentage fee calculation of the SELL orders)
74:         }
75:     }

      // ...SNIPPED...

106:  function _requestOrderAccounting(OrderRequest calldata orderRequest, bytes32 orderId)
107:        internal
108:        virtual
109:        override
110:        returns (Order memory order)
111:    {
112:        // Determine fees
113: @>     (uint256 flatFee, uint256 percentageFee) = getFeesForOrder(orderRequest.paymentToken, orderRequest.quantityIn); //@audit orderRequest.quantityIn = order.paymentTokenQuantity + order.fee
114:        uint256 totalFees = flatFee + percentageFee;

          // ...SNIPPED...
142:    }

In L71, the percentage fee was calculated by inputting the inputValue - flatFee into the orderFees.percentageFeeForValue(). Let's dig deeper into the input of the percentageFeeForValue().

input = inputValue - flatFee
      = ( paymentTokenQuantity + fee (containing both the flatFee and percentageFee) ) - flatFee
      = paymentTokenQuantity + percentageFee

As you can see, there was the precomputed percentageFee inside the input of the percentageFeeForValue().

You've deleted an escalation for this issue.
Oot2k commented 1 year ago

I think this is a design choice, and considered low/invalid regarding sherlock rules.

serial-coder commented 1 year ago

I think this is a design choice, and considered low/invalid regarding sherlock rules.

Even if I can prove the impact, do you still consider this an invalid issue? (see the proof of impact below)


I decided to extend my PoC to test what if I change the percentageFeeRate parameter, and the below shows the result.

Assume that the following are constants. perOrderFee = 100000000000000000 (1e18) orderValue = 1000000000000000000 (1e17)

  1. If percentageFeeRate = 100000000000000000 ( 1 * 1e17 (10% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 100000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 111111111111111111
  percentageFee2 (BUY order): 111111111111111111
  percentageFee3 (SELL order): 100000000000000000
  1. If percentageFeeRate = 200000000000000000 ( 2 * 1e17 (20% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 200000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 250000000000000000
  percentageFee2 (BUY order): 250000000000000000
  percentageFee3 (SELL order): 200000000000000000
  1. If percentageFeeRate = 300000000000000000 ( 3 * 1e17 (30% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 300000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 428571428571428571
  percentageFee2 (BUY order): 428571428571428571
  percentageFee3 (SELL order): 300000000000000000
  1. If percentageFeeRate = 400000000000000000 ( 4 * 1e17 (40% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 400000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 666666666666666666
  percentageFee2 (BUY order): 666666666666666666
  percentageFee3 (SELL order): 400000000000000000
  1. If percentageFeeRate = 500000000000000000 ( 5 * 1e17 (50% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 500000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 1000000000000000000
  percentageFee2 (BUY order): 1000000000000000000
  percentageFee3 (SELL order): 500000000000000000
  1. If percentageFeeRate = 600000000000000000 ( 6 * 1e17 (60% fee rate) ),
  perOrderFee: 100000000000000000
  percentageFeeRate: 600000000000000000
  orderValue: 1000000000000000000
  flatFee1 (BUY order): 100000
  flatFee2 (BUY order): 100000
  flatFee3 (SELL order): 100000
  percentageFee1 (BUY order): 1500000000000000000
  percentageFee2 (BUY order): 1500000000000000000
  percentageFee3 (SELL order): 600000000000000000