The fee logic is massively flawed and unfair to all parties _(holders, creators & platform)_ if/when protocol scales large enough

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/bonding_curve/LinearBondingCurve.sol#L14-L36

Vulnerability details

Proof of Concept

Take a look at LinearBondingCurve.sol#L14-L36

 function getPriceAndFee(uint256 shareCount, uint256 amount)
        external
        view
        override
        returns (uint256 price, uint256 fee)
    {
        for (uint256 i = shareCount; i < shareCount + amount; i++) {
            uint256 tokenPrice = priceIncrease * i;
            price += tokenPrice;
            fee += (getFee(i) * tokenPrice) / 1e18;
        }
    }


    function getFee(uint256 shareCount) public pure override returns (uint256) {
        uint256 divisor;
        if (shareCount > 1) {
            divisor = log2(shareCount);
        } else {
            divisor = 1;
        }
        // 0.1 / log2(shareCount)
        return 1e17 / divisor;
    }

As seen, these functions are used in order to get the price and the fee for some executions in Market.sol, i.e getBuyPrice() getSellPrice() & even getNFTMintingPrice(), issue with this is that protocol currently claims that the value of the fee returned should also be increasing as the share count increases, but slower, case is that current implementation does not take into account that the value of the log2(shareCount) is susceptible to not only a precision loss which would lead to the value being truncated, but also the fact that the log2 was implemented which leads to the fees being almost non-increasing, for example a 10,000% increase in share count, would lead to ~78% increase in fees, take the graph below as a case study.

The graph above is for ( x = \log_2(y) ) , where as already explained, x is the fee and y is the value of share count, it's already obvious how massive the difference in growth is.

If we'd like to argue that the amount of share count would never be this massive, we can still use a lower value of share counts, to put this in place, the difference of fees between a share count of 100 & 1,000 (which is a 1000% increase) is actually just _~ 49% increase_, and only when the value of share count has increased by 10,000% using 100 as a starting point does the fee increases by roughly 100%, keep in mind that in these cases the precision loss that would happen while truncating this in solidity is not even taken into consideration.

Impact

A heavy flaw in the fee logic, asides the fact that one could say the allocated fees are almost stagnant, another issue could be the precision loss that's going to be attached to this, naturally by taking the log2 of share count between 1024 <= shareCount < 2048 to their whole numbers, they would inherently be truncated to 10, I believe multiple examples could be made on how this could go very bad, which in any case eventually leads to not providing enough fees for all three parties, i.e the creator, holder & platform.

Tool Used

• Manual Review
• Desmos's Graphing Calculator.

Recommended Mitigation Steps

I'd recommend using a different fee growth logic, i.e not the log2 of share counts, if at all this can't be implemented, then protocol really needs to take into account the precision loss that happens here and add more precision to the returned from here.

Assessed type

Context

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-sponsor]

OpenCoreCH marked the issue as disagree with severity

[c4-sponsor]

OpenCoreCH (sponsor) acknowledged

[OpenCoreCH]

Fee logic is a design decision (with the explicit decision for slow decrease), the precision loss is addressed in other issues.

[c4-judge]

MarioPoneder marked the issue as duplicate of #25

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b