Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Ch_301 - The _fee() function is wrongly implemented in the code #95

Open
sherlock-admin opened this issue Nov 22, 2022 · 1 comment
Open

Comments

@sherlock-admin
Copy link
Contributor

Ch_301

high

The _fee() function is wrongly implemented in the code

Summary

_fee() function is wrongly implemented in the code so the protocol will get fewer fees and the trader will earn more

Vulnerability Detail

        (uint256 unitFee, , ) = _fees(10**decimals(), settlementFeePercentage);
        amount = (newFee * 10**decimals()) / unitFee;

let's say we have:
newFee 100 USDC
USDC Decimals is 6
settlementFeePercentage is 20% ==> 200

The unitFee will be 520_000

amount = (100 * 1_000_000) / 520_000
amount = 192 USDC
Which is supposed to be amount = 160 USDC

Impact

The protocol will earn fees less than expected

Code Snippet

       function checkParams(OptionParams calldata optionParams)
        external
        view
        override
        returns (
            uint256 amount,
            uint256 revisedFee,
            bool isReferralValid
        )
    {
        require(
            assetCategory != AssetCategory.Forex ||
                isInCreationWindow(optionParams.period),
            "O30"
        );

        uint256 maxAmount = getMaxUtilization();

        // Calculate the max fee due to the max txn limit
        uint256 maxPerTxnFee = ((pool.availableBalance() *
            config.optionFeePerTxnLimitPercent()) / 100e2);
        uint256 newFee = min(optionParams.totalFee, maxPerTxnFee);

        // Calculate the amount here from the new fees
        uint256 settlementFeePercentage;
        (
            settlementFeePercentage,
            isReferralValid
        ) = _getSettlementFeePercentage(
            referral.codeOwner(optionParams.referralCode),
            optionParams.user,
            _getbaseSettlementFeePercentage(optionParams.isAbove),
            optionParams.traderNFTId
        );
        (uint256 unitFee, , ) = _fees(10**decimals(), settlementFeePercentage);
        amount = (newFee * 10**decimals()) / unitFee;

https://github.com/bufferfinance/Buffer-Protocol-v2/blob/83d85d9b18f1a4d09c728adaa0dde4c37406dfed/contracts/core/BufferBinaryOptions.sol#L318-L353

    function _fees(uint256 amount, uint256 settlementFeePercentage)
        internal
        pure
        returns (
            uint256 total,
            uint256 settlementFee,
            uint256 premium
        )
    {
        // Probability for ATM options will always be 0.5 due to which we can skip using BSM
        premium = amount / 2;
        settlementFee = (amount * settlementFeePercentage) / 1e4;
        total = settlementFee + premium;
    }

https://github.com/bufferfinance/Buffer-Protocol-v2/blob/83d85d9b18f1a4d09c728adaa0dde4c37406dfed/contracts/core/BufferBinaryOptions.sol#L424-L437

Tool used

Manual Review

Recommendation

The _fee() function needs to calculate the fees in this way

total_fee = (5000 * amount)/ (10000 - sf)
@IAm0x52
Copy link
Collaborator

IAm0x52 commented Dec 6, 2022

Fixed in PR#22

Changes look good. New math returns correct values. Validated for both unit fee and option amount.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants