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

FeeSplitter.onBalanceChange doesn't claim already earned fees #66

Closed
c4-bot-7 opened this issue Jan 9, 2024 · 4 comments
Closed

FeeSplitter.onBalanceChange doesn't claim already earned fees #66

c4-bot-7 opened this issue Jan 9, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-39 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-bot-7
Copy link
Contributor

c4-bot-7 commented Jan 9, 2024

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L96-L100

Vulnerability details

Proof of Concept

Subject token holders receive some portion of fees from subject token trading.
First, user's data.userFeeOffset[account] is updated to current reward rate of token. And then fee is distributed among all subject token holders and reward rate is increased.

The problem is that when onBalanceChange is called, then function doesn't account situation, when user's balance was not 0 before and that he already accrued some fee. In this case they are not claimed and userFeeOffset is updated, which means that user has lost that portion of fees.

Impact

User loses part of fee.

Tools Used

VsCode

Recommended Mitigation Steps

Make sure that all earned fees are accumulated and then update the rate.

Assessed type

Error

@c4-bot-7 c4-bot-7 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jan 9, 2024
c4-bot-7 added a commit that referenced this issue Jan 9, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Jan 16, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #39

@c4-judge
Copy link
Contributor

c4-judge commented Feb 1, 2024

alcueca marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Feb 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Feb 1, 2024

alcueca changed the severity to 3 (High Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge labels Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-39 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants