Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creator fee calculation bypass in _getCreatorFee can lead to unexpected fees on zero supply #382

Open
howlbot-integration bot opened this issue Sep 23, 2024 · 3 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_116_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/curve/BondingCurve.sol#L128-L151

Vulnerability details

Impact

The _getCreatorFee function allows the creator fee calculation to bypass the zero supply check. As a result, unexpected creator fees being charged when the supply is zero, contradicting the behavior as seen in other parts of the codebase.

POC

Consider the _getCreatorFee function:

function _getCreatorFee(
    uint256 credId_,
    uint256 supply_,
    uint256 price_,
    bool isSign_
)
    internal
    view
    returns (uint256 creatorFee)
{
    if (!credContract.isExist(credId_)) {
        return 0;
    }
    if (supply_ == 0) {
        creatorFee = 0;
    }

    (uint16 buyShareRoyalty, uint16 sellShareRoyalty) = credContract.getCreatorRoyalty(credId_);

    uint16 royaltyRate = isSign_ ? buyShareRoyalty : sellShareRoyalty;
    creatorFee = (price_ * royaltyRate) / RATIO_BASE;
}

The function sets creatorFee = 0 when supply_ == 0, but it doesn't return immediately. Instead it continues execution and calculates a new creatorFee value based on the royaltyRate, potentially overwriting the zero value.

This means that even when the supply is zero, a creator fee might still be charged, which is likely not the intended behavior based on this part of the code in the getPriceData function that sets creator fee to 0 when supply is 0.

Tools Used

None

Recommended Mitigation Steps

Consider modifying the _getCreatorFee function to return immediately when supply_ == 0

function _getCreatorFee(
    uint256 credId_,
    uint256 supply_,
    uint256 price_,
    bool isSign_
)
    internal
    view
    returns (uint256 creatorFee)
{
    if (!credContract.isExist(credId_)) {
        return 0;
    }
    if (supply_ == 0) {
-       creatorFee = 0;
+       return 0;
    }

    (uint16 buyShareRoyalty, uint16 sellShareRoyalty) = credContract.getCreatorRoyalty(credId_);

    uint16 royaltyRate = isSign_ ? buyShareRoyalty : sellShareRoyalty;
    creatorFee = (price_ * royaltyRate) / RATIO_BASE;
}

Assessed type

Context

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_116_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 23, 2024
howlbot-integration bot added a commit that referenced this issue Sep 23, 2024
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 23, 2024
@c4-judge
Copy link
Contributor

fatherGoose1 changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

fatherGoose1 marked the issue as grade-a

@fatherGoose1
Copy link

Valid issue, but only affects calculations on the frontend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_116_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants