Vulnerability in `_getDiscountedPaymentAmount` Function

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0xa6911970c7e08a71b2cf2dc8391f93cdfbae6a90a9d65b9d69d94ed32e20b758 Severity: medium

Description: Description\ The function _getDiscountedPaymentAmount is designed to compute the discounted payment amount for a given OTC amount in USD. The function parameters include the OTC amount, the payment token valuation, the discount in basis points (BPS), and the payment token's decimal precision. The function ensures that the payment amount accounts for both the discount and the token's decimal places.

However, the function contains several vulnerabilities and lacks certain validation checks, which can result in incorrect calculations, potential division by zero errors, and mismanagement of token decimal adjustments. The vulnerabilities in this function could potentially lead to financial losses or system malfunctions if exploited.

Attack Scenario\

Division by Zero: If an attacker passes a value of zero for _paymentTokenValuation, the function will revert with PaymentTokenValuationNotValid(), which is appropriate. However, if _paymentTokenDecimals is set to zero, the function does not handle this edge case, potentially leading to unintended behaviors when adjusting decimals.
Precision Loss: The function's division operations can lead to precision loss due to integer division truncation. An attacker could manipulate the values of _otcAmountInUSD and _paymentTokenValuation to exploit this loss in precision, potentially leading to incorrect payment amounts.
Overflow and Underflow: Although Solidity 0.8.x includes built-in overflow and underflow checks, complex arithmetic operations without explicit handling might still pose risks. An attacker might exploit extreme values for _otcAmountInUSD, _paymentTokenValuation, or _discount to cause overflows or underflows in the calculation, leading to incorrect results.
Improper Decimal Handling: If _paymentTokenDecimals is incorrectly set outside the valid range (1-18), the function does not revert, which can result in invalid token amount calculations. An attacker could exploit this by passing improper decimal values, causing erroneous payment amounts.

Attachments

Proof of Concept (PoC) File

contract VulnerabilityPoC {

error PaymentTokenValuationNotValid();
error PaymentTokenDecimalsNotValid();

function _getDiscountedPaymentAmount(
    uint256 _otcAmountInUSD,
    uint256 _paymentTokenValuation,
    uint256 _discount,
    uint256 _paymentTokenDecimals
) internal pure returns (uint256 paymentAmount) {
    if (_paymentTokenValuation == 0) revert PaymentTokenValuationNotValid();
    if (_paymentTokenDecimals == 0 || _paymentTokenDecimals > 18) revert PaymentTokenDecimalsNotValid();

    uint256 rawPaymentAmount = muldiv(_otcAmountInUSD, 1e18, _paymentTokenValuation);
    paymentAmount = rawPaymentAmount - muldiv(rawPaymentAmount, _discount, 100e4);

    if (_paymentTokenDecimals < 18) {
        paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
    } else if (_paymentTokenDecimals > 18) {
        paymentAmount = paymentAmount * (10 ** (_paymentTokenDecimals - 18));
    }
}

I was wrong about it. It should to be like this

function _getDiscountedPaymentAmount(
    uint256 _otcAmountInUSD,
    uint256 _paymentTokenValuation,
    uint256 _discount,
    uint256 _paymentTokenDecimals
) internal pure returns (uint256 paymentAmount) {
    if (_paymentTokenValuation == 0) revert PaymentTokenValuationNotValid();
    if (_discount > 10000) revert DiscountNotValid(); // Ensure the discount is within a valid range
    if (_paymentTokenDecimals > 18) revert PaymentTokenDecimalsNotValid(); // Ensure decimals are within a valid range

    // Calculate raw payment amount with full precision
    uint256 rawPaymentAmount = muldiv(_otcAmountInUSD, 10**18, _paymentTokenValuation);

    // Apply discount
    uint256 discountAmount = muldiv(rawPaymentAmount, _discount, 10000);
    paymentAmount = rawPaymentAmount - discountAmount;

    // Adjust for payment token decimals
    if (_paymentTokenDecimals < 18) {
        paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
    } else if (_paymentTokenDecimals == 18) {
        // No adjustment needed
    } else {
        paymentAmount = paymentAmount * (10 ** (_paymentTokenDecimals - 18));
    }

    return paymentAmount;
}

Hi @Jelev123 thanks for the submission:

well the thing is, will those scenarios be reached at some point? Does not seem like it.

Division by Zero: Are we going to have _paymentTokenDecimals as 0? I don't think so
Precision Loss: This is not really tested. Could be 1 wei, most of precision loses are acceptable if they can't be leveraged
Overflow and Underflow: need a PoC of what values you mean
Improper Decimal Handling: I don't think we will use a tokens > 18 decimals

We need a PoC for the "precision loss" values and the values at which you claim it will over/under flow, otherwise we can't consider this submission as a medium. We need to know what the impact is in what values.

Additionally, we do have some checks prior the function call. So, if you could send the PoC with the accurate values, that would be great! Ty

Hey @Jelev123 , this was a great submission and we thank you for the efforts. However we want to note that there are some few mistakes with your PoC, and that at the end, even after rectifying those, our implementation is the one that we want. Let us know if we missed something.

Comparing the 2, and using certain values, a difference is noticed between the 2, however, your implementation favours the external actor instead of the protocol, which can potentially be exploited. We've written a PoC demonstrating this below.

If the test doesn't fail, try to run it again until it does, sometimes forge doesn't use the right values to trigger the difference.

Output

  Hats: 99999899999
  tapioca: 99999900000
  Error: a == b not satisfied [uint]
        Left: 99999899999
       Right: 99999900000

On the left is your implementation, which requires less payment token to pay for the TAP, which is bad because we always want to round up values on payment scenarios.

PoC

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

import "forge-std/Test.sol";
import {TWAML} from "tap-token/options/twAML.sol";

contract DiscountTest is Test, TWAML {
    error Invalid();

    /// @notice Computes the discounted payment amount for a given OTC amount in USD
    ///  _otcAmountInUSD The OTC amount in USD, 18 decimals
    ///  _paymentTokenValuation The payment token valuation in USD, 18 decimals
    ///  _discount The discount in BPS
    ///  _paymentTokenDecimals The payment token decimals
    ///  paymentAmount The discounted payment amount
    function testFuzz_GetDiscount(
        uint256 _otcAmountInUSD,
        uint256 _paymentTokenValuation,
        uint256 _discount,
        uint8 _paymentTokenDecimals
    ) public {
        _otcAmountInUSD = bound(_otcAmountInUSD, 1e36, 1e40); // between 1 TAP and 10m TAP. Starts breaking at 1e47, or 100m TAP
        _paymentTokenValuation = bound(_paymentTokenValuation, 1e17, 1e18); // between 0.1 USD and 1 USD
        vm.assume(_discount <= 100e4); // 0-100% discount
        _paymentTokenDecimals = uint8(bound(_paymentTokenDecimals, 6, 18)); // between 6 and 18 decimals

        uint256 hats =
            _getDiscountedPaymentAmountHats(_otcAmountInUSD, _paymentTokenValuation, _discount, _paymentTokenDecimals);
        uint256 tapioca = _getDiscountedPaymentAmountTapioca(
            _otcAmountInUSD, _paymentTokenValuation, _discount, _paymentTokenDecimals
        );

        console2.log("Hats: %s", hats);
        console2.log("tapioca: %s", tapioca);
        assertEq(hats, tapioca);
    }

    function _getDiscountedPaymentAmountHats(
        uint256 _otcAmountInUSD,
        uint256 _paymentTokenValuation,
        uint256 _discount,
        uint256 _paymentTokenDecimals
    ) internal pure returns (uint256 paymentAmount) {
        if (_paymentTokenValuation == 0) revert Invalid();
        if (_discount > 10000) revert Invalid(); // Ensure the discount is within a valid range
        if (_paymentTokenDecimals > 18) revert Invalid(); // Ensure decimals are within a valid range
        if (_paymentTokenDecimals < 6) revert Invalid(); // Ensure decimals are within a valid range

        // Calculate raw payment amount with full precision
        uint256 rawPaymentAmount = muldiv(_otcAmountInUSD, 10 ** 18, _paymentTokenValuation);

        // Apply discount
        uint256 discountAmount = muldiv(rawPaymentAmount, _discount, 100e4);
        paymentAmount = rawPaymentAmount - discountAmount;

        // Adjust for payment token decimals
        if (_paymentTokenDecimals < 18) {
            paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
        } else if (_paymentTokenDecimals == 18) { // @protocol could be _paymentTokenDecimals >= 18 to avoid void branching
            // No adjustment needed
        } else {
            paymentAmount = paymentAmount * (10 ** (_paymentTokenDecimals - 18));
        }
        paymentAmount = paymentAmount / 1e18; // @protocol auditor forgot to remove the extra 1e18 - Remove extra precision

        return paymentAmount;
    }

    function _getDiscountedPaymentAmountTapioca(
        uint256 _otcAmountInUSD,
        uint256 _paymentTokenValuation,
        uint256 _discount,
        uint256 _paymentTokenDecimals
    ) internal pure returns (uint256 paymentAmount) {
        if (_paymentTokenValuation == 0) revert Invalid();

        uint256 discountedOTCAmountInUSD = _otcAmountInUSD - muldiv(_otcAmountInUSD, _discount, 100e4); // 1e4 is discount decimals, 100 is discount percentage

        // Calculate payment amount
        paymentAmount = discountedOTCAmountInUSD / _paymentTokenValuation;

        if (_paymentTokenDecimals <= 18) {
            paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
        } else {
            paymentAmount = paymentAmount * (10 ** (_paymentTokenDecimals - 18));
        }
    }
}

I tried to run my test but i can`t i will try and will reply if i find something

hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

Vulnerability in `_getDiscountedPaymentAmount` Function #8